Re: block-level incremental backup

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(dot)ladhe(at)enterprisedb(dot)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
>