Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit23697ff

Browse files
committed
Review issue_79. Added several TODO comments
1 parent4611ee3 commit23697ff

File tree

4 files changed

+50
-28
lines changed

4 files changed

+50
-28
lines changed

‎src/backup.c

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -493,15 +493,6 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
493493
}
494494
}
495495

496-
/* write database map to file and add it to control file */
497-
if (database_map)
498-
{
499-
write_database_map(&current,database_map,backup_files_list);
500-
/* we don`t need it anymore */
501-
parray_walk(database_map,db_map_entry_free);
502-
parray_free(database_map);
503-
}
504-
505496
/* clean previous backup file list */
506497
if (prev_backup_filelist)
507498
{
@@ -578,6 +569,15 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
578569
parray_free(xlog_files_list);
579570
}
580571

572+
/* write database map to file and add it to control file */
573+
if (database_map)
574+
{
575+
write_database_map(&current,database_map,backup_files_list);
576+
/* we don`t need it anymore */
577+
parray_walk(database_map,db_map_entry_free);
578+
parray_free(database_map);
579+
}
580+
581581
/* Print the list of files to backup catalog */
582582
write_backup_filelist(&current,backup_files_list,instance_config.pgdata,
583583
external_dirs);
@@ -1066,7 +1066,7 @@ pg_ptrack_support(PGconn *backup_conn)
10661066

10671067
/*
10681068
* Create 'datname to Oid' map
1069-
* Return NULL if failed to construct database_map
1069+
* Return NULL if failed to construct database_map // TODO doesn't look safe. See comment below.
10701070
*/
10711071
parray*
10721072
get_database_map(PGconn*conn)
@@ -1075,11 +1075,18 @@ get_database_map(PGconn *conn)
10751075
parray*database_map=NULL;
10761076
inti;
10771077

1078+
/* TODO add a comment why we exclude template0 and template1 from the map */
10781079
res=pgut_execute_extended(conn,
10791080
"SELECT oid, datname FROM pg_catalog.pg_database "
10801081
"WHERE datname NOT IN ('template1', 'template0')",
10811082
0,NULL, true, true);
10821083

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+
*/
10831090
if (PQresultStatus(res)!=PGRES_TUPLES_OK)
10841091
{
10851092
PQclear(res);
@@ -1110,6 +1117,7 @@ get_database_map(PGconn *conn)
11101117
}
11111118

11121119
/* extra paranoia */
1120+
// TODO This code block has no value. Let's delete it.
11131121
if (database_map&& (parray_num(database_map)==0))
11141122
{
11151123
parray_free(database_map);

‎src/data.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1078,7 +1078,7 @@ create_empty_file(fio_location from_location, const char *to_root,
10781078
charto_path[MAXPGPATH];
10791079
FILE*out;
10801080

1081-
/* openbackupfile for write */
1081+
/* open file for write */
10821082
join_path_components(to_path,to_root,file->rel_path);
10831083
out=fio_fopen(to_path,PG_BINARY_W,to_location);
10841084
if (out==NULL)

‎src/dir.c

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,10 @@ pgFileDelete(pgFile *file)
249249
}
250250
}
251251

252+
/*
253+
* Read the file to compute its CRC.
254+
* As a handy side effect, we return filesize via bytes_read parameter.
255+
*/
252256
pg_crc32
253257
pgFileGetCRC(constchar*file_path,booluse_crc32c,boolraise_on_deleted,
254258
size_t*bytes_read,fio_locationlocation)
@@ -442,12 +446,12 @@ BlackListCompare(const void *str1, const void *str2)
442446
}
443447

444448
void
445-
db_map_entry_free(void*map)
449+
db_map_entry_free(void*entry)
446450
{
447-
db_map_entry*m= (db_map_entry*)map;
451+
db_map_entry*m= (db_map_entry*)entry;
448452

449453
free(m->datname);
450-
free(map);
454+
free(entry);
451455
}
452456

453457
/*
@@ -1702,14 +1706,12 @@ write_database_map(pgBackup *backup, parray *database_map, parray *backup_files_
17021706
database_map_path,strerror(errno));
17031707
}
17041708

1705-
/* */
1709+
/*Add metadata to backup_content.control */
17061710
file=pgFileNew(database_map_path,DATABASE_MAP, true,0,
17071711
FIO_BACKUP_HOST);
17081712
file->crc=pgFileGetCRC(file->path, true, false,
17091713
&file->read_size,FIO_BACKUP_HOST);
17101714
file->write_size=file->read_size;
1711-
free(file->path);
1712-
file->path=strdup(DATABASE_MAP);
17131715
parray_append(backup_files_list,file);
17141716
}
17151717

