Re: backup manifests

On Thu, Dec 5, 2019 at 12:17 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Dec 4, 2019 at 1:01 PM Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
> wrote:
> > As per the discussion on the thread, here is the patch which
> >
> > a) Make checksum for manifest file optional.
> > b) Allow user to choose a particular algorithm.
> >
> > Currently with the WIP patch SHA256 and CRC checksum algorithm
> > supported. Patch also changed the manifest file format to append
> > the used algorithm name before the checksum, this way it will be
> > easy to validator to know which algorithm to used.
> >
> > Ex:
> > ./db/bin/pg_basebackup -D bksha/ --manifest-with-checksums=SHA256
> >
> > $ cat bksha/backup_manifest | more
> > PostgreSQL-Backup-Manifest-Version 1
> > File backup_label 226 2019-12-04 17:46:46 GMT
> SHA256:7cf53d1b9facca908678ab70d93a9e7460cd35cedf7891de948dcf858f8a281a
> > File pg_xact/0000 8192 2019-12-04 17:46:46 GMT
> SHA256:8d2b6cb1dc1a6e8cee763b52d75e73571fddce06eb573861d44082c7d8c03c26
> >
> > ./db/bin/pg_basebackup -D bkcrc/ --manifest-with-checksums=CRC
> > PostgreSQL-Backup-Manifest-Version 1
> > File backup_label 226 2019-12-04 17:58:40 GMT CRC:343138313931333134
> > File pg_xact/0000 8192 2019-12-04 17:46:46 GMT CRC:363538343433333133
> >
> > Pending TODOs:
> > - Documentation update
> > - Code cleanup
> > - Testing.
> >
> > I will further continue to work on the patch and meanwhile feel free to
> provide
> > thoughts/inputs.
>
> + initilize_manifest_checksum(&cCtx);
>
> Spelling.
>
>
Fixed.

-
>
> Spurious.
>
> + case MC_CRC:
> + INIT_CRC32C(cCtx->crc_ctx);
>
> Suggest that we do CRC -> CRC32C throughout the patch. Someone might
> conceivably want some other CRC variant, mostly likely 64-bit, in the
> future.
>
>
Make sense, done.

+final_manifest_checksum(ChecksumCtx *cCtx, char *checksumbuf)
>
> finalize
>
>
Done.

printf(_(" --manifest-with-checksums\n"
> - " do calculate checksums for manifest files\n"));
> + " calculate checksums for manifest files
> using provided algorithm\n"));
>
> Switch name is wrong. Suggest --manifest-checksums.
> Help usually shows that an argument is expected, e.g.
> --manifest-checksums=ALGORITHM or
> --manifest-checksums=sha256|crc32c|none
>
>
Fixed.

This seems to apply over some earlier version of the patch. A
> consolidated patch, or the whole stack, would be better.
>

Here is the whole stack of patches.

Thanks,
Rushabh Lathia
www.EnterpriseDB.com