Horton

Re: Back-patch of: avoid multiple hard links to same WAL file after a crash

Noah Misch <noah@leadboat.com>

View thread View on postgr.es
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Andres Freund <andres@anarazel.de>, Robert Pang <robertpang@google.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, robertmhaas@gmail.com, pgsql-hackers@postgresql.org
Date: 2025-04-13T15:33:12Z
Lists: pgsql-hackers 
On Sat, Apr 05, 2025 at 07:09:58PM -0700, Noah Misch wrote:
> On Sun, Apr 06, 2025 at 07:42:02AM +0900, Michael Paquier wrote:
> > On Sat, Apr 05, 2025 at 12:13:39PM -0700, Noah Misch wrote:
> > > Since the 2025-02 releases made non-toy-size archive recoveries fail easily,
> > > that's not enough.  If the proposed 3-second test is the wrong thing, what
> > > instead?
> > 
> > I don't have a good idea about that in ~16, TBH, but I am sure to not
> > be a fan of the low reproducibility rate of this test as proposed.
> > It's not perfect, but as the design to fix the original race condition
> > has been introduced in v15, why not begin with a test in 17~ using
> > some injection points?
> 
> Two reasons:
> 
> a) The fix ended calls to the whole range of relevant code.  Hence, the
>    injection point placement that would have been relevant before the fix
>    isn't reached.  In other words, there's no right place for the injection
>    point.  (The place for the injection point would be in durable_rename(), in
>    the checkpointer.  After the fix, the checkpointer just doesn't call
>    durable_rename().)
> 
> b) Stochastic tests catch defects beyond the specific one the test author
>    targeted.  An injection point test is less likely to do that.  (That said,
>    with reason (a), there's no known injection point test design to compete
>    with the stochastic design.)

Tom and Michael, do you still object to the test addition, or not?  If there
are no new or renewed objections by 2025-04-20, I'll proceed to add the test.

As another data point, raising the runtime from 3s to 17s makes it reproduce
the problem 25% of the time.  You can imagine a plot with axes of runtime and
percent detection.  One can pick any point on that plot's curve.  Given how
little wall time it takes for the buildfarm and CI to reach a few hundred
runs, I like the trade-off of 3s runtime and 1% detection.  In particular, I
like it better than 17s runtime for 25% detection.  How do you see it?

Commits

  1. Test restartpoints in archive recovery.

  2. Reset InstallXLogFileSegmentActive after walreceiver self-initiated exit.

  3. Skip WAL recycling and preallocation during archive recovery.

  4. Don't ERROR on PreallocXlogFiles() race condition.

  5. Revert "Add HINT for restartpoint race with KeepFileRestoredFromArchive()."

  6. Remove XLogFileInit() ability to unlink a pre-existing file.

  7. In XLogFileInit(), fix *use_existent postcondition to suit callers.

  8. Remove XLogFileInit() ability to skip ControlFileLock.

  9. Replace durable_rename_excl() by durable_rename(), take two

  10. Add HINT for restartpoint race with KeepFileRestoredFromArchive().

  11. Remove durable_rename_excl()

  12. Replace existing durable_rename_excl() calls with durable_rename()