From ede9dfa4a47dfed3ad3e20d941f64d91125f6fb3 Mon Sep 17 00:00:00 2001 From: Manu Date: Fri, 23 Oct 2026 15:24:21 -0300 Subject: [PATCH v5 1/2] Absorb ProcSignalBarriers while waiting for sync rep or client input WaitForProcSignalBarrier() waits until every process has absorbed the barrier, but two kinds of wait never absorb one. A backend waiting in SyncRepWaitForLSN() holds off interrupts, since its commit record has already been flushed, so CHECK_FOR_INTERRUPTS() does nothing there. A backend waiting for more data from its client in the middle of a command, as in COPY FROM STDIN, only services ProcDiePending in ProcessClientReadInterrupt(). Whatever waits for the barrier, such as DROP DATABASE, then waits for as long as they do. With VACUUM FULL of pg_subscription and pg_authid queued behind DROP DATABASE's lock on pg_subscription, even new connections block, since rebuilding the relcache init files needs a lock on every catalog with a syscache. Add ProcessProcSignalBarrierNoError(), which absorbs the pending barriers only if every pending type can be absorbed where an ERROR is not allowed, and call it in both waits. Each barrier type now declares this in BarrierTypeAbsorbableWithoutError(), whose switch has no default case, so that a new type has to be classified. A type qualifies only if its processing does nothing that CommitTransaction() does not already do after the commit record. PROCSIGNAL_BARRIER_SMGRRELEASE closes files; with io_method=io_uring that may wait for in-flight IO and can raise an ERROR on an internal consistency failure, but smgrDoPendingDeletes() runs the same path after the commit record. PROCSIGNAL_BARRIER_UPDATE_XLOG_LOGICAL_INFO only sets a flag or reads shared state. The data checksum types do not qualify, because AbsorbDataChecksumsBarrier() raises an ERROR on an unexpected state, which after the commit record would be a PANIC; if one of them is pending, nothing is absorbed, as before. The sync rep waiter keeps waiting: its commit is acknowledged only once the standby confirms it. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/CAA4eK1LC09Tq3vTr04ppXRbyQnMb66_pgyuES2C_5ZoWfYRUrQ@mail.gmail.com --- src/backend/replication/syncrep.c | 10 ++ src/backend/storage/ipc/procsignal.c | 108 ++++++++++++ src/backend/tcop/postgres.c | 11 ++ src/include/storage/procsignal.h | 6 + src/test/recovery/meson.build | 1 + .../t/061_procsignal_barrier_absorb.pl | 164 ++++++++++++++++++ 6 files changed, 300 insertions(+) create mode 100644 src/test/recovery/t/061_procsignal_barrier_absorb.pl diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index d870f09..572d747 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -82,6 +82,7 @@ #include "replication/walsender.h" #include "replication/walsender_private.h" #include "storage/proc.h" +#include "storage/procsignal.h" #include "tcop/tcopprot.h" #include "utils/guc_hooks.h" #include "utils/ps_status.h" @@ -334,6 +335,15 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit) break; } + /* + * Interrupts are held off, so CHECK_FOR_INTERRUPTS() would not absorb + * a ProcSignalBarrier, and whoever waits for one would wait as long + * as we do. Absorb it here if every pending type allows it where an + * ERROR is not allowed: the transaction has already committed. + */ + if (ProcSignalBarrierPending) + ProcessProcSignalBarrierNoError(); + /* * Wait on latch. Any condition that should wake us up will set the * latch, so no need for timeout. diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index eb0f4d6..95a3269 100644 --- a/src/backend/storage/ipc/procsignal.c +++ b/src/backend/storage/ipc/procsignal.c @@ -124,6 +124,8 @@ static ProcSignalSlot *MyProcSignalSlot = NULL; static bool CheckProcSignal(ProcSignalReason reason); static void CleanupProcSignalState(int status, Datum arg); static void ResetProcSignalBarrierBits(uint32 flags); +static void ProcessProcSignalBarrierInternal(bool noerror); +static bool BarrierTypeAbsorbableWithoutError(ProcSignalBarrierType type); /* * ProcSignalShmemRequest @@ -511,6 +513,90 @@ HandleProcSignalBarrierInterrupt(void) */ void ProcessProcSignalBarrier(void) +{ + ProcessProcSignalBarrierInternal(false); +} + +/* + * ProcessProcSignalBarrierNoError - absorb barriers where an ERROR is not + * allowed + * + * Some waits cannot run CHECK_FOR_INTERRUPTS(): SyncRepWaitForLSN() holds off + * interrupts after the commit record has been flushed, and a backend waiting + * for more data from its client in the middle of a command, as in COPY FROM + * STDIN, cannot process a query cancel there. If such a process did not + * absorb barriers while it waits, every process waiting for a barrier, such as + * DROP DATABASE, would wait as long as it does. + * + * This absorbs the pending barriers only if every pending type can be absorbed + * where an ERROR is not allowed (see BarrierTypeAbsorbableWithoutError()). + * Otherwise it absorbs nothing and leaves all of them pending for the next + * CHECK_FOR_INTERRUPTS(), exactly as if it had not been called: the barrier + * generation can only advance once every pending type has been handled. + * + * It may be called with interrupts held off, but not in a critical section, + * and not while holding an LWLock. + */ +void +ProcessProcSignalBarrierNoError(void) +{ + Assert(CritSectionCount == 0); + + ProcessProcSignalBarrierInternal(true); +} + +/* + * Can this barrier type be absorbed where an ERROR is not allowed? + * + * ProcessProcSignalBarrierNoError() absorbs the pending barriers only if this + * returns true for every pending type. Return true only for a type whose + * processing does nothing that CommitTransaction() does not already do after + * its commit record: it must not raise an ERROR, allocate memory or take a lock + * that the caller could be holding, except on a path that commit processing + * itself runs in the same state. There is deliberately no default case, so + * that the compiler warns if a new type is not classified here. + */ +static bool +BarrierTypeAbsorbableWithoutError(ProcSignalBarrierType type) +{ + switch (type) + { + case PROCSIGNAL_BARRIER_SMGRRELEASE: + + /* + * smgrreleaseall() closes files without allocating memory (see + * _fdvec_resize()). With io_method=io_uring, FileClose() may + * wait for this backend's in-flight IO in pgaio_closing_fd(), + * which can raise an ERROR only on an internal consistency + * failure. CommitTransaction() runs the same path after its + * commit record, in smgrDoPendingDeletes(). + */ + return true; + case PROCSIGNAL_BARRIER_UPDATE_XLOG_LOGICAL_INFO: + + /* + * With a transaction ID assigned this only sets a flag, consumed + * by AtEOXact_LogicalCtl(); otherwise it reads shared state under + * a shared LWLock that no barrier emitter holds while it waits. + */ + return true; + case PROCSIGNAL_BARRIER_CHECKSUM_OFF: + case PROCSIGNAL_BARRIER_CHECKSUM_INPROGRESS_ON: + case PROCSIGNAL_BARRIER_CHECKSUM_INPROGRESS_OFF: + case PROCSIGNAL_BARRIER_CHECKSUM_ON: + /* AbsorbDataChecksumsBarrier() raises an ERROR on a bad state. */ + return false; + } + + return false; /* not reached for a valid type */ +} + +/* + * Workhorse for ProcessProcSignalBarrier() and + * ProcessProcSignalBarrierNoError(). + */ +static void +ProcessProcSignalBarrierInternal(bool noerror) { uint64 local_gen; uint64 shared_gen; @@ -556,6 +642,28 @@ ProcessProcSignalBarrier(void) */ flags = pg_atomic_exchange_u32(&MyProcSignalSlot->pss_barrierCheckMask, 0); + /* + * Where an ERROR is not allowed, absorb nothing unless every pending type + * can be absorbed there: put all the bits back for the next call. + */ + if (noerror) + { + uint32 check = flags; + + while (check != 0) + { + ProcSignalBarrierType type; + + type = (ProcSignalBarrierType) pg_rightmost_one_pos32(check); + if (!BarrierTypeAbsorbableWithoutError(type)) + { + ResetProcSignalBarrierBits(flags); + return; + } + BARRIER_CLEAR_BIT(check, type); + } + } + /* * If there are no flags set, then we can skip doing any real work. * Otherwise, establish a PG_TRY block, so that we don't lose track of diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index b6bdfe2..b3ad8e4 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -544,6 +544,17 @@ ProcessClientReadInterrupt(bool blocked) else SetLatch(MyLatch); } + else if (blocked && ProcSignalBarrierPending) + { + /* + * Waiting for more data from the client in the middle of a command, + * as in COPY FROM STDIN. CHECK_FOR_INTERRUPTS() is not safe here, + * but barriers that can be absorbed where an ERROR is not allowed + * are, so that a client that stops sending does not hold up whoever + * waits for the barrier. + */ + ProcessProcSignalBarrierNoError(); + } errno = save_errno; } diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h index aaa158b..7721814 100644 --- a/src/include/storage/procsignal.h +++ b/src/include/storage/procsignal.h @@ -45,6 +45,11 @@ typedef enum #define NUM_PROCSIGNALS (PROCSIG_RECOVERY_CONFLICT + 1) +/* + * Barrier types. A new type must also be classified in + * BarrierTypeAbsorbableWithoutError() (procsignal.c), which decides whether it + * can be absorbed where an ERROR is not allowed. + */ typedef enum { PROCSIGNAL_BARRIER_SMGRRELEASE, /* ask smgr to close files */ @@ -77,6 +82,7 @@ extern void SendCancelRequest(int backendPID, const uint8 *cancel_key, int cance extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type); extern void WaitForProcSignalBarrier(uint64 generation); extern void ProcessProcSignalBarrier(void); +extern void ProcessProcSignalBarrierNoError(void); extern void procsignal_sigusr1_handler(SIGNAL_ARGS); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index b6ac9dd..5732992 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -69,6 +69,7 @@ tests += { 't/058_shutdown_crash_restart.pl', 't/059_remote_apply_status_interval.pl', 't/060_multixact_crashed_updater.pl', + 't/061_procsignal_barrier_absorb.pl', ], }, } diff --git a/src/test/recovery/t/061_procsignal_barrier_absorb.pl b/src/test/recovery/t/061_procsignal_barrier_absorb.pl new file mode 100644 index 0000000..478a133 --- /dev/null +++ b/src/test/recovery/t/061_procsignal_barrier_absorb.pl @@ -0,0 +1,164 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Sessions that wait where they cannot run CHECK_FOR_INTERRUPTS() still absorb +# the ProcSignalBarriers that can be absorbed where an ERROR is not allowed (see +# BarrierTypeAbsorbableWithoutError()): a session waiting +# for synchronous replication after its commit, and one in COPY FROM STDIN +# waiting for more data from its client. DROP DATABASE must not wait for them, +# and they must go on waiting. A pending barrier whose processing can raise +# an ERROR, such as a data checksums change, is not absorbed there. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init(allows_streaming => 1); +# A synchronous standby that never connects. It is set before the server +# starts, so that it is in effect before the first commit; only the session +# that waits for synchronous replication requests it. +$node->append_conf( + 'postgresql.conf', qq( +synchronous_standby_names = 'nosuch' +synchronous_commit = local +)); +$node->start; + +$node->safe_psql('postgres', 'CREATE TABLE copy_target (a int)'); +$node->safe_psql('postgres', 'CREATE DATABASE target1'); +$node->safe_psql('postgres', 'CREATE DATABASE target2'); + +# Start a psql running $sql in the background, with application_name $appname. +sub start_psql +{ + my ($appname, $sql, $in, $out, $err) = @_; + + return IPC::Run::start( + [ + 'psql', '--no-psqlrc', '--quiet', + '--dbname' => $node->connstr('postgres') + . " application_name=$appname", + defined $sql ? ('--command' => $sql) : () + ], + '<' => $in, + '>' => $out, + '2>' => $err); +} + +# Return the PID of the session $appname once its wait event is $wait_event. +sub session_pid +{ + my ($appname, $wait_event) = @_; + + $node->poll_query_until('postgres', + "SELECT wait_event = '$wait_event' FROM pg_stat_activity WHERE application_name = '$appname'" + ) or die "timed out waiting for session \"$appname\""; + return $node->safe_psql('postgres', + "SELECT pid FROM pg_stat_activity WHERE application_name = '$appname'"); +} + +# DROP DATABASE $dbname must complete while session $pid keeps waiting. +sub check_drop_does_not_wait +{ + my ($pid, $dbname, $what, $wait_event) = @_; + my $offset = -s $node->logfile; + my $timed_out = 0; + + # With a timeout, so that a regression fails here instead of hanging. + $node->psql( + 'postgres', + "DROP DATABASE $dbname", + timeout => $PostgreSQL::Test::Utils::timeout_default, + timed_out => \$timed_out); + ok(!$timed_out, "$what: DROP DATABASE did not wait for the session"); + is( $node->safe_psql( + 'postgres', + "SELECT count(*) FROM pg_database WHERE datname = '$dbname'"), + '0', + "$what: DROP DATABASE completed"); + is( $node->safe_psql( + 'postgres', "SELECT wait_event FROM pg_stat_activity WHERE pid = $pid"), + $wait_event, + "$what: session is still waiting"); + ok( !$node->log_contains( + "still waiting for backend with PID $pid", $offset), + "$what: no wait for the barrier was reported"); +} + +# +# A session waiting for a synchronous standby that is not connected, after its +# transaction has committed locally. +# +my $log_offset = -s $node->logfile; +my ($sr_in, $sr_out, $sr_err) = ('', '', ''); +my $syncrep = start_psql('syncrep_waiter', + 'SET synchronous_commit = on; CREATE TABLE syncrep_target ()', + \$sr_in, \$sr_out, \$sr_err); +my $syncrep_pid = session_pid('syncrep_waiter', 'SyncRep'); +check_drop_does_not_wait($syncrep_pid, 'target1', 'synchronous replication', + 'SyncRep'); + +# Disabling data checksums emits barriers whose processing can raise an ERROR, +# so the waiting session must not absorb them: the operation waits for it. +if ($node->safe_psql('postgres', 'SHOW data_checksums') ne 'on') +{ + $node->safe_psql('postgres', 'SELECT pg_enable_data_checksums()'); + $node->poll_query_until('postgres', 'SHOW data_checksums', 'on') + or die 'timed out waiting for data checksums to be enabled'; +} +my $offset = -s $node->logfile; +$node->safe_psql('postgres', 'SELECT pg_disable_data_checksums()'); +ok( eval { + $node->wait_for_log( + qr/still waiting for backend with PID $syncrep_pid to accept ProcSignalBarrier/, + $offset); + 1; + }, + 'data checksums: the operation waits for the session'); +isnt($node->safe_psql('postgres', 'SHOW data_checksums'), + 'off', 'data checksums: not disabled while the session waits'); +is( $node->safe_psql( + 'postgres', + "SELECT wait_event FROM pg_stat_activity WHERE pid = $syncrep_pid"), + 'SyncRep', + 'data checksums: session is still waiting'); + +# Release the session: its commit is acknowledged, without a cancel, and the +# data checksums change goes through. +$node->safe_psql('postgres', + "ALTER SYSTEM SET synchronous_standby_names = ''"); +$node->reload; +$syncrep->finish; +ok( !$node->log_contains( + 'canceling the wait for synchronous replication', $log_offset), + 'synchronous replication: the wait was not canceled'); +is( $node->safe_psql( + 'postgres', + "SELECT count(*) FROM pg_class WHERE relname = 'syncrep_target'"), + '1', + 'synchronous replication: transaction completed'); +ok($node->poll_query_until('postgres', 'SHOW data_checksums', 'off'), + 'data checksums: disabled once the session moved on'); + +# +# A COPY FROM STDIN whose client sent one row and then paused. +# +my ($copy_in, $copy_out, $copy_err) = ("COPY copy_target FROM STDIN;\n1\n", '', ''); +my $copy = start_psql('copy_paused', undef, \$copy_in, \$copy_out, \$copy_err); +$copy->pump_nb; +$node->poll_query_until('postgres', + "SELECT wait_event = 'ClientRead' AND state = 'active' FROM pg_stat_activity WHERE application_name = 'copy_paused'" +) or die 'timed out waiting for COPY FROM STDIN to start'; +my $copy_pid = session_pid('copy_paused', 'ClientRead'); +check_drop_does_not_wait($copy_pid, 'target2', 'COPY FROM STDIN', + 'ClientRead'); +$copy_in .= "2\n\\.\n"; +$copy->finish; +is($node->safe_psql('postgres', 'SELECT count(*) FROM copy_target'), + '2', 'COPY FROM STDIN: completed'); + +$node->stop; +done_testing(); -- 2.56.0