Hi Fujii,
> I think it's better to backpatch this to v19, since commit 67c20979ce7
> introduced the unnecessary barrier wait. Thoughts?
+1, and thanks for the comment fixes. Only the comments changed from
0003, and they read right to me. I ran v3 on REL_19_STABLE: it applies
cleanly there and on master, and make check and the recovery suite pass.
Unpatched 19 hung 12/12 on promotion with one backend stuck writing to a
client; with v3 it promoted 12/12 in 10-27 ms. Unpatched 18 promoted
12/12 in the same time, so 19 is the only branch that needs it.
v3 only covers promotion without logical slots. Shihao's 0001, which
came with his report, is the general fix for when the wait is real.
Since 0001 and his test will need backpatching too, I ran them on 15
through master. Shihao, the full numbers are in the attachment; in
short:
- 060 is now taken on every branch (060_multixact_crashed_updater.pl
went in today), so the test needs another number. 061 is free
everywhere. Renumbered and otherwise unchanged, it passes with 0001
on 17 through master, recovery suite included.
- On 15 and 16 it dies before testing anything: psql there runs
FETCH_COUNT through a cursor, so the backend never blocks. psql()
there also returns 0 on timeout, so the DROP DATABASE check can pass
while DROP DATABASE is still stuck.
- Once v3 is in, promotion with no slot doesn't wait at all, so the
promotion subtest passes without 0001.
The attached diff is a suggestion for all three: a COPY TO STDOUT writer,
a timed_out check, and a logical slot on the standby (19 and master
only). With it the test fails without 0001 and passes with it on 15
through master. The COPY writer also helps on its own: on unpatched
master the promotion subtest caught the hang 5/5 with it, against 1/5
as it stands. It's your test, so take whatever is useful.
Hayato, I could reproduce what you found: with your change, DROP DATABASE
stays blocked behind a backend in COPY FROM STDIN, 6/6, with or without
v3. So it's in 0001's absorb path, and v3 neither causes nor fixes it.
It's latent today: every barrier callback, in 19 and master, returns
true or throws ERROR, and none of about 1800 calls over the recovery
suite returned false. If 0001 wants to guard against it, setting the
latch again when ProcSignalBarrierPending is still set (second attached
diff) cleared your reproducer 3/3. A plain Assert there could fire
spuriously, since a new barrier can arrive right after the call.
Regards,
Manu
Measurements for v3 ("Avoid unnecessary end-of-recovery barrier waits"),
Hayato's point,
and Shihao's test on the back branches
==========================================================================================
2026-10-15. Builds: --enable-cassert --enable-debug --enable-tap-tests (+
injection points
where available), CFLAGS=-O1. Clusters on tmpfs, fsync=off: timings compare
variants only.
Branch tips: master 8f2b598b4bc, REL_19_STABLE 0fb192f1b02 (sections 1-2;
differs from the
tip 86ce68d9e6f only in nbtsearch.c and analyzejoins.c) and 86ce68d9e6f
(section 3),
REL_18_STABLE 10b2d9986b9, REL_17_STABLE 96823bfd7c9, REL_16_STABLE 7792528fd69,
REL_15_STABLE 72053f183e0, REL_14_STABLE 46ef0bdd0b1.
"0001" = Shihao's v2-0001, "0002" = Shihao's v2-0002 test.
1. v3 and the backpatch to v19
------------------------------
- v3 vs 0003: identical code once comments are stripped; only the comments
changed. The new
comments match the code, and the DisableLogicalDecoding() reference holds: it
emits the same
barrier without waiting, for the same reason.
- git apply --check (no fuzz): clean on REL_19_STABLE (0fb192f1b02 and
86ce68d9e6f) and master.
- REL_19_STABLE + v3: make check, all 239 tests pass; src/test/recovery 58
files, 793 tests,
PASS (051_effective_wal_level included).
Promotion with one standby backend blocked in ClientWrite (client stopped
reading), fresh
primary + standby per run, pg_promote(false). HUNG = not promoted after 10 s;
each hung run
was then released by pg_terminate_backend() of that backend in 12-16 ms.
branch variant wal_level / case result promote ms
min/med/max
19 unpatched replica, no slots HUNG 12/12 -
19 v3 replica, no slots PROMOTED 12/12 10/25/27
18 unpatched replica, no slots PROMOTED 12/12 10/25/27
19 unpatched logical HUNG 4/4 -
19 v3 logical PROMOTED 4/4 11/18/26
19 v3 backend stuck sending PROMOTED 4/4 11/18/26
an ERROR message
19 unpatched idle backend PROMOTED 6/6 11/20/28
19 v3 idle backend PROMOTED 6/6 11/12/27
19 v3 standby logical slot HUNG 3/3 - (wait is
needed there)
19 0001+v3 standby logical slot PROMOTED 3/3 10/11/11
The 10 vs 25 ms split comes from the 10 ms polling plus psql round trips; it
shows on 18 too.
2. Hayato's point (a barrier callback returning false)
------------------------------------------------------
His change, verbatim: ProcessBarrierSmgrRelease() returns false once under
ExitOnAnyError,
i.e. on its first call from 0001's ProcessClientBarrierInterrupt().
(a) Real with that change. 0002, 30 s timeout:
0001 PASS 2/2
0001 + his change FAIL 2/2 (DROP DATABASE subtest; "still waiting
for backend
with PID ... to accept
ProcSignalBarrier", the waited
PID being the COPY FROM STDIN backend)
0001 + his change + v3 FAIL 2/2 (same subtest)
DROP DATABASE against a backend in COPY FROM STDIN, 10 s limit: 0001 DONE
4/4 (15 ms);
+ his change BLOCKED 6/6; + his change + v3 BLOCKED 6/6; unpatched BLOCKED
2/2.
(b) Independent of v3, which neither causes nor fixes it. After a false return
ProcSignalBarrierPending stays set (ResetProcSignalBarrierBits()) but the
latch does not,
so secure_read() goes back to WaitEventSetWait() on the socket.
The same trick in ProcessBarrierUpdateXLogLogicalInfo() (the barrier
promotion emits),
backend blocked in ClientWrite, wal_level=replica:
0001 PROMOTED 4/4
0001 + trick HUNG 6/6
0001 + trick + v3 PROMOTED 6/6 (no wait when turning
it off)
0001 + trick + v3, standby slot HUNG 4/4
(c) Latent: no callback returns false today.
Code: REL_19_STABLE has ProcessBarrierSmgrRelease() and
ProcessBarrierUpdateXLogLogicalInfo(),
whose only exit is "return true". master adds AbsorbDataChecksumsBarrier():
every return is
"return true", the other exits are ERRORs (FATAL under 0001's
ExitOnAnyError).
Run: 0001 + v3 + a write_stderr() after every callback call, whole
src/test/recovery
suite plus 0002: 535 SMGRRELEASE and 1257 UPDATE_XLOG_LOGICAL_INFO calls,
none returned
false. (A first probe used elog(LOG); errfinish() runs
CHECK_FOR_INTERRUPTS(), re-entered
the absorb loop and broke 051. That was the probe: unpatched, v3, 0001 and
0001+v3 pass
051 3/3 each, and so does the elog-free probe.)
(d) Two possible changes to ProcessClientBarrierInterrupt(), after
ProcessProcSignalBarrier():
- a comment recording the invariant (no behaviour change);
- "if (ProcSignalBarrierPending) SetLatch(MyLatch);". With his change on
top:
0002 PASS 3/3, DROP DATABASE DONE 3/3, and the logical-info trick
PROMOTED 3/3
(without the SetLatch: FAIL / BLOCKED / HUNG). 0001 + SetLatch alone
passes 0002 2/2.
A plain Assert(!ProcSignalBarrierPending) there could fire spuriously: a
new barrier
can be signalled right after the call (harmlessly, since the handler sets
the latch).
3. 0002 on the back branches
----------------------------
t/060_ is taken on every branch from 14 to master:
060_multixact_crashed_updater.pl was
backpatched today. 061 is free on all of them and follows the highest number
(060).
The meson.build hunk then goes after the 060 line (16 and later; 14 and 15 have
no
meson.build there).
0002 renumbered to 061 and otherwise unchanged. "alone" = only this test, 30 s
timeout:
branch applies 0001 applies unpatched 0001 (alone) recovery suite with
0001 + 061
master clean clean FAIL PASS 2/2 PASS (60 files, 816
tests)
19 clean clean FAIL PASS 2/2 PASS (59 files, 795
tests)
18 clean clean FAIL PASS 2/2 PASS (54 files, 647
tests)
17 clean clean FAIL PASS 2/2 PASS (52 files, 625
tests)
16 clean clean dies dies 2/2 only 061 fails
(exit 29)
15 clean clean dies dies only 061 fails
(exit 29)
14 clean clean dies dies only 061 fails
(exit 29)
"dies" = the test stops before checking anything (below), so on 14-16 a failure
says
nothing about the fix.
- 15 and 16: the test dies with "timed out waiting for a backend to block in
ClientWrite".
psql there implements FETCH_COUNT with a cursor (DECLARE _psql_cursor), not
chunked rows,
so the backend goes idle between fetches and never blocks.
- 15 and 16 also: PostgreSQL::Test::Cluster::psql() returns 0 when it times out
(there the
undef $ret goes through ">> 8"), so on unpatched 16 the DROP DATABASE subtest
passed while
DROP DATABASE sat for 30 s on "still waiting for backend ...". 17 and later
return undef.
- 14: no WaitForProcSignalBarrier() caller outside procsignal.c, so nothing to
hang on.
- On 15 and 14 the stand-alone runs after the first one could not start
("tmp_check/...
File exists"; those branches don't use TESTDATADIR). The suites above are
clean runs.
Possible changes (Shihao's call), measured:
(i) stuck writer = psql running COPY (SELECT ...) TO STDOUT instead of the
FETCH_COUNT
query; psql writes COPY data straight to its stdout, which nobody reads;
(ii) the DROP DATABASE check is ok(!$timed_out && $ret == 0, ...);
(iii) 19 and master: a logical slot on the standby before promoting. Once v3
is in,
promotion with no slot doesn't wait at all, so the promotion subtest no
longer
exercises 0001: 19 + v3 without 0001 passes it 2/2, with zero "still
waiting".
branch version unpatched 0001 recovery suite
with 0001 + 061
16 (i)+(ii) DROP DATABASE FAIL 2/2 PASS 3/3 PASS (45 files,
538 tests)
15 (i)+(ii) DROP DATABASE FAIL 2/2 PASS 3/3 PASS (43 files,
446 tests)
17 (i)+(ii) DROP DATABASE FAIL 1/1 PASS 2/2 PASS (52 files,
625 tests)
18 (i)+(ii) DROP DATABASE FAIL 1/1 PASS 1/1 PASS (54 files,
647 tests)
19 all three see below PASS (59 files,
796 tests)
master all three see below PASS (60 files,
817 tests)
Promotion subtest on unpatched master, 5 runs each: catches the hang 1/5 as
posted, 5/5
with (i). DROP DATABASE subtest: 5/5 both ways.
19 with all three: v3 without 0001: promotion FAIL 3/3 (the wait is back),
DROP DATABASE
FAIL 3/3; 0001 + v3: PASS 3/3; unpatched FAIL 1/1; 0001 alone PASS 1/1.
master with all three: unpatched promotion and DROP DATABASE FAIL 1/1; 0001
PASS 1/1.
--- a/src/test/recovery/t/061_barrier_blocked_client.pl
+++ b/src/test/recovery/t/061_barrier_blocked_client.pl
@@ -23,17 +23,25 @@
$standby->init_from_backup($primary, 'backup', has_streaming => 1);
$standby->start;
-# A client that stops reading. psql fetches the result in chunks and blocks
-# writing to a pipe that we never read, so the backend fills its socket
-# buffer and blocks in ClientWrite.
+# With a logical slot on the standby, promotion keeps logical decoding enabled
+# and waits for every process to absorb a barrier. Without one it doesn't
+# wait at all.
+$primary->safe_psql('postgres',
+ "SELECT pg_create_logical_replication_slot('primary_slot', 'test_decoding')");
+$primary->wait_for_replay_catchup($standby);
+$standby->create_logical_slot_on_standby($primary, 'standby_slot', 'postgres');
+
+# A client that stops reading. psql writes the COPY data to a pipe that we
+# never read and blocks, so the backend fills its socket buffer and blocks in
+# ClientWrite. (Not a FETCH_COUNT query: before v17 psql runs that through a
+# cursor, and the backend goes idle between fetches.)
my ($in, $out, $err) = ('', '', '');
my $writer = IPC::Run::start(
[
'psql', '--no-psqlrc',
- '--set' => 'FETCH_COUNT=100',
'--dbname' => $standby->connstr('postgres'),
'--command' =>
- "SELECT repeat('x', 1000) FROM generate_series(1, 1000000)"
+ "COPY (SELECT repeat('x', 1000) FROM generate_series(1, 1000000)) TO STDOUT"
],
'<' => \$in,
'>' => \$out,
@@ -43,7 +51,6 @@
"SELECT count(*) = 1 FROM pg_stat_activity WHERE wait_event = 'ClientWrite'"
) or die "timed out waiting for a backend to block in ClientWrite";
-# Promotion waits for every process to absorb a barrier.
my $promoted = $standby->safe_psql('postgres',
"SELECT pg_promote(true, $PostgreSQL::Test::Utils::timeout_default)");
is($promoted, 't', 'promotion completes with a backend blocked on a write');
@@ -73,7 +80,9 @@
'postgres', 'DROP DATABASE dropme',
timeout => $PostgreSQL::Test::Utils::timeout_default,
timed_out => \$timed_out);
-is($ret, 0, 'DROP DATABASE completes with a backend blocked on a read');
+# Check timed_out too: before v17, psql() returns 0 when it times out.
+ok(!$timed_out && $ret == 0,
+ 'DROP DATABASE completes with a backend blocked on a read');
$reader->kill_kill;
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -525,6 +525,13 @@
ProcessProcSignalBarrier();
+ /*
+ * A barrier callback that returned false wants to be retried, but our
+ * latch is not set, so make sure we don't go back to sleep on the socket.
+ */
+ if (ProcSignalBarrierPending)
+ SetLatch(MyLatch);
+
ExitOnAnyError = save_ExitOnAnyError;
whereToSendOutput = save_whereToSendOutput;
}
