Date: Tue, 3 Dec 2024 16:26:36 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: checksum verification code breaks backups in v16-
Message-ID: <Z0-FnG0zQ9xlciQP@nathan>
References: <CA+TgmoaWmO=V55Cy1brfww9ewjRrZE80ej8ox1iu2p2JhKKu_A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+TgmoaWmO=V55Cy1brfww9ewjRrZE80ej8ox1iu2p2JhKKu_A@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z0-FnG0zQ9xlciQP%40nathan>
Precedence: bulk

On Tue, Dec 03, 2024 at 11:39:43AM -0500, Robert Haas wrote:
> If we want a narrower and thus less-risky fix, we could consider just
> adjusting this code here:
> 
>                 segmentno = atoi(segmentpath + 1);
>                 if (segmentno == 0)
>                     ereport(ERROR,
>                             (errmsg("invalid segment number %d in file \"%s\"",
>                                     segmentno, filename)));

I'm only seeing this code in pg_checksums.  Is that what you are proposing
to change?

> I think we could just replace the ereport() with verify_checksum =
> false (and update the comments). That would leave open the possibility
> of trying to checksum some random file with a name like
> this_file_should_not_be_here, which we'll interpret as segment 0
> because the name does not contain a dot; but if the file were called
> this.file.should.not.be.here, we'd not try to checksum it at all
> because "here" is not an integer. That's a little random, but it
> avoids taking a position on whether
> 025584a168a4b3002e19350bb8db0ebf1fd10235 is fully correct. It simply
> takes the narrower position that we shouldn't fail the entire backup
> because we're unable to perform a checksum calculation that in the
> worst case would only produce a warning anyway.

Hm.

> At the moment, I think I slightly prefer the narrower fix, but it
> might be a little too narrow, since the resultant behavior as
> described in the preceding paragraph is somewhat wonky and
> nonsensical.

Yeah, I'm a little worried that it is too narrow.  I was hoping that the
refactoring commit was older, because then I think we'd be much more
willing to back-patch it.  But there are a couple of v17 releases out there
already, so maybe it's not too much of a stretch.  I guess I'm leaning
towards the back-patching idea...

-- 
nathan