Skip to content

Commit e8b9ca1

Browse files
alubennikovaalubennikova
committed
code cleanup, add comments
1 parent 52bf25c commit e8b9ca1

File tree

6 files changed

+65
-51
lines changed

6 files changed

+65
-51
lines changed

‎src/backup.c

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,10 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
315315
dir_list_file(backup_files_list, instance_config.pgdata,
316316
true, true, false, 0, FIO_DB_HOST);
317317

318-
/* create database_map used for partial restore */
318+
/*
319+
* Get database_map (name to oid) for use in partial restore feature.
320+
* It's possible that we fail and database_map will be NULL.
321+
*/
319322
database_map = get_database_map(pg_startbackup_conn);
320323

321324
/*
@@ -573,7 +576,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
573576
if (database_map)
574577
{
575578
write_database_map(&current, database_map, backup_files_list);
576-
/* we don`t need it anymore */
579+
/* cleanup */
577580
parray_walk(database_map, db_map_entry_free);
578581
parray_free(database_map);
579582
}
@@ -1065,8 +1068,15 @@ pg_ptrack_support(PGconn *backup_conn)
10651068
}
10661069

10671070
/*
1068-
* Create 'datname to Oid' map
1069-
* Return NULL if failed to construct database_map // TODO doesn't look safe. See comment below.
1071+
* Fill 'datname to Oid' map
1072+
*
1073+
* This function can fail to get the map for legal reasons, e.g. missing
1074+
* permissions on pg_database during `backup`.
1075+
* As long as user do not use partial restore feature it`s fine.
1076+
*
1077+
* To avoid breaking a backward compatibility don't throw an ERROR,
1078+
* throw a warning instead of an error and return NULL.
1079+
* Caller is responsible for checking the result.
10701080
*/
10711081
parray *
10721082
get_database_map(PGconn *conn)
@@ -1075,18 +1085,16 @@ get_database_map(PGconn *conn)
10751085
parray *database_map = NULL;
10761086
int i;
10771087

1078-
/* TODO add a comment why we exclude template0 and template1 from the map */
1088+
/*
1089+
* Do not include template0 and template1 to the map
1090+
* as default databases that must always be restored.
1091+
*/
10791092
res = pgut_execute_extended(conn,
10801093
"SELECT oid, datname FROM pg_catalog.pg_database "
10811094
"WHERE datname NOT IN ('template1', 'template0')",
10821095
0, NULL, true, true);
10831096

1084-
1085-
/* TODO How is that possible? Shouldn't instance have at least one database?
1086-
* How can we distinguish case when instance only has template databases
1087-
* and case of query failure?
1088-
* Is it ok to ignore the failure?
1089-
*/
1097+
/* Don't error out, simply return NULL. See comment above. */
10901098
if (PQresultStatus(res) != PGRES_TUPLES_OK)
10911099
{
10921100
PQclear(res);
@@ -1116,15 +1124,6 @@ get_database_map(PGconn *conn)
11161124
parray_append(database_map, db_entry);
11171125
}
11181126

1119-
/* extra paranoia */
1120-
// TODO This code block has no value. Let's delete it.
1121-
if (database_map && (parray_num(database_map) == 0))
1122-
{
1123-
parray_free(database_map);
1124-
elog(WARNING, "Failed to get database map");
1125-
return NULL;
1126-
}
1127-
11281127
return database_map;
11291128
}
11301129

‎src/data.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1096,7 +1096,7 @@ create_empty_file(fio_location from_location, const char *to_root,
10961096
}
10971097

10981098
if (fio_fclose(out))
1099-
elog(ERROR, "cannot write \"%s\": %s", to_path, strerror(errno));
1099+
elog(ERROR, "cannot close \"%s\": %s", to_path, strerror(errno));
11001100

11011101
return true;
11021102
}

‎src/dir.c

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,22 @@ BlackListCompare(const void *str1, const void *str2)
445445
return strcmp(*(char **) str1, *(char **) str2);
446446
}
447447

448+
/* Compare two Oids */
449+
int
450+
pgCompareOid(const void *f1, const void *f2)
451+
{
452+
Oid v1 = *(Oid *)f1;
453+
Oid v2 = *(Oid *)f2;
454+
455+
elog(WARNING, "pgCompareOid %u %u", v1, v2);
456+
if (v1 > v2)
457+
return 1;
458+
else if (v1 < v2)
459+
return -1;
460+
else
461+
return 0;}
462+
463+
448464
void
449465
db_map_entry_free(void *entry)
450466
{
@@ -1679,7 +1695,7 @@ print_database_map(FILE *out, parray *database_map)
16791695
}
16801696

16811697
/*
1682-
* Create file 'database_map' and add its meta to backup_content.control
1698+
* Create file 'database_map' and add its meta to backup_files_list
16831699
* NULL check for database_map must be done by the caller.
16841700
*/
16851701
void
@@ -1709,8 +1725,10 @@ write_database_map(pgBackup *backup, parray *database_map, parray *backup_files_
17091725
/* Add metadata to backup_content.control */
17101726
file = pgFileNew(database_map_path, DATABASE_MAP, true, 0,
17111727
FIO_BACKUP_HOST);
1712-
file->crc = pgFileGetCRC(file->path, true, false,
1713-
&file->read_size, FIO_BACKUP_HOST);
1728+
pfree(file->path);
1729+
file->path = strdup(DATABASE_MAP);
1730+
file->crc = pgFileGetCRC(database_map_path, true, false,
1731+
&file->read_size, FIO_BACKUP_HOST);
17141732
file->write_size = file->read_size;
17151733
parray_append(backup_files_list, file);
17161734
}

