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

From: Noah Misch <[email-masked]>
Date: Sun, 13 Apr 2025 08:33:12 -0700

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?

---

From: Tom Lane <[email-masked]>
Date: Sun, 13 Apr 2025 11:51:57 -0400

> are no new or renewed objections by 2025-04-20, I'll proceed to add the test.

While I still don't love it, I don't have a better proposal.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Mon, 14 Apr 2025 09:19:35 +0900

> While I still don't love it, I don't have a better proposal.

No objections from here to use your proposal for the time being on all
the branches.  I am planning to spend some cycles thinking about a
better alternative, but I doubt that this will be portable down to
v13.
--
Michael

---

From: Noah Misch <[email-masked]>
Date: Sun, 20 Apr 2025 14:53:39 -0700

> > > are no new or renewed objections by 2025-04-20, I'll proceed to add the test.

Pushed as commit 714bd9e.  The failure so far is
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-20%2015%3A36%3A35
with these highlights:

pg_ctl: server does not shut down

2025-04-20 17:27:35.735 UTC [1576688][postmaster][:0] LOG:  received immediate shutdown request
2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] FATAL:  archive command was terminated by signal 3: Quit
2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] DETAIL:  The failed archive command was: cp "pg_wal/00000001000000000000006D" "/home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/recovery/045_archive_restartpoint/data/t_045_archive_restartpoint_primary_data/archives/00000001000000000000006D"

The checkpoints and WAL creation took 30s, but archiving was only 20% done
(based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT.  I can
reproduce this if I test with valgrind --trace-children=yes.  With my normal
valgrind settings, the whole test file takes only 18s.  I recommend one of
these changes to skink:

- Add --trace-children-skip='/bin/*,/usr/bin/*' so valgrind doesn't instrument
  "sh" and "cp" commands.
- Remove --trace-children=yes

Andres, what do you think about making one of those skink configuration
changes?  Alternatively, I could make the test poll until archiving catches
up.  However, that would take skink about 30min, and I expect little value
from 30min of valgrind instrumenting the "cp" command.

---

From: Noah Misch <[email-masked]>
Date: Sun, 20 Apr 2025 15:15:59 -0700

> - Remove --trace-children=yes

I gave that more thought.  One can be more surgical than that, via
--trace-children-skip-by-arg='*cp "*' or similar.  My previous message's two
options stop valgrind instrumentation at boundaries like pg_dumpall calling
system(pg_dump ...), since that execs /bin/sh to run pg_dump.  If we wanted to
make it even more explicit and surgical, skink could use
--trace-children-skip-by-arg='*valgrind-ignore-child*' combined with:

--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1404 +1404 @@ sub enable_restoring
-	  : qq{cp "$path/%f" "%p"};
+	  : qq{cp "$path/%f" "%p"  # valgrind-ignore-child};
@@ -1474 +1474 @@ sub enable_archiving
-	  : qq{cp "%p" "$path/%f"};
+	  : qq{cp "%p" "$path/%f"  # valgrind-ignore-child};

What's your preference?

---

From: Andres Freund <[email-masked]>
Date: Fri, 25 Apr 2025 15:35:06 -0400

Hi,

On 2025-04-20 14:53:39 -0700, Noah Misch wrote:
> (based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT.

Huh.  That seems surprisingly slow, even for valgrind.  I guess it's one more
example for why the single-threaded archiving approach sucks so badly :)

> I can reproduce this if I test with valgrind --trace-children=yes.  With my
> normal valgrind settings, the whole test file takes only 18s.  I recommend
> one of these changes to skink:
> 

Hm. I think I used --trace-children=yes because I was thinking it was required
to track forks. But a newer version of valgrind's man page has an important
clarification:
       --trace-children=<yes|no> [default: no]
           When enabled, Valgrind will trace into sub-processes initiated via the exec system call. This is necessary for multi-process programs.
 
           Note that Valgrind does trace into the child of a fork (it would be difficult not to, since fork makes an identical copy of a process), so this
           option is arguably badly named. However, most children of fork calls immediately call exec anyway.

So there doesn't seem to be much point in using --trace-children=yes.

> from 30min of valgrind instrumenting the "cp" command.

I just changed the config to --trace-children=no. There already is a valgrind
run in progress, so it won't be in effect for the next run.

Greetings,

Andres Freund

---

From: Noah Misch <[email-masked]>
Date: Sat, 26 Apr 2025 16:03:15 -0700

> example for why the single-threaded archiving approach sucks so badly :)

Yes!  I also didn't expect that v14/v13 would run much faster.

> run in progress, so it won't be in effect for the next run.

Works for me.  I see that resolved things.