Date: Sat, 28 Mar 2020 20:40:10 -0700
From: Noah Misch <noah@leadboat.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: David Steele <david@pgmasters.net>, Stephen Frost <sfrost@snowman.net>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Suraj Kharage <suraj.kharage@enterprisedb.com>,
	tushar <tushar.ahuja@enterprisedb.com>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	Rushabh Lathia <rushabh.lathia@gmail.com>,
	Tels <nospam-pg-abuse@bloodgate.com>,
	Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
	vignesh C <vignesh21@gmail.com>
Subject: Re: backup manifests
Message-ID: <20200329034010.GB2247658@rfd.leadboat.com>
References: 
 <CAA4eK1+WyCVLZ03ZPH+POAY_NE80p+QxXDUnF10iryHj3vFjTA@mail.gmail.com>
 <CA+TgmoZBrwYvfQ=ymXk669+hgdQxpoypQVYdWiGwpOE_yuMW4A@mail.gmail.com>
 <20200323224217.GN13712@tamriel.snowman.net>
 <CA+TgmoYdFx_Hm4TUmc2qxLvaUo1KXH2yuxBk1RrgT5Oj+PwvNw@mail.gmail.com>
 <20200325133106.GS13712@tamriel.snowman.net>
 <CA+TgmoYOKC_8o-AR1jTQs0mOrFx=_Rcy5udor1m-LjyJNiSWPQ@mail.gmail.com>
 <20200325205433.GV13712@tamriel.snowman.net>
 <CA+TgmoZcEXq0SUveFRbqrso1EsDKCn8YXPSyWJBhC-c3fDbO=Q@mail.gmail.com>
 <16538d02-fd4b-6c4f-81a5-132c8fe8c3e9@pgmasters.net>
 <CA+TgmoZfj3wqcgoaXp+4a7ZSk3coARcxvghT7m=7Hf=8r6st6w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoZfj3wqcgoaXp+4a7ZSk3coARcxvghT7m=7Hf=8r6st6w@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Fri, Mar 27, 2020 at 01:53:54PM -0400, Robert Haas wrote:
> - Replace a doc paragraph about the advantages and disadvantages of
> CRC-32C with one by Stephen Frost, with a slightly change by me that I
> thought made it sound more grammatical.

Defaulting to CRC-32C seems prudent to me:

- As Andres Freund said, SHA-512 is slow relative to storage now available.
  Since gzip is a needlessly-slow choice for backups (or any application that
  copies the compressed data just a few times), comparison to "gzip -6" speed
  is immaterial.

- While I'm sure some other fast hash would be a superior default, introducing
  a new algorithm is a bikeshed, as you said.  This design makes it easy,
  technically, for someone to introduce a new algorithm later.  CRC-32C is not
  catastrophically unfit for 1GiB files.

- Defaulting to SHA-512 would, in the absence of a WAL archive that also uses
  a cryptographic hash function, give a false sense of having achieved some
  coherent cryptographic goal.  With the CRC-32C default, WAL and the rest get
  similar protection.  I'm discounting the case of using BASE_BACKUP without a
  WAL archive, because I expect little intersection between sites "worried
  enough to hash everything" and those "not worried enough to use an archive".
  (On the other hand, the program that manages the WAL archive can reasonably
  own hashing base backups; putting ownership in the server isn't achieving
  much extra.)

> + <refnamediv>
> +  <refname>pg_validatebackup</refname>
> +  <refpurpose>verify the integrity of a base backup of a
> +  <productname>PostgreSQL</productname> cluster</refpurpose>
> + </refnamediv>

> +    <listitem>
> +      <para>
> +        <literal>pg_wal</literal> is ignored because WAL files are sent
> +        separately from the backup, and are therefore not described by the
> +        backup manifest.
> +      </para>
> +    </listitem>

Stephen Frost mentioned that a backup could pass validation even if
pg_basebackup were killed after writing the base backup and before finishing
the writing of pg_wal.  One might avoid that by simply writing the manifest to
a temporary name and renaming it to the final name after populating pg_wal.

What do you think of having the verification process also call pg_waldump to
validate the WAL CRCs (shown upthread)?  That looked helpful and simple.

I think this functionality doesn't belong in its own program.  If you suspect
pg_basebackup or pg_restore will eventually gain the ability to merge
incremental backups into a recovery-ready base backup, I would put the
functionality in that program.  Otherwise, I would put it in pg_checksums.
For me, part of the friction here is that the program description indicates
general verification, but the actual functionality merely checks hashes on a
directory tree that happens to represent a PostgreSQL base backup.

> +		parse->pathname = palloc(raw_length + 1);

I don't see this freed anywhere; is it?  (It's useful to make peak memory
consumption not grow in proportion to the number of files backed up.)

[This message is not a full code review.]