‎src/pg_probackup.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -641,7 +641,6 @@ main(int argc, char *argv[])
641641
if (datname_exclude_list && datname_include_list)
642642
elog(ERROR, "You cannot specify '--db-include' and '--db-exclude' together");
643643

644-
/* At this point we are sure that user requested partial restore */
645644
if (datname_exclude_list)
646645
datname_list = datname_exclude_list;
647646

@@ -777,7 +776,7 @@ opt_datname_exclude_list(ConfigOption *opt, const char *arg)
777776

778777
dbname = pgut_malloc(strlen(arg) + 1);
779778

780-
/* add sanity for database name */
779+
/* TODO add sanity for database name */
781780
strcpy(dbname, arg);
782781

783782
parray_append(datname_exclude_list, dbname);
@@ -794,7 +793,7 @@ opt_datname_include_list(ConfigOption *opt, const char *arg)
794793

795794
dbname = pgut_malloc(strlen(arg) + 1);
796795

797-
/* add sanity for database name */
796+
/* TODO add sanity for database name */
798797
strcpy(dbname, arg);
799798

800799
parray_append(datname_include_list, dbname);

‎src/pg_probackup.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,7 @@ extern int pgFileComparePathDesc(const void *f1, const void *f2);
654654
extern int pgFileComparePathWithExternalDesc(const void *f1, const void *f2);
655655
extern int pgFileCompareLinked(const void *f1, const void *f2);
656656
extern int pgFileCompareSize(const void *f1, const void *f2);
657+
extern int pgCompareOid(const void *f1, const void *f2);
657658

658659
/* in data.c */
659660
extern bool check_data_file(ConnectionArgs* arguments, pgFile* file, uint32 checksum_version);

‎src/restore.c

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ static void *restore_files(void *arg);
4444

4545
static parray *get_dbOid_exclude_list(pgBackup *backup, parray *files,
4646
parray *datname_list, bool partial_restore_type);
47-
48-
static int pgCompareOid(const void *f1, const void *f2);
4947
static void set_orphan_status(parray *backups, pgBackup *parent_backup);
5048

5149
/*
@@ -482,7 +480,7 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
482480
}
483481

484482
/*
485-
* At least restore backups files starting from the parent backup.
483+
* Restore backups files starting from the parent backup.
486484
*/
487485
for (i = parray_num(parent_chain) - 1; i >= 0; i--)
488486
{
@@ -1185,27 +1183,38 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
11851183
int j;
11861184
parray *database_map = NULL;
11871185
parray *dbOid_exclude_list = NULL;
1188-
bool found_database_map = false;
1186+
pgFile *database_map_file = NULL;
1187+
pg_crc32 crc;
1188+
char path[MAXPGPATH];
1189+
char database_map_path[MAXPGPATH];
11891190

11901191
/* make sure that database_map is in backup_content.control */
1191-
// TODO can't we use parray_bsearch here?
11921192
for (i = 0; i < parray_num(files); i++)
11931193
{
11941194
pgFile *file = (pgFile *) parray_get(files, i);
11951195

11961196
if ((file->external_dir_num == 0) &&
1197-
strcmp(DATABASE_MAP, file->rel_path) == 0)
1197+
strcmp(DATABASE_MAP, file->name) == 0)
11981198
{
1199-
found_database_map = true;
1199+
database_map_file = file;
12001200
break;
12011201
}
12021202
}
12031203

1204-
// TODO rephrase error message
1205-
if (!found_database_map)
1206-
elog(ERROR, "Backup %s has missing database_map, partial restore is impossible.",
1204+
if (!database_map_file)
1205+
elog(ERROR, "Backup %s doesn't contain a database_map, partial restore is impossible.",
12071206
base36enc(backup->start_time));
12081207

1208+
pgBackupGetPath(backup, path, lengthof(path), DATABASE_DIR);
1209+
join_path_components(database_map_path, path, DATABASE_MAP);
1210+
1211+
/* check database_map CRC */
1212+
crc = pgFileGetCRC(database_map_path, true, true, NULL, FIO_LOCAL_HOST);
1213+
1214+
if (crc != database_map_file->crc)
1215+
elog(ERROR, "Invalid CRC of backup file \"%s\" : %X. Expected %X",
1216+
database_map_file->path, crc, database_map_file->crc);
1217+
12091218
/* get database_map from file */
12101219
database_map = read_database_map(backup);
12111220

@@ -1215,12 +1224,12 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12151224
base36enc(backup->start_time));
12161225

12171226
/*
1218-
* So we have a list of datnames and database_map for it.
1227+
* So we have a list of datnames and a database_map for it.
12191228
* We must construct a list of dbOids to exclude.
12201229
*/
12211230
if (partial_restore_type)
12221231
{
1223-
/* For 'include' find dbOid of every datname NOT specified by user */
1232+
/* For 'include' keep dbOid of every datname NOT specified by user */
12241233
for (i = 0; i < parray_num(datname_list); i++)
12251234
{
12261235
bool found_match = false;
@@ -1294,15 +1303,3 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12941303

12951304
return dbOid_exclude_list;
12961305
}
1297-
1298-
/* Compare two Oids */
1299-
// TODO is it overflow safe?
1300-
// TODO move it to dir.c like other compare functions
1301-
int
1302-
pgCompareOid(const void *f1, const void *f2)
1303-
{
1304-
Oid *f1p = *(Oid **)f1;
1305-
Oid *f2p = *(Oid **)f2;
1306-
1307-
return (*(Oid*)f1p - *(Oid*)f2p);
1308-
}

0 commit comments

Comments
 (0)