‎src/restore.c

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -457,8 +457,11 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
457457
* It is important that we do this after(!) validation so
458458
* database_map can be trusted.
459459
* NOTE: database_map could be missing for legal reasons, e.g. missing
460-
* permissions on pg_database during `backup` and, as long as user do not request
461-
* partial restore, it`s OK
460+
* permissions on pg_database during `backup` and, as long as user
461+
* do not request partial restore, it`s OK.
462+
*
463+
* If partial restore is requested and database map doesn't exist,
464+
* throw an error.
462465
*/
463466
if (datname_list)
464467
dbOid_exclude_list=get_dbOid_exclude_list(dest_backup,dest_files,datname_list,
@@ -709,19 +712,22 @@ restore_files(void *arg)
709712
i+1, (unsigned long)parray_num(arguments->files),
710713
file->rel_path);
711714

712-
/* only files from pgdata can be skipped by partial restore */
713-
if (arguments->dbOid_exclude_list&&
714-
file->external_dir_num==0)
715+
/* Only files from pgdata can be skipped by partial restore */
716+
if (arguments->dbOid_exclude_list&&file->external_dir_num==0)
715717
{
716-
/*exclude map is not empty */
718+
/*Check if the file belongs to the database we exclude */
717719
if (parray_bsearch(arguments->dbOid_exclude_list,
718720
&file->dbOid,pgCompareOid))
719721
{
720-
/* got a match, destination file will truncated */
722+
/*
723+
* We cannot simply skip the file, because it may lead to
724+
* failure during WAL redo; hence, create empty file.
725+
*/
721726
create_empty_file(FIO_BACKUP_HOST,
722727
instance_config.pgdata,FIO_DB_HOST,file);
723728

724-
elog(VERBOSE,"Exclude file due to partial restore: \"%s\"",file->rel_path);
729+
elog(VERBOSE,"Exclude file due to partial restore: \"%s\"",
730+
file->rel_path);
725731
continue;
726732
}
727733
}
@@ -1176,7 +1182,8 @@ parseRecoveryTargetOptions(const char *target_time,
11761182
returnrt;
11771183
}
11781184

1179-
/* Return array of dbOids of databases that should not be restored
1185+
/*
1186+
* Return array of dbOids of databases that should not be restored
11801187
* Regardless of what option user used, db-include or db-exclude,
11811188
* we always convert it into exclude_list.
11821189
*/
@@ -1191,6 +1198,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
11911198
boolfound_database_map= false;
11921199

11931200
/* make sure that database_map is in backup_content.control */
1201+
// TODO can't we use parray_bsearch here?
11941202
for (i=0;i<parray_num(files);i++)
11951203
{
11961204
pgFile*file= (pgFile*)parray_get(files,i);
@@ -1203,6 +1211,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12031211
}
12041212
}
12051213

1214+
// TODO rephrase error message
12061215
if (!found_database_map)
12071216
elog(ERROR,"Backup %s has missing database_map, partial restore is impossible.",
12081217
base36enc(backup->start_time));
@@ -1215,7 +1224,8 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12151224
elog(ERROR,"Backup %s has empty or mangled database_map, partial restore is impossible.",
12161225
base36enc(backup->start_time));
12171226

1218-
/* So we have list of datnames and database_map for it.
1227+
/*
1228+
* So we have a list of datnames and database_map for it.
12191229
* We must construct a list of dbOids to exclude.
12201230
*/
12211231
if (partial_restore_type)
@@ -1284,7 +1294,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12841294
}
12851295
}
12861296

1287-
/* extra sanity, we must be totally sure that list is not empty */
1297+
/* extra sanity: ensure that list is not empty */
12881298
if (!dbOid_exclude_list||parray_num(dbOid_exclude_list)<1)
12891299
elog(ERROR,"Failed to find a match in database_map of backup %s for partial restore",
12901300
base36enc(backup->start_time));
@@ -1296,6 +1306,8 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
12961306
}
12971307

12981308
/* Compare two Oids */
1309+
// TODO is it overflow safe?
1310+
// TODO move it to dir.c like other compare functions
12991311
int
13001312
pgCompareOid(constvoid*f1,constvoid*f2)
13011313
{

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp