Here are some comments:
+/* The reference XLOG position for the incremental backup. */ +static XLogRecPtr refptr; As Robert already pointed we may want to pass this as parameter around instead of a global variable. Also, can be renamed to something like: incr_backup_refptr. I see in your earlier version of patch this was named startincrptr, which I think was more meaningful. --------- /* * If incremental backup, see whether the filename is a relation filename * or not. */ Can be reworded something like: "If incremental backup, check if it is relation file and can be sent partially." --------- + if (verify_checksum) + { + ereport(WARNING, + (errmsg("cannot verify checksum in file \"%s\", block " + "%d: read buffer size %d and page size %d " + "differ", + readfilename, blkno, (int) cnt, BLCKSZ))); + verify_checksum = false; + } For do_incremental_backup() it does not make sense to show the block number in warning as it is always going to be 0 when we throw this warning. Further, I think this can be rephrased as: "cannot verify checksum in file \"%s\", read file size %d is not multiple of page size %d". Or maybe we can just say: "cannot verify checksum in file \"%s\"" if checksum requested, disable the checksum and leave it to the following message: + ereport(WARNING, + (errmsg("file size (%d) not in multiple of page size (%d), sending whole file", + (int) cnt, BLCKSZ))); --------- If you agree on the above comment for blkno, then we can shift declaration of blkno inside the condition " if (!sendwholefile)" in do_incremental_backup(), or avoid it altogether, and just pass "i" as blkindex, as well as blkno to verify_page_checksum(). May be add a comment why they are same in case of incremental backup. --------- I think we should give the user hint from where he should be reading the input lsn for incremental backup in the --help option as well as documentation? Something like - "To take an incremental backup, please provide value of "--lsn" as the "START WAL LOCATION" of previously taken full backup or incremental backup from backup_lable file. --------- pg_combinebackup: +static bool made_new_outputdata = false; +static bool found_existing_outputdata = false; Both of these are global, I understand that we need them global so that they are accessible in cleanup_directories_atexit(). But they are passed to verify_dir_is_empty_or_create() as parameters, which I think is not needed. Instead verify_dir_is_empty_or_create() can directly change the globals. --------- I see that checksum_failure is never set and always remains as false. May be it is something that you wanted to set in combine_partial_files() when a the corrupted partial file is detected? --------- I think the logic for verifying the backup chain should be moved out from main() function to a separate function. --------- + /* + * Verify the backup chain. INCREMENTAL BACKUP REFERENCE WAL LOCATION of + * the incremental backup must match with the START WAL LOCATION of the + * previous backup, until we reach a full backup in which there is no + * INCREMENTAL BACKUP REFERENCE WAL LOCATION. + */ The current logic assumes the incremental backup directories are to be provided as input in the serial order the backups were taken. This is bit confusing unless clarified in pg_combinebackup help menu or documentation. I think we should clarify it at both the places. --------- I think scan_directory() should be rather renamed as do_combinebackup(). Regards, Jeevan Ladhe On Thu, Aug 29, 2019 at 8:11 PM Jeevan Ladhe <jeevan.la...@enterprisedb.com> wrote: > Due to the inherent nature of pg_basebackup, the incremental backup also > allows taking backup in tar and compressed format. But, pg_combinebackup > does not understand how to restore this. I think we should either make > pg_combinebackup support restoration of tar incremental backup or restrict > taking the incremental backup in tar format until pg_combinebackup > supports the restoration by making option '--lsn' and '-Ft' exclusive. > > It is arguable that one can take the incremental backup in tar format, > extract > that manually and then give the resultant directory as input to the > pg_combinebackup, but I think that kills the purpose of having > pg_combinebackup utility. > > Thoughts? > > Regards, > Jeevan Ladhe >