On Fri, Aug 30, 2019 at 6:52 PM Jeevan Ladhe <jeevan.la...@enterprisedb.com>
wrote:
> Here are some comments:
> 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)));
>
>
Opted for the above suggestion.
>
> 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.
>
Added this in the documentation. In help, it will be too crowdy.
> 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.
>
After adding support for a tablespace, these two functions take different
values depending upon the context.
> 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.
>
Added in doc.
>
> I think scan_directory() should be rather renamed as do_combinebackup().
>
I am not sure about this renaming. scan_directory() is called recursively
to scan each sub-directories too. If we rename it then it is not actually
recursively doing a combinebackup. Combine backup is a single whole
process.
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
