agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedRace conditions in logical decoding
38+ messages / 8 participants
[nested] [flat]
* Race conditions in logical decoding
@ 2026-01-19 16:29 Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-01-19 16:29 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
A stress test [1] for the REPACK patch [1] revealed data
corruption. Eventually I found out that the problem is in postgres core. In
particular, it can happen that a COMMIT record is decoded, but before the
commit could be recorded in CLOG, a snapshot that takes the commit into
account is created and even used. Visibility checks then work incorrectly
until the CLOG gets updated.
In logical replication, the consequences are not only wrong data on the
subscriber, but also corrutped table on publisher - this is due to incorrectly
set commit hint bits.
Attached is a spec file that demonstrates the issue. I did not add it to
Makefile because I don't expect the current version to be merged (see the
commit message for details.
I'm not sure yet how to fix the problem. I tried to call XactLockTableWait()
from SnapBuildAddCommittedTxn() (like it happens in SnapBuildWaitSnapshot()),
but it made at least one regression test (subscription/t/010_truncate.pl)
stuck - probably a deadlock. I can spend more time on it, but maybe someone
can come up with a good idea sooner than me.
[1] https://www.postgresql.org/message-id/CADzfLwU78as45To9a%3D-Qkr5jEg3tMxc5rUtdKy2MTv4r_SDGng%40mail.g...
[2] https://commitfest.postgresql.org/patch/5117/
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
[text/x-diff] 0001-Demonstrate-possible-race-conditions-in-logical-decoding.patch (11.6K, ../../85833.1768840165@localhost/2-0001-Demonstrate-possible-race-conditions-in-logical-decoding.patch)
download | inline diff:
From ca765ee0a49ac1005c5eb7ebd1d29bea97a63552 Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Mon, 19 Jan 2026 12:54:55 +0100
Subject: [PATCH 1/2] Demonstrate possible race conditions in logical decoding.
The problem is that the snapshot builder can create a snapshot before CLOG has
been updated. That breaks visibility check that use such snapshot. For more
details, see startup_race.spec.
Success of the test means that the problem is present. Thus it would need to
be modified if it should be merged into the tree.
Another problem that currently prevents this test from being merged is that it
hard-wires the logical decoding setup into the SET TRANSACTION command. I
tried to modify the isolation tester so it can use the logical replication
protocol (in which case the test could use the "CREATE_REPLICATION_SNAPSHOT
... (SNAPSHOT 'use')" command), but the tester does things that are not
compatible with that protocol (e.g. it sets the application_name parameter).
---
.../test_decoding/expected/startup_race.out | 85 ++++++++++++
contrib/test_decoding/specs/startup_race.spec | 126 ++++++++++++++++++
src/backend/access/transam/xact.c | 6 +
src/backend/replication/logical/snapbuild.c | 3 +
src/backend/utils/time/snapmgr.c | 66 +++++++++
src/include/utils/snapmgr.h | 1 +
6 files changed, 287 insertions(+)
create mode 100644 contrib/test_decoding/expected/startup_race.out
create mode 100644 contrib/test_decoding/specs/startup_race.spec
diff --git a/contrib/test_decoding/expected/startup_race.out b/contrib/test_decoding/expected/startup_race.out
new file mode 100644
index 00000000000..597fa617831
--- /dev/null
+++ b/contrib/test_decoding/expected/startup_race.out
@@ -0,0 +1,85 @@
+Parsed test spec with 6 sessions
+
+starting permutation: s1_assign_xid s2_set_snapshot s3_assign_xid s1_rollback s3_rollback s4_do_changes s5_wake_up_full_snapshot s2_scan s2_rollback s5_wake_up_before_clog s6_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s1_assign_xid:
+ BEGIN;
+ CREATE TABLE b(i int);
+
+step s2_set_snapshot:
+ BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
+ SET TRANSACTION SNAPSHOT 'from_slot';
+ <waiting ...>
+step s3_assign_xid:
+ BEGIN;
+ CREATE TABLE c(i int);
+
+step s1_rollback:
+ ROLLBACK;
+
+step s3_rollback:
+ ROLLBACK;
+
+step s4_do_changes:
+ INSERT INTO a(i, j) VALUES (3, 3);
+ UPDATE a SET j = j + 1 WHERE i = 1;
+ DELETE FROM a WHERE i = 2;
+ <waiting ...>
+step s5_wake_up_full_snapshot:
+ SELECT injection_points_wakeup('snapbuild-full-snapshot');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s2_set_snapshot: <... completed>
+step s2_scan:
+ TABLE a;
+
+i|j
+-+-
+1|1
+2|2
+(2 rows)
+
+step s2_rollback:
+ ROLLBACK;
+
+step s5_wake_up_before_clog:
+ SELECT injection_points_wakeup('before-clog-update');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_do_changes: <... completed>
+step s6_check:
+ SELECT * FROM a ORDER BY i;
+
+i|j
+-+-
+1|1
+2|2
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/contrib/test_decoding/specs/startup_race.spec b/contrib/test_decoding/specs/startup_race.spec
new file mode 100644
index 00000000000..8f67e07fa7a
--- /dev/null
+++ b/contrib/test_decoding/specs/startup_race.spec
@@ -0,0 +1,126 @@
+setup
+{
+ CREATE TABLE a(i int primary key, j int) WITH (autovacuum_enabled = off);
+ INSERT INTO a(i, j) VALUES (1, 1), (2, 2);
+ CREATE EXTENSION injection_points;
+}
+
+session s1
+step s1_assign_xid
+{
+ BEGIN;
+ CREATE TABLE b(i int);
+}
+step s1_rollback
+{
+ ROLLBACK;
+}
+
+session s2
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('snapbuild-full-snapshot', 'wait');
+}
+# Use special, hard-wired snapshot name to set the initial snapshot from
+# logical replication slot.
+step s2_set_snapshot
+{
+ BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
+ SET TRANSACTION SNAPSHOT 'from_slot';
+}
+# Perform the scan.
+step s2_scan
+{
+ TABLE a;
+}
+step s2_rollback
+{
+ ROLLBACK;
+}
+teardown
+{
+ SELECT injection_points_detach('snapbuild-full-snapshot');
+}
+
+session s3
+step s3_assign_xid
+{
+ BEGIN;
+ CREATE TABLE c(i int);
+}
+step s3_rollback
+{
+ ROLLBACK;
+}
+
+session s4
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-clog-update', 'wait');
+}
+step s4_do_changes
+{
+ INSERT INTO a(i, j) VALUES (3, 3);
+ UPDATE a SET j = j + 1 WHERE i = 1;
+ DELETE FROM a WHERE i = 2;
+}
+teardown
+{
+ SELECT injection_points_detach('before-clog-update');
+}
+
+session s5
+step s5_wake_up_full_snapshot
+{
+ SELECT injection_points_wakeup('snapbuild-full-snapshot');
+}
+step s5_wake_up_before_clog
+{
+ SELECT injection_points_wakeup('before-clog-update');
+}
+
+session s6
+step s6_check
+{
+ SELECT * FROM a ORDER BY i;
+}
+
+permutation
+# Let the snapshot builder go through all the states. The problematic case
+# happens in the FULL_SNAPSHOT.
+s1_assign_xid
+# This should leave the builder in BUILDING_SNAPSHOT, waiting for the active
+# transaction to end.
+s2_set_snapshot
+# Make sure that s1_rollback does not allow going to CONSISTENT directly.
+s3_assign_xid
+# Let the builder proceed to FULL_SNAPSHOT. It should stop at the injection
+# point 'snapbuild-full-snapshot'.
+s1_rollback
+# The transaction of s3 is not needed anymore, CONSISTENT should be the next
+# stage.
+s3_rollback
+# Perform data changes before the snapshot builder triggers creation of the
+# RUNNING_XACTS record. This will stop before setting transaction status in
+# CLOG.
+s4_do_changes
+# Unblock the injection point so that the snapshot can finally be created.
+s5_wake_up_full_snapshot
+# Use the snapshot for a scan. The snapshot will consider s4 not running
+# anymore, however CLOG is not aware of the commit yet. Thus
+# HeapTupleSatisfiesMVCC will consider the transaction aborted. In particular,
+# for UPDATE, if both xmax of the old version and xmin of the new version are
+# considered aborted, so the effects of the UPDATE are lost
+# altogether. Similarly, INSERT and DELETE have no effect because the xmin
+# transaction of the inserted tuple and xmax of the deleted tuple are
+# considered aborted
+s2_scan
+s2_rollback
+# CLOG can be updated now.
+s5_wake_up_before_clog
+# Scan the table again using a new transaction, with a normal transaction
+# snapshot. The results are still wrong due to hint bits set incorrectly.
+s6_check
+
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c857e23552f..2fea45b2fed 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -1348,6 +1349,9 @@ RecordTransactionCommit(void)
&RelcacheInitFileInval);
wrote_xlog = (XactLastRecEnd != 0);
+ /* Load the injection point before entering the critical section */
+ INJECTION_POINT_LOAD("before-clog-update");
+
/*
* If we haven't been assigned an XID yet, we neither can, nor do we want
* to write a COMMIT record.
@@ -1514,6 +1518,8 @@ RecordTransactionCommit(void)
{
XLogFlush(XactLastRecEnd);
+ INJECTION_POINT_CACHED("before-clog-update", NULL);
+
/*
* Now we may update the CLOG, if we wrote a COMMIT record above
*/
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 7f79621b57e..9b09dc8eac1 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
#include "storage/procarray.h"
#include "storage/standby.h"
#include "utils/builtins.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/snapshot.h"
@@ -1387,6 +1388,8 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
errdetail("Waiting for transactions (approximately %d) older than %u to end.",
running->xcnt, running->nextXid));
+ INJECTION_POINT("snapbuild-full-snapshot", NULL);
+
SnapBuildWaitSnapshot(running, running->nextXid);
}
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 2e6197f5f35..f327b779004 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -110,10 +110,13 @@
#include "access/subtrans.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "access/xlogutils.h"
#include "datatype/timestamp.h"
#include "lib/pairingheap.h"
#include "miscadmin.h"
#include "port/pg_lfind.h"
+#include "replication/logical.h"
+#include "replication/snapbuild.h"
#include "storage/fd.h"
#include "storage/predicate.h"
#include "storage/proc.h"
@@ -1421,6 +1424,17 @@ ImportSnapshot(const char *idstr)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("a snapshot-importing transaction must have isolation level SERIALIZABLE or REPEATABLE READ")));
+ if (strcmp(idstr, "from_slot") == 0)
+ {
+ Snapshot snap;
+
+ snap = create_test_snapshot();
+ /* XXX sourcevxid shouldn't be needed in this special case */
+ SetTransactionSnapshot(snap, NULL, MyProcPid, MyProc);
+
+ return;
+ }
+
/*
* Verify the identifier: only 0-9, A-F and hyphens are allowed. We do
* this mainly to prevent reading arbitrary files.
@@ -1969,3 +1983,55 @@ ResOwnerReleaseSnapshot(Datum res)
{
UnregisterSnapshotNoOwner((Snapshot) DatumGetPointer(res));
}
+
+/*
+ * CreateReplicationSlot() with the CRS_USE_SNAPSHOT option would be useful
+ * for testing, but regression tests cannot speak both replication and query
+ * protocol at the same time. This function can be used instead to create a
+ * snapshot for special tests of logical replication.
+ */
+Snapshot
+create_test_snapshot(void)
+{
+ const char *slotname = "test_slot";
+ const char *plugin;
+ LogicalDecodingContext *ctx;
+ Snapshot snap;
+
+ /*
+ * XXX Hard-wired values are fine for the special test that needs this
+ * function.
+ */
+ plugin = "test_decoding";
+
+ Assert(!MyReplicationSlot);
+
+ CheckLogicalDecodingRequirements();
+
+ ReplicationSlotCreate(slotname, true, RS_TEMPORARY,
+ false, false, false);
+
+ /*
+ * Ensure the logical decoding is enabled before initializing the
+ * logical decoding context.
+ */
+ EnsureLogicalDecodingEnabled();
+ Assert(IsLogicalDecodingEnabled());
+
+ ctx = CreateInitDecodingContext(plugin, NIL, true,
+ InvalidXLogRecPtr,
+ XL_ROUTINE(.page_read = read_local_xlog_page,
+ .segment_open = wal_segment_open,
+ .segment_close = wal_segment_close),
+ NULL, NULL, NULL);
+
+ /* build initial snapshot, might take a while */
+ DecodingContextFindStartpoint(ctx);
+
+ /* Do what the function is called for. */
+ snap = SnapBuildInitialSnapshot(ctx->snapshot_builder);
+ snap = CopySnapshot(snap);
+ FreeDecodingContext(ctx);
+
+ return snap;
+}
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index b8c01a291a1..9f0d60ebc1b 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -123,4 +123,5 @@ extern Snapshot RestoreSnapshot(char *start_address);
struct PGPROC;
extern void RestoreTransactionSnapshot(Snapshot snapshot, struct PGPROC *source_pgproc);
+extern Snapshot create_test_snapshot(void);
#endif /* SNAPMGR_H */
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-01-20 08:30 ` Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-01-22 19:59 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 2 replies; 38+ messages in thread
From: Antonin Houska @ 2026-01-20 08:30 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Antonin Houska <ah@cybertec.at> wrote:
> I'm not sure yet how to fix the problem. I tried to call XactLockTableWait()
> from SnapBuildAddCommittedTxn() (like it happens in SnapBuildWaitSnapshot()),
> but it made at least one regression test (subscription/t/010_truncate.pl)
> stuck - probably a deadlock. I can spend more time on it, but maybe someone
> can come up with a good idea sooner than me.
Attached here is what I consider a possible fix - simply wait for the CLOG
update before building a new snapshot.
Unfortunately I have no idea right now how to test it using the isolation
tester. With the fix, the additional waiting makes the current test
block. (And if a step is added that unblock the session, it will not reliably
catch failure to wait.)
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
[text/x-diff] 0001-Fix-race-conditions-during-the-setup-of-logical-deco.patch (4.1K, ../../62335.1768897833@localhost/2-0001-Fix-race-conditions-during-the-setup-of-logical-deco.patch)
download | inline diff:
From 5a6002215fb8ebeaf1dde120e5f6706bca7b62ae Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Mon, 19 Jan 2026 16:07:45 +0100
Subject: [PATCH] Fix race conditions during the setup of logical decoding.
Although it's rather unlikely, it can happen that the snapshot builder
considers transaction committed (according to WAL) before the commit could be
recorded in CLOG. In an extreme case, snapshot can even be created and used in
between. Since both snapshot and CLOG are needed for visibility checks, this
inconsistency can make them work incorrectly.
The typical symptom is that a transaction that the snapshot considers not
running anymore is (per CLOG) considered aborted instead of committed. Thus a
new tuple version can be evaluated as invisible (if xmin is incorrectly
considered aborted) or a deleted tuple version can be evaluated as visible (if
xmax is incorrectly considered aborted).
This patch fixes the problem by checking if all the XIDs that the new snapshot
considers committed are really committed per CLOG. If at least one is not, the
check is repeated after a short delay. However, a single check is sufficient
in almost all cases, so the performance impact should be minimal.
---
src/backend/replication/logical/snapbuild.c | 41 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 1 +
2 files changed, 42 insertions(+)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 9b09dc8eac1..c02d08f3417 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -400,6 +400,47 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
snapshot->xmin = builder->xmin;
snapshot->xmax = builder->xmax;
+ /*
+ * Although it's very unlikely, it's possible that a commit WAL record was
+ * decoded but CLOG is not aware of the commit yet. Should the CLOG update
+ * be delayed even more, visibility checks that use this snapshot could
+ * work incorrectly. Therefore we check the CLOG status here.
+ */
+ while (true)
+ {
+ bool found = false;
+
+ for (int i = 0; i < builder->committed.xcnt; i++)
+ {
+ /*
+ * XXX Is it worth remembering the XIDs that appear to be
+ * committed per CLOG and skipping them in the next iteration of
+ * the outer loop? Not sure it's worth the effort - a single
+ * iteration is enough in most cases.
+ */
+ if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
+ {
+ found = true;
+
+ /*
+ * Wait a bit before going to the next iteration of the outer
+ * loop. The race conditions we address here is pretty rare,
+ * so we shouldn't need to wait too long.
+ */
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+ 10L,
+ WAIT_EVENT_SNAPBUILD_CLOG);
+ ResetLatch(MyLatch);
+
+ break;
+ }
+ }
+
+ if (!found)
+ break;
+ }
+
/* store all transactions to be treated as committed by this snapshot */
snapshot->xip =
(TransactionId *) ((char *) snapshot + sizeof(SnapshotData));
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 5537a2d2530..b6318b0cf37 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -181,6 +181,7 @@ PG_SLEEP "Waiting due to a call to <function>pg_sleep</function> or a sibling fu
RECOVERY_APPLY_DELAY "Waiting to apply WAL during recovery because of a delay setting."
RECOVERY_RETRIEVE_RETRY_INTERVAL "Waiting during recovery when WAL data is not available from any source (<filename>pg_wal</filename>, archive or stream)."
REGISTER_SYNC_REQUEST "Waiting while sending synchronization requests to the checkpointer, because the request queue is full."
+SNAPBUILD_CLOG "Waiting for CLOG update before building snapshot."
SPIN_DELAY "Waiting while acquiring a contended spinlock."
VACUUM_DELAY "Waiting in a cost-based vacuum delay point."
VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-01-20 17:50 ` Andres Freund <andres@anarazel.de>
2026-01-22 00:24 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-01-23 08:02 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
1 sibling, 3 replies; 38+ messages in thread
From: Andres Freund @ 2026-01-20 17:50 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2026-01-20 09:30:33 +0100, Antonin Houska wrote:
> Antonin Houska <ah@cybertec.at> wrote:
>
> > I'm not sure yet how to fix the problem. I tried to call XactLockTableWait()
> > from SnapBuildAddCommittedTxn() (like it happens in SnapBuildWaitSnapshot()),
> > but it made at least one regression test (subscription/t/010_truncate.pl)
> > stuck - probably a deadlock. I can spend more time on it, but maybe someone
> > can come up with a good idea sooner than me.
>
> Attached here is what I consider a possible fix - simply wait for the CLOG
> update before building a new snapshot.
I don't think that's enough - during non-timetravel visibility semantics, you
can only look at the clog if the transaction isn't marked as in-progress in
the procarray. ISTM that we need to do that here too?
Greetings,
Andres
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
@ 2026-01-22 00:24 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-01-22 10:32 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2 siblings, 1 reply; 38+ messages in thread
From: Mihail Nikalayeu @ 2026-01-22 00:24 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org
Hello, Andres.
On Tue, Jan 20, 2026 at 6:50 PM Andres Freund <andres@anarazel.de> wrote:
> I don't think that's enough - during non-timetravel visibility semantics,
you
> can only look at the clog if the transaction isn't marked as in-progress
in
> the procarray. ISTM that we need to do that here too?
Do you mean replace
> if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
to
> if (unlikely(TransactionIdIsInProgress(builder->committed.xip[i]) ||
!TransactionIdDidCommit(builder->committed.xip[i])))
?
If so, yes, it feels correct to me.
Mikhail.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-01-22 00:24 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
@ 2026-01-22 10:32 ` Antonin Houska <ah@cybertec.at>
2026-01-22 18:58 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-01-22 10:32 UTC (permalink / raw)
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:
> Hello, Andres.
>
> On Tue, Jan 20, 2026 at 6:50 PM Andres Freund <andres@anarazel.de> wrote:
> > I don't think that's enough - during non-timetravel visibility semantics, you
> > can only look at the clog if the transaction isn't marked as in-progress in
> > the procarray. ISTM that we need to do that here too?
>
> Do you mean replace
> > if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
> to
> > if (unlikely(TransactionIdIsInProgress(builder->committed.xip[i]) || !TransactionIdDidCommit(builder->committed.xip[i])))
This way the synchronous replication gets stuck, as it did when I tried to use
XactLockTableWait(): subscriber cannot confirm replication of certain LSN
because publisher is not able to even finalize the commit (due to the waiting
for the subscriber's confirmation), and therefore publisher it's not able to
decode the data and send it to the subscriber.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-01-22 00:24 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-01-22 10:32 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-01-22 18:58 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-22 19:50 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2026-01-22 18:58 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
On 2026-Jan-22, Antonin Houska wrote:
> Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:
>
> > Hello, Andres.
> >
> > On Tue, Jan 20, 2026 at 6:50 PM Andres Freund <andres@anarazel.de> wrote:
> > > I don't think that's enough - during non-timetravel visibility semantics, you
> > > can only look at the clog if the transaction isn't marked as in-progress in
> > > the procarray. ISTM that we need to do that here too?
> >
> > Do you mean replace
> > > if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
> > to
> > > if (unlikely(TransactionIdIsInProgress(builder->committed.xip[i]) || !TransactionIdDidCommit(builder->committed.xip[i])))
>
> This way the synchronous replication gets stuck, as it did when I tried to use
> XactLockTableWait(): subscriber cannot confirm replication of certain LSN
> because publisher is not able to even finalize the commit (due to the waiting
> for the subscriber's confirmation), and therefore publisher it's not able to
> decode the data and send it to the subscriber.
The layering here is wild, but if I understand it correctly, these XIDs
are all added to an array by SnapBuildAddCommittedTxn(), which in turn
is only called by SnapBuildCommitTxn(), which is only called by
DecodeCommit(), which is only called by xact_decode() when it sees a
XLOG_XACT_COMMIT or XLOG_XACT_COMMIT_PREPARED record by reading WAL.
Crucially, RecordTransactionCommit() writes the WAL first, then marks
everything as committed in CLOG, and finally does the waiting for
the synchronous replica to ACK the commit if necessary. However, the
transaction is only removed from procarray after RecordTransactionCommit
has returned.
This means that DecodeCommit() could add a transaction to the
SnapBuilder (that needs to be waited for) while that transaction is
still shown as running in ProcArray. This sounds problematic in itself,
so I'm wondering whether we should do anything (namely, wait) on
DecodeCommit() instead of hacking SnapBuildBuildSnapshot() to patch it
up by waiting after the fact.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-01-22 00:24 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-01-22 10:32 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-22 18:58 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-01-22 19:50 ` Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 38+ messages in thread
From: Álvaro Herrera @ 2026-01-22 19:50 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
On 2026-Jan-22, Álvaro Herrera wrote:
> On 2026-Jan-22, Antonin Houska wrote:
> > > Do you mean replace
> > > > if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
> > > to
> > > > if (unlikely(TransactionIdIsInProgress(builder->committed.xip[i]) || !TransactionIdDidCommit(builder->committed.xip[i])))
> >
> > This way the synchronous replication gets stuck, as it did when I tried to use
> > XactLockTableWait(): subscriber cannot confirm replication of certain LSN
> > because publisher is not able to even finalize the commit (due to the waiting
> > for the subscriber's confirmation), and therefore publisher it's not able to
> > decode the data and send it to the subscriber.
BTW, the reason XactLockTableWait and TransactionIdIsInProgress() cause
a deadlock in the same way, is that they are using essentially the same
mechanism. The former uses the Lock object on the transaction, which is
released (by the ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) call in
CommitTransaction) after RecordTransactionCommit() has returned -- that
is, after the wait on a synchronous replica has happened.
XactLockTableWait does an _additional_ test for
TransactionIdIsInProgress, but that should be pretty much innocuous at
that point.
One thing that I just realized I don't know, is what exactly are the two
pieces that are deadlocking. I mean, one is this side that's decoding
commit. But how/why is that other side, the one trying to mark the
transaction as committed, waiting on the commit decoding? Maybe there's
something that we need to do to _that_ side to prevent the blockage, so
that we can use TransactionIdIsInProgress() here.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
@ 2026-01-23 08:02 ` Antonin Houska <ah@cybertec.at>
2 siblings, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-01-23 08:02 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org; alvherre@kurilemu.de
Andres Freund <andres@anarazel.de> wrote:
> > Attached here is what I consider a possible fix - simply wait for the CLOG
> > update before building a new snapshot.
>
> I don't think that's enough - during non-timetravel visibility semantics, you
> can only look at the clog if the transaction isn't marked as in-progress in
> the procarray. ISTM that we need to do that here too?
I understand that CLOG must be up-to-date by the time the snapshot is used for
visibility checks, but I think that - from the snapshot user POV - what
matters is "snapshot->xip vs CLOG" rather than "procarray vs CLOG".
For procarray-based snapshots, this consistency is ensured by 1) not removing
the XID from procarray until the status is set in CLOG and 2) getting the list
of running transactions from procarray. Thus if an MVCC snapshot does not have
particular XID in its "xip" array, it implies that it's no longer in procarray
and therefore it's been marked in CLOG.
As for logical decoding based snapshots (whether HISTORIC_MVCC or those
converted eventually to regular MVCC), we currently do not check if CLOG is
consistent with the transaction list in snapshot->xip. What I proposed is that
we enforce this consistency by checking CLOG (and possibly waiting) before we
finalize the snapshot. Thus the snapshot user can safely assume that the
snapshot->xip array is consistent with CLOG, as if the snapshot was based on
procarray.
Or is there another issue with the CLOG itself? I thought about wraparound
(i.e. getting the XID status from a CLOG slot which is still being used by old
transactions) but I wouldn't expect that (AFAICS, CLOG truncation takes place
during XID freezing). Concurrent access to the slot should neither be a
problem since only a single byte (which is atomic) needs to be fetched during
the XID status check.
Another hypothetical problem that occurs to me is memory access ordering,
i.e. one backend creates and exports the snapshot and another one imports it
before it can see the CLOG update. It's hard to imagine though.
Or are there other concerns?
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
@ 2026-03-20 15:55 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-03-23 09:58 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2 siblings, 2 replies; 38+ messages in thread
From: Álvaro Herrera @ 2026-03-20 15:55 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Hi,
I realized that I hadn't posted the patch version I described somewhere
downthread. Here it is, as 0001.
While thinking about it for posting just now, I wondered if it would
work to consider that any transaction whose commit record has been
decoded but which nevertheless gets a true return from
TransactionIdIsInProgress(), just is not committed yet and so should be
omitted from the xip array of the snapshot. That is, if it's
still-running, then we just don't copy it into the output snapshot.
That's implemented as 0002 here. This seems somehow less controversial,
as we don't have to test TransactionIdDidCommit() for a transaction that
we haven't seen as not-running per PGPROC; and it should also be faster,
because we don't have to wait for anybody to commit. However it gives
me pause that perhaps the snapshot would not be fully correct. (Indeed
there are a few failing tests in the subscription suite).
Failing other ideas, I think we should just go with 0001. We'd need more
commentary on why is TransactionIdDidCommit() OK, when we haven't
scanned PGPROC for that xid, though.
Thanks,
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")
Attachments:
[text/x-diff] 0001-Fix-race-conditions-during-the-setup-of-logical-deco.patch (4.0K, ../../202603201543.t6gxppyyk66p@alvherre.pgsql/2-0001-Fix-race-conditions-during-the-setup-of-logical-deco.patch)
download | inline diff:
From 6bcc6c35e480ffa02117c1e6591f0bccdc70ad12 Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Mon, 19 Jan 2026 16:07:45 +0100
Subject: [PATCH 1/2] Fix race conditions during the setup of logical decoding.
Although it's rather unlikely, it can happen that the snapshot builder
considers transaction committed (according to WAL) before the commit could be
recorded in CLOG. In an extreme case, snapshot can even be created and used in
between. Since both snapshot and CLOG are needed for visibility checks, this
inconsistency can make them work incorrectly.
The typical symptom is that a transaction that the snapshot considers not
running anymore is (per CLOG) considered aborted instead of committed. Thus a
new tuple version can be evaluated as invisible (if xmin is incorrectly
considered aborted) or a deleted tuple version can be evaluated as visible (if
xmax is incorrectly considered aborted).
This patch fixes the problem by checking if all the XIDs that the new snapshot
considers committed are really committed per CLOG. If at least one is not, the
check is repeated after a short delay. However, a single check is sufficient
in almost all cases, so the performance impact should be minimal.
---
src/backend/replication/logical/snapbuild.c | 27 ++++++++++++++++++-
.../utils/activity/wait_event_names.txt | 1 +
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 37f0c6028bd..d7ea098cb37 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -377,7 +377,7 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
/*
* We misuse the original meaning of SnapshotData's xip and subxip fields
- * to make the more fitting for our needs.
+ * to make them more fitting for our needs.
*
* In the 'xip' array we store transactions that have to be treated as
* committed. Since we will only ever look at tuples from transactions
@@ -402,6 +402,31 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
snapshot->xmin = builder->xmin;
snapshot->xmax = builder->xmax;
+ /*
+ * Although it's very unlikely, it's possible that a commit WAL record was
+ * decoded but CLOG is not aware of the commit yet. Should the CLOG update
+ * be delayed even more, visibility checks that use this snapshot could
+ * work incorrectly. Therefore we check the CLOG status here.
+ */
+ for (int i = 0; i < builder->committed.xcnt; i++)
+ {
+ for (;;)
+ {
+ if (TransactionIdDidCommit(builder->committed.xip[i]))
+ break;
+ else
+ {
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT |
+ WL_EXIT_ON_PM_DEATH,
+ 10L,
+ WAIT_EVENT_SNAPBUILD_CLOG);
+ ResetLatch(MyLatch);
+ }
+ CHECK_FOR_INTERRUPTS();
+ }
+ }
+
/* store all transactions to be treated as committed by this snapshot */
snapshot->xip =
(TransactionId *) ((char *) snapshot + sizeof(SnapshotData));
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 4aa864fe3c3..987df777e47 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -181,6 +181,7 @@ PG_SLEEP "Waiting due to a call to <function>pg_sleep</function> or a sibling fu
RECOVERY_APPLY_DELAY "Waiting to apply WAL during recovery because of a delay setting."
RECOVERY_RETRIEVE_RETRY_INTERVAL "Waiting during recovery when WAL data is not available from any source (<filename>pg_wal</filename>, archive or stream)."
REGISTER_SYNC_REQUEST "Waiting while sending synchronization requests to the checkpointer, because the request queue is full."
+SNAPBUILD_CLOG "Waiting for CLOG update before building snapshot."
SPIN_DELAY "Waiting while acquiring a contended spinlock."
VACUUM_DELAY "Waiting in a cost-based vacuum delay point."
VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
--
2.47.3
[text/x-diff] 0002-What-about-just-ignoring-the-xacts-if-they-re-still-.patch (2.0K, ../../202603201543.t6gxppyyk66p@alvherre.pgsql/3-0002-What-about-just-ignoring-the-xacts-if-they-re-still-.patch)
download | inline diff:
From 54d22ff9cd46b00b3bdd06f3bdd4b22738645eb2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 20 Mar 2026 15:58:05 +0100
Subject: [PATCH 2/2] What about just ignoring the xacts if they're still
running?
---
src/backend/replication/logical/snapbuild.c | 37 +++++----------------
1 file changed, 8 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index d7ea098cb37..2a4581b75a6 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -402,38 +402,17 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
snapshot->xmin = builder->xmin;
snapshot->xmax = builder->xmax;
- /*
- * Although it's very unlikely, it's possible that a commit WAL record was
- * decoded but CLOG is not aware of the commit yet. Should the CLOG update
- * be delayed even more, visibility checks that use this snapshot could
- * work incorrectly. Therefore we check the CLOG status here.
- */
- for (int i = 0; i < builder->committed.xcnt; i++)
- {
- for (;;)
- {
- if (TransactionIdDidCommit(builder->committed.xip[i]))
- break;
- else
- {
- (void) WaitLatch(MyLatch,
- WL_LATCH_SET | WL_TIMEOUT |
- WL_EXIT_ON_PM_DEATH,
- 10L,
- WAIT_EVENT_SNAPBUILD_CLOG);
- ResetLatch(MyLatch);
- }
- CHECK_FOR_INTERRUPTS();
- }
- }
-
/* store all transactions to be treated as committed by this snapshot */
snapshot->xip =
(TransactionId *) ((char *) snapshot + sizeof(SnapshotData));
- snapshot->xcnt = builder->committed.xcnt;
- memcpy(snapshot->xip,
- builder->committed.xip,
- builder->committed.xcnt * sizeof(TransactionId));
+
+ for (int i = 0; i < builder->committed.xcnt; i++)
+ {
+ if (!TransactionIdIsInProgress(builder->committed.xip[i]))
+ {
+ snapshot->xip[snapshot->xcnt++] = builder->committed.xip[i];
+ }
+ }
/* sort so we can bsearch() */
qsort(snapshot->xip, snapshot->xcnt, sizeof(TransactionId), xidComparator);
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-03-23 09:58 ` Antonin Houska <ah@cybertec.at>
2026-06-03 16:37 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
1 sibling, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-03-23 09:58 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> I realized that I hadn't posted the patch version I described somewhere
> downthread. Here it is, as 0001.
>
> While thinking about it for posting just now, I wondered if it would
> work to consider that any transaction whose commit record has been
> decoded but which nevertheless gets a true return from
> TransactionIdIsInProgress(), just is not committed yet and so should be
> omitted from the xip array of the snapshot. That is, if it's
> still-running, then we just don't copy it into the output snapshot.
> That's implemented as 0002 here. This seems somehow less controversial,
> as we don't have to test TransactionIdDidCommit() for a transaction that
> we haven't seen as not-running per PGPROC; and it should also be faster,
> because we don't have to wait for anybody to commit.However it gives
> me pause that perhaps the snapshot would not be fully correct. (Indeed
> there are a few failing tests in the subscription suite).
I recall that in some of the patches for REPACK enhancements (snapshot
switching, MVCC-safety, etc.) for v20 I had a problem with
TransactionIdIsInProgress(). In particular, I tried to add a flag like
PROC_IN_VACUUM, to limit the REPACK's impact on VACUUM xmin horizon.
The problem was that TransactionIdIsInProgress() compares the xid to
RecentXmin before accessing CLOG. IIRC VACUUM's RecentXmin skipped the xid of
REPACK and considered its xid not in progress anymore, but since the
transaction wasn't committed yet per CLOG, VACCUM incorrectly considered it
aborted (per HeapTupleSatisfiesVacuumHorizon).
Thus I imagine that with 0002, transaction having PROC_IN_SAFE_IC set might be
added to the snapshot's list of committed transactions although its still in
progress.
> Failing other ideas, I think we should just go with 0001. We'd need more
> commentary on why is TransactionIdDidCommit() OK, when we haven't
> scanned PGPROC for that xid, though.
Mihail already told me that I should consider adding this patch to the CF. I
said that I'm aware of its importance (because REPACK probably exposes the bug
more than logical replication does) and that I'll remind you if you happen to
forget about it. However I thought that fixes of existing bugs are not subject
to feature freeze, so I did not bring it up yet.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-03-23 09:58 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-06-03 16:37 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-08-19 13:15 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Mihail Nikalayeu @ 2026-06-03 16:37 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: alvherre@kurilemu.de, Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
Hello, everyone!
Decided to add to CF to ensure we track this properly.
https://commitfest.postgresql.org/patch/6840/
Mikhail.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-03-23 09:58 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-06-03 16:37 ` Re: Race conditions in logical decoding Mihail Nikalayeu <mihailnikalayeu@gmail.com>
@ 2026-08-19 13:15 ` Antonin Houska <ah@cybertec.at>
0 siblings, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-08-19 13:15 UTC (permalink / raw)
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; +Cc: alvherre@kurilemu.de, Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:
> Decided to add to CF to ensure we track this properly.
>
> https://commitfest.postgresql.org/patch/6840/
The current CF queue is for v20, however this should better be fixed in
v19. I've added an item to
https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
I've set Alvaro as the owner because his most recent message in this thread
indicated that he's more or less ready to push the fix.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-08-21 18:16 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-08-25 17:25 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-10 23:49 ` Re: Race conditions in logical decoding Noah Misch <noah@leadboat.com>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
1 sibling, 4 replies; 38+ messages in thread
From: Álvaro Herrera @ 2026-08-21 18:16 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Mar-20, Álvaro Herrera wrote:
> Failing other ideas, I think we should just go with 0001. We'd need more
> commentary on why is TransactionIdDidCommit() OK, when we haven't
> scanned PGPROC for that xid, though.
I spent some more time stepping through the motions here. In the test I
saw, the problem is caused by the check for latestCompletedXid. The
transaction we saw as committed in WAL has not yet been removed from
ProcArray, which is what updates latestCompletedXid. So that makes
TransactionIdIsInProgress() report that yes, the transaction is in
progress, therefore we continue to wait in a loop forever, at least in
synchronous replication.
To recap: the problem was that returned a snapshot with a transaction
recorded as committed, but which was not yet marked as such in CLOG, so
when we did things like HeapTupleSatisfiesMVCC() with the snapshot so
obtained, it would run TransactionIdDidCommit(), get false from it, and
conclude that the transaction "must have aborted or crashed", therefore
marking the tuple as HEAP_XMIN_INVALID. So what we do here is ensure
that TransactionIdDidCommit() will return the correct value before
giving the snapshot back.
The other problem with this patch in the back of my mind was that we may
be doing TransactionIdDidCommit() potentially for a lot of transactions.
Instrumenting these code paths I saw that some tests in the suite would
call the transam.c routine several thousand times, and some XIDs would
repeat over and over. This may not sound like much, but we don't
actually know what happens in production systems; and every transam.c
call has the potential to do I/O to get the relevant CLOG page. And
because we do this snapshot building in places like
SnapBuildProcessChange(), it has the potential to do nasty. So I added
a quick and dirty process-local cache: the list of transactions we
tested on the previous cycle. We don't test nor wait for any
transaction that's on that list, since evidently we must have tested it
already and it cannot become uncommitted after that. All in all, we
test for each potentially in-progress transaction just once per backend.
So, what do you think of the attached?
(On second thought, it may be a good idea to plant some of my
explanation above in the new comment in SnapBuildBuildSnapshot. No time
for that right now though.)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
Attachments:
[text/x-diff] v3-0001-Fix-race-conditions-during-the-setup-of-logical-d.patch (6.1K, ../../aoiRAEAAzDnXfkDN@alvherre.pgsql/2-v3-0001-Fix-race-conditions-during-the-setup-of-logical-d.patch)
download | inline diff:
From 45252d5989dc0e06e0027142a1bfd4cfd744e48e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 21 Aug 2026 19:52:05 +0200
Subject: [PATCH v3] Fix race conditions during the setup of logical decoding.
Although it's rather unlikely, it can happen that the snapshot builder
considers transaction committed (according to WAL) before the commit could be
recorded in CLOG. In an extreme case, snapshot can even be created and used in
between. Since both snapshot and CLOG are needed for visibility checks, this
inconsistency can make them work incorrectly.
The typical symptom is that a transaction that the snapshot considers not
running anymore is (per CLOG) considered aborted instead of committed. Thus a
new tuple version can be evaluated as invisible (if xmin is incorrectly
considered aborted) or a deleted tuple version can be evaluated as visible (if
xmax is incorrectly considered aborted).
This patch fixes the problem by checking if all the XIDs that the new snapshot
considers committed are really committed per CLOG. If at least one is not, the
check is repeated after a short delay. However, a single check is sufficient
in almost all cases, so the performance impact should be minimal.
Author: Antonin Houska <ah@cybertec.at>
Discussion: https://postgr.es/m/85833.1768840165@localhost
---
src/backend/replication/logical/snapbuild.c | 74 ++++++++++++++++++-
.../utils/activity/wait_event_names.txt | 1 +
2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index f60bcf09605..31608c186dc 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -130,6 +130,7 @@
#include "access/xact.h"
#include "common/file_utils.h"
#include "miscadmin.h"
+#include "nodes/pg_list.h"
#include "pgstat.h"
#include "replication/logical.h"
#include "replication/reorderbuffer.h"
@@ -365,6 +366,8 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
{
Snapshot snapshot;
Size ssize;
+ static List *xids_already_tested = NIL;
+ static List *new_xids_already_tested = NIL;
Assert(builder->state >= SNAPBUILD_FULL_SNAPSHOT);
@@ -378,7 +381,7 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
/*
* We misuse the original meaning of SnapshotData's xip and subxip fields
- * to make the more fitting for our needs.
+ * to make them more fitting for our needs.
*
* In the 'xip' array we store transactions that have to be treated as
* committed. Since we will only ever look at tuples from transactions
@@ -403,6 +406,75 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
snapshot->xmin = builder->xmin;
snapshot->xmax = builder->xmax;
+ /*
+ * Although very unlikely, it's possible that a commit WAL record was
+ * decoded but CLOG is not aware of the commit yet. Should the CLOG update
+ * be delayed even more, visibility checks that use this snapshot could
+ * work incorrectly. Therefore we check the CLOG status here.
+ *
+ * We must not do this using TransactionIdIsInProgress()! The check there
+ * for latestCompletedXid would wreak havoc because the transaction we're
+ * interested in may not be out of ProcArray yet, but we must not wait for
+ * that. Doing the transam.c check directly is correct, though unusual.
+ *
+ * We don't want to repeatedly read the CLOG status for the same
+ * transaction, and it's easy to keep track of which ones we've already
+ * checked. Keep a list of the ones we test on each cycle, and use the
+ * list from the previous cycle to skip testing them now.
+ */
+ for (int i = 0; i < builder->committed.xcnt; i++)
+ {
+ for (;;)
+ {
+ if (list_member_xid(xids_already_tested, builder->committed.xip[i]))
+ {
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+ new_xids_already_tested = lappend_xid(new_xids_already_tested,
+ builder->committed.xip[i]);
+ MemoryContextSwitchTo(oldcxt);
+ break;
+ }
+ else if (TransactionIdDidCommit(builder->committed.xip[i]))
+ {
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+ new_xids_already_tested = lappend_xid(new_xids_already_tested,
+ builder->committed.xip[i]);
+ MemoryContextSwitchTo(oldcxt);
+ break;
+ }
+ else
+ {
+ /*
+ * Note that the other process doesn't know we're waiting for
+ * them, so nothing is going to signal us out of this latch.
+ * Therefore use a short timeout.
+ */
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT |
+ WL_EXIT_ON_PM_DEATH,
+ 2L,
+ WAIT_EVENT_SNAPBUILD_CLOG);
+ ResetLatch(MyLatch);
+ }
+ CHECK_FOR_INTERRUPTS();
+ }
+ }
+
+ /* Swap these lists for next time */
+ if (xids_already_tested != NIL)
+ list_free(xids_already_tested);
+ if (new_xids_already_tested != NIL)
+ {
+ xids_already_tested = new_xids_already_tested;
+ new_xids_already_tested = NIL;
+ }
+ else
+ xids_already_tested = NIL;
+
/* store all transactions to be treated as committed by this snapshot */
snapshot->xip =
(TransactionId *) ((char *) snapshot + sizeof(SnapshotData));
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 256b3a3c02e..55a9c8296b5 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -185,6 +185,7 @@ PG_SLEEP "Waiting due to a call to <function>pg_sleep</function> or a sibling fu
RECOVERY_APPLY_DELAY "Waiting to apply WAL during recovery because of a delay setting."
RECOVERY_RETRIEVE_RETRY_INTERVAL "Waiting during recovery when WAL data is not available from any source (<filename>pg_wal</filename>, archive or stream)."
REGISTER_SYNC_REQUEST "Waiting while sending synchronization requests to the checkpointer, because the request queue is full."
+SNAPBUILD_CLOG "Waiting for CLOG update before building snapshot."
SPIN_DELAY "Waiting while acquiring a contended spinlock."
VACUUM_DELAY "Waiting in a cost-based vacuum delay point."
VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* RE: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-08-22 16:22 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
3 siblings, 1 reply; 38+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-08-22 16:22 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Hi,
On Saturday, August 22, 2026 3:16 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> I spent some more time stepping through the motions here. In the test I saw,
> the problem is caused by the check for latestCompletedXid. The transaction
> we saw as committed in WAL has not yet been removed from ProcArray, which
> is what updates latestCompletedXid. So that makes
> TransactionIdIsInProgress() report that yes, the transaction is in progress,
> therefore we continue to wait in a loop forever, at least in synchronous
> replication.
>
> To recap: the problem was that returned a snapshot with a transaction
> recorded as committed, but which was not yet marked as such in CLOG, so
> when we did things like HeapTupleSatisfiesMVCC() with the snapshot so
> obtained, it would run TransactionIdDidCommit(), get false from it, and
> conclude that the transaction "must have aborted or crashed", therefore
> marking the tuple as HEAP_XMIN_INVALID. So what we do here is ensure
> that TransactionIdDidCommit() will return the correct value before giving the
> snapshot back.
>
>
> The other problem with this patch in the back of my mind was that we may be
> doing TransactionIdDidCommit() potentially for a lot of transactions.
> Instrumenting these code paths I saw that some tests in the suite would call
> the transam.c routine several thousand times, and some XIDs would repeat
> over and over. This may not sound like much, but we don't actually know
> what happens in production systems; and every transam.c call has the
> potential to do I/O to get the relevant CLOG page. And because we do this
> snapshot building in places like SnapBuildProcessChange(), it has the potential
> to do nasty. So I added a quick and dirty process-local cache: the list of
> transactions we tested on the previous cycle. We don't test nor wait for any
> transaction that's on that list, since evidently we must have tested it already
> and it cannot become uncommitted after that. All in all, we test for each
> potentially in-progress transaction just once per backend.
>
> So, what do you think of the attached?
Just sharing a few thoughts.
I think the cache might be better placed in the SnapBuild struct (at least on
HEAD) rather than in static variables. As currently written, it persists across
decoding sessions in the same backend - a session could build a snapshot, drop
the slot, and later create a new slot and build another snapshot, potentially
consulting stale entries from the first builder. For example, it has a wraparound
concern: after XID wrap, a cached value could refer to a different transaction,
causing us to skip the CLOG wait and reintroduce the inconsistency this patch
aims to fix.
Besides, just to confirm one note: IIUC, for exported snapshots by logicalrep, a
transaction could be treated as committed while still in PGPROC, while
concurrent MVCC snapshots still see it as in progress which looks inconsistent.
I understand that waiting for ProcArray removal in the general case could
deadlock against synchronous replication, so it's probably acceptable to leave
it unchanged for internal usage in active replication processes.
However, for cases where the snapshot is exported, would it be possible to
additionally wait for it in SnapBuildInitialSnapshot() (which is used only by
CREATE_REPLICATION_SLOT and REPACK)? Since that runs before START_REPLICATION,
the process isn't streaming or feeding any subscriber, so I believe the deadlock
wouldn't occur there. (I think that the walsender executing
CREATE_REPLICATION_SLOT shouldn't be added to sync_standby_names, otherwise
building the initial snapshot itself would already have a deadlock risk via
SnapBuildWaitSnapshot->XactLockTableWait.)
Best Regards,
Zhijie Hou
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
@ 2026-09-09 10:20 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-09 18:08 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-16 07:59 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 2 replies; 38+ messages in thread
From: Álvaro Herrera @ 2026-09-09 10:20 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Hello, replying to Hou and Houska emails in one.
On 2026-Aug-22, Zhijie Hou (Fujitsu) wrote:
> I think the cache might be better placed in the SnapBuild struct (at least on
> HEAD) rather than in static variables. As currently written, it persists across
> decoding sessions in the same backend - a session could build a snapshot, drop
> the slot, and later create a new slot and build another snapshot, potentially
> consulting stale entries from the first builder. For example, it has a wraparound
> concern: after XID wrap, a cached value could refer to a different transaction,
> causing us to skip the CLOG wait and reintroduce the inconsistency this patch
> aims to fix.
Hmm, yeah, there's definitely a problem with that. Now, this bug can
affect logical decoding in existing releases as well, so we should
backpatch this fix, and I'm not sure about changing SnapBuild in
backbranches. Maybe another approach is to use file-level statics
(rather than function-level) so that they can be reset by slot drop
routines.
> Besides, just to confirm one note: IIUC, for exported snapshots by logicalrep, a
> transaction could be treated as committed while still in PGPROC, while
> concurrent MVCC snapshots still see it as in progress which looks inconsistent.
> I understand that waiting for ProcArray removal in the general case could
> deadlock against synchronous replication, so it's probably acceptable to leave
> it unchanged for internal usage in active replication processes.
OK. TBH I'm somewhat unease about this inconsistency; I wondered about
doing the CLOG-based test only in sync replication and using
XidIsInProgress otherwise, but didn't really try (which is to say: I'm
not even sure if it's _possible_ at all.)
> However, for cases where the snapshot is exported, would it be possible to
> additionally wait for it in SnapBuildInitialSnapshot() (which is used only by
> CREATE_REPLICATION_SLOT and REPACK)? Since that runs before START_REPLICATION,
> the process isn't streaming or feeding any subscriber, so I believe the deadlock
> wouldn't occur there. (I think that the walsender executing
> CREATE_REPLICATION_SLOT shouldn't be added to sync_standby_names, otherwise
> building the initial snapshot itself would already have a deadlock risk via
> SnapBuildWaitSnapshot->XactLockTableWait.)
Yeah, we could do that.
Do you want to try and write a patch?
On 2026-Aug-25, Antonin Houska wrote:
> Besides, that, it occurred to me that a sorted array might be appropriate
> instead of a list, so that bsearch() can be used, but I'm not sure about that.
I think we should absolutely do something like that, because repeated
list_member_oid() are unlikely to be great. Maybe as output of each run
we end up with an unsorted array; when SnapBuildBuildSnapshot runs next
time, the first thing we do is sort the array for bsearch. That way, we
don't have to sort unless absolutely necessary.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Para tener más hay que desear menos"
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-09 18:08 ` Antonin Houska <ah@cybertec.at>
1 sibling, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-09-09 18:08 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Hello, replying to Hou and Houska emails in one.
>
> On 2026-Aug-22, Zhijie Hou (Fujitsu) wrote:
>
> > I think the cache might be better placed in the SnapBuild struct (at least on
> > HEAD) rather than in static variables. As currently written, it persists across
> > decoding sessions in the same backend - a session could build a snapshot, drop
> > the slot, and later create a new slot and build another snapshot, potentially
> > consulting stale entries from the first builder. For example, it has a wraparound
> > concern: after XID wrap, a cached value could refer to a different transaction,
> > causing us to skip the CLOG wait and reintroduce the inconsistency this patch
> > aims to fix.
>
> Hmm, yeah, there's definitely a problem with that. Now, this bug can
> affect logical decoding in existing releases as well, so we should
> backpatch this fix, and I'm not sure about changing SnapBuild in
> backbranches. Maybe another approach is to use file-level statics
> (rather than function-level) so that they can be reset by slot drop
> routines.
>
> > Besides, just to confirm one note: IIUC, for exported snapshots by logicalrep, a
> > transaction could be treated as committed while still in PGPROC, while
> > concurrent MVCC snapshots still see it as in progress which looks inconsistent.
> > I understand that waiting for ProcArray removal in the general case could
> > deadlock against synchronous replication, so it's probably acceptable to leave
> > it unchanged for internal usage in active replication processes.
>
> OK. TBH I'm somewhat unease about this inconsistency; I wondered about
> doing the CLOG-based test only in sync replication and using
> XidIsInProgress otherwise, but didn't really try (which is to say: I'm
> not even sure if it's _possible_ at all.)
I'm trying to understand if this kind of inconsistency has the chance to be
seen by users. I suspect the concern is about a session having isolation level
at least REPEATABLE_READ which scans the table two times using the same
snapshot, however another session runs REPACK (CONCURRENTLY) in between. Due
to the inconsistency explained above, the snapshot might miss some changes
that REPACK already does see.
IMO the 2nd scan will not see a different version of the table the table had
to be locked before the first scan started, so REPACK won't be able to finish
until the whole transaction is finished. Even w/o keeping the lock between the
scans, both scans would retrieve the same rows as long as REPACK
(CONCURRENTLY) is MVCC-safe (currently it's is not, but should be in the
future).
Regarding logical replication, yes, this inconsistency can be the reason some
data changes are already visible on the replica while some snapshots don't yet
see it on the primary. However I think that can happen anyway if the MVCC
snapshot for the scan on the primary had been created before the snapshot for
the logical replication.
Maybe I've just misunderstood something.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* RE: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-16 07:59 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-17 09:52 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 38+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-09-16 07:59 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Hi,
On Wednesday, September 9, 2026 6:20 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Aug-22, Zhijie Hou (Fujitsu) wrote:
> >
> > Besides, just to confirm one note: IIUC, for exported snapshots by
> > logicalrep, a transaction could be treated as committed while still in
> > PGPROC, while concurrent MVCC snapshots still see it as in progress which
> looks inconsistent.
> > I understand that waiting for ProcArray removal in the general case
> > could deadlock against synchronous replication, so it's probably
> > acceptable to leave it unchanged for internal usage in active replication processes.
>
> OK. TBH I'm somewhat unease about this inconsistency; I wondered about
> doing the CLOG-based test only in sync replication and using XidIsInProgress
> otherwise, but didn't really try (which is to say: I'm not even sure if it's
> _possible_ at all.)
I experimented with this a bit and confirmed that the inconsistency exists,
though it doesn't affect REPACK (CONCURRENTLY), the command takes an exclusive
lock on the table when switching the old and new heap, which forces any
concurrent transactions on that table to finish first. However, the
inconsistency can be observed if a user directly uses the exported snapshot, as
shown in the attachment (generated with AI assistance).
>
> > However, for cases where the snapshot is exported, would it be
> > possible to additionally wait for it in SnapBuildInitialSnapshot()
> > (which is used only by CREATE_REPLICATION_SLOT and REPACK)? Since
> that
> > runs before START_REPLICATION, the process isn't streaming or feeding
> > any subscriber, so I believe the deadlock wouldn't occur there. (I
> > think that the walsender executing CREATE_REPLICATION_SLOT shouldn't
> > be added to sync_standby_names, otherwise building the initial
> > snapshot itself would already have a deadlock risk via
> > SnapBuildWaitSnapshot->XactLockTableWait.)
>
> Yeah, we could do that.
>
> Do you want to try and write a patch?
I see that Rui Zhao has shared a patch based on this approach [1]. The email is
lengthy, but the core idea is to perform the wait inside
SnapBuildInitialSnapshot().
[1] https://www.postgresql.org/message-id/CAHWVJhHXyLtS-8mdL9WhEWfsERb%3DFN7JdPD0GYAXgTmCnqbYGw%40mail.g...
Best Regards,
Zhijie Hou
Attachments:
[application/octet-stream] nocfbot-0001-Test-snapshot-export-while-a-committing-xact-is.patch (16.0K, ../../TY4PR01MB1771857EBA55C85BC365AC5E594B92@TY4PR01MB17718.jpnprd01.prod.outlook.com/2-nocfbot-0001-Test-snapshot-export-while-a-committing-xact-is.patch)
download | inline diff:
From 04961a283f7927a580c95c8a905e8171efef3cae Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Wed, 16 Sep 2026 15:00:31 +0800
Subject: [PATCH vPOC] Test snapshot export while a committing xact is still in
the procarray
A transaction's commit becomes visible to logical decoding as soon as its
commit record has been inserted into WAL: SnapBuildCommitTxn() then puts
the xid into the builder's list of committed transactions, and
SnapBuildInitialSnapshot() converts that list into the exported MVCC
snapshot's in-progress array, so the xid ends up treated as committed.
The committing backend removes itself from the procarray only later, in
ProcArrayEndTransaction(). In between, a snapshot exported by
CREATE_REPLICATION_SLOT ... (SNAPSHOT 'export') treats the transaction as
committed while concurrent MVCC snapshots still see it as in progress.
The snapshot builder's state transitions wait for the transactions listed
in a xl_running_xacts record via XactLockTableWait(), with one exception:
the FULL_SNAPSHOT -> CONSISTENT transition does not wait. That makes the
window reachable deterministically: if the committing transaction is still
in the procarray when that last transition happens, CREATE_REPLICATION_SLOT
returns with the transaction already in the committed list of the exported
snapshot.
Add a TAP test that drives the snapshot builder through its states with
two sacrificial transactions and stops a third transaction with a new
injection point in CommitTransaction(), after RecordTransactionCommit()
has flushed the commit record and updated the CLOG, but before
ProcArrayEndTransaction(). The slot creation then returns while the
committing transaction is still in the procarray, and the exported
snapshot sees its row while a concurrent MVCC snapshot does not. A
control scenario in which the committing transaction is let finish before
the builder reaches the consistent state shows both snapshots agreeing.
---
src/backend/access/transam/xact.c | 11 +
src/test/recovery/meson.build | 1 +
.../t/058_exported_snapshot_pgproc_window.pl | 293 ++++++++++++++++++
3 files changed, 305 insertions(+)
create mode 100644 src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index aca92507ebd..0f44a5942a2 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -2404,6 +2405,16 @@ CommitTransaction(void)
* durably commit.
*/
latestXid = RecordTransactionCommit();
+
+ /*
+ * Testing hook for the window in which the commit record has already
+ * been written (and flushed, if synchronous_commit is on), but the
+ * PGPROC entry has not been cleaned yet and shared invalidations have
+ * not been sent. Only fire when a commit record was actually
+ * written, so that read-only transactions don't get stuck here.
+ */
+ if (TransactionIdIsValid(latestXid))
+ INJECTION_POINT("xact-commit-after-record", NULL);
}
else
{
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 72113c5ac6e..ac2bf15e54f 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -65,6 +65,7 @@ tests += {
't/054_unlogged_sequence_promotion.pl',
't/055_cascade_reconnect.pl',
't/056_standby_snapshot_export.pl',
+ 't/058_exported_snapshot_pgproc_window.pl',
],
},
}
diff --git a/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl b/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
new file mode 100644
index 00000000000..4b6d8dd8353
--- /dev/null
+++ b/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
@@ -0,0 +1,293 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+#
+# Test that a snapshot exported by CREATE_REPLICATION_SLOT ... (SNAPSHOT
+# 'export') can treat a transaction as committed while that transaction is
+# still in the procarray, so that concurrent MVCC snapshots taken by other
+# backends still see it as in progress.
+#
+# A transaction's commit becomes visible to logical decoding as soon as its
+# commit record has been inserted into WAL: SnapBuildCommitTxn() then puts
+# the xid into the builder's list of committed transactions, and
+# SnapBuildInitialSnapshot() converts that list into the exported MVCC
+# snapshot's in-progress array, so the xid ends up treated as committed.
+# The committing backend, however, removes itself from the procarray only
+# later, in ProcArrayEndTransaction(). In between, the exported snapshot
+# and any snapshot taken by GetSnapshotData() disagree about the
+# transaction's outcome.
+#
+# The state transitions of the snapshot builder wait for the transactions
+# listed in a xl_running_xacts record via XactLockTableWait() (see
+# SnapBuildWaitSnapshot()), with one exception: the FULL_SNAPSHOT ->
+# CONSISTENT transition does not wait. That makes the window reachable
+# deterministically: if the committing transaction is still in the
+# procarray when that last transition happens, CREATE_REPLICATION_SLOT
+# returns with the transaction already in the committed list of the
+# exported snapshot.
+#
+# An injection point in CommitTransaction() makes the window deterministic:
+# the committing session is stopped after RecordTransactionCommit() has
+# flushed the commit record and updated the CLOG, but before
+# ProcArrayEndTransaction(). Note that the CLOG is already up to date at
+# that point, so the only observer that can notice the not-yet-finished
+# transaction is the procarray - exactly the "exported snapshot sees the
+# xact as committed, concurrent MVCC snapshots see it as in progress"
+# inconsistency discussed in
+#
+# https://www.postgresql.org/message-id/aoiRAEAAzDnXfkDN@alvherre.pgsql
+#
+# A fix that makes SnapBuildInitialSnapshot() wait for such transactions to
+# leave the procarray would change this test's expectations: the slot
+# creation would then wait on the transaction's lock until the session is
+# woken up, and the exported snapshot would agree with concurrent MVCC
+# snapshots.
+#
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
+$node->append_conf(
+ 'postgresql.conf', qq{
+autovacuum = off
+checkpoint_timeout = 1h
+});
+$node->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+$node->safe_psql('postgres', q(CREATE TABLE t(i int)));
+
+# Drive the snapshot builder of a CREATE_REPLICATION_SLOT (SNAPSHOT export)
+# through its states, and stop a committing transaction (the "victim")
+# after its commit record has been flushed but before its PGPROC entry is
+# removed, so that the victim's commit is the last thing decoded before the
+# builder reaches a consistent state.
+#
+# If $wake_victim is false, the victim is left stopped, and the slot
+# creation returns while the victim is still in the procarray. If it is
+# true, the victim is woken up again before the builder can reach the
+# consistent state, which closes the window.
+#
+# Returns the name of the exported snapshot, the victim's xid, and the
+# walsender and victim sessions (kept alive by the caller as needed).
+sub export_snapshot_across_victim_commit
+{
+ my ($slot_name, $value, $wake_victim) = @_;
+
+ # First sacrificial transaction; being listed in the first
+ # xl_running_xacts record the slot creation decodes drives the snapshot
+ # builder from SNAPBUILD_START to SNAPBUILD_BUILDING_SNAPSHOT.
+ my $s1 = $node->background_psql('postgres', on_error_stop => 1);
+ $s1->query_safe('BEGIN');
+ $s1->query_safe('SELECT txid_current()');
+
+ # Walsender that creates the slot and exports the initial snapshot.
+ # The command does not return until the snapshot builder has reached a
+ # consistent point, which the rest of this test drives step by step.
+ my $ws = $node->background_psql('postgres',
+ on_error_stop => 1,
+ replication => 'database');
+ $ws->query_until(
+ qr/slot_creation_started/,
+ qq{\\echo slot_creation_started
+ CREATE_REPLICATION_SLOT $slot_name LOGICAL test_decoding (SNAPSHOT export)
+ ;
+ \\echo slot_done
+ });
+
+ # Wait until the slot's restart point has been fixed, so that the
+ # standby snapshot logged below is guaranteed to be decoded by the slot
+ # creation.
+ $node->poll_query_until(
+ 'postgres',
+ qq{SELECT count(*) FROM pg_replication_slots
+ WHERE slot_name = '$slot_name' AND restart_lsn IS NOT NULL},
+ '1')
+ or die 'timed out when waiting for the slot to be created';
+
+ # Log a xl_running_xacts record listing the first transaction.
+ # Decoding it moves the builder to BUILDING_SNAPSHOT, where it waits
+ # for that transaction's lock.
+ $node->safe_psql('postgres', q{SELECT pg_log_standby_snapshot()});
+
+ # Second sacrificial transaction, started only now so that its xid is
+ # newer than the previous record's nextXid. Being listed in the next
+ # xl_running_xacts record moves the builder to FULL_SNAPSHOT.
+ my $s2 = $node->background_psql('postgres', on_error_stop => 1);
+ $s2->query_safe('BEGIN');
+ my $x2 = $s2->query_safe('SELECT txid_current()');
+
+ # End the first transaction. The builder stops waiting for it, logs
+ # another xl_running_xacts record (listing the second transaction),
+ # decodes that record, moves to FULL_SNAPSHOT and waits for the second
+ # transaction's lock.
+ $s1->query_safe('ROLLBACK');
+
+ # Wait until the builder waits for the second transaction's lock
+ # specifically. At that point the xl_running_xacts record that took
+ # the builder to FULL_SNAPSHOT has been logged, so any xid assigned
+ # from now on is newer than the builder's next_phase_at.
+ $node->poll_query_until(
+ 'postgres',
+ qq{SELECT count(*) FROM pg_locks l JOIN pg_stat_activity a ON a.pid = l.pid
+ WHERE l.locktype = 'transactionid' AND l.transactionid = '$x2'
+ AND NOT l.granted AND a.backend_type = 'walsender'},
+ '1')
+ or die 'timed out when waiting for the slot creation to wait for the second xact';
+
+ # The victim transaction. Stop it after its commit record has been
+ # flushed and the CLOG has been updated, but before its PGPROC entry is
+ # removed.
+ my $t = $node->background_psql('postgres', on_error_stop => 1);
+ $t->query_until(
+ qr/about_to_commit/,
+ qq{SELECT injection_points_attach('xact-commit-after-record', 'wait')
+ ;
+ BEGIN
+ ;
+ INSERT INTO t VALUES ($value)
+ ;
+ \\echo about_to_commit
+ COMMIT
+ ;
+ });
+
+ # Wait until the commit record is flushed, the CLOG is updated, and the
+ # session is stopped before cleaning its PGPROC entry.
+ $node->wait_for_event('client backend', 'xact-commit-after-record');
+
+ # The victim is still in the procarray.
+ my $xt = $node->safe_psql(
+ 'postgres',
+ q{SELECT backend_xid FROM pg_stat_activity
+ WHERE wait_event = 'xact-commit-after-record'});
+ ok($xt, "$slot_name: committing backend has not cleaned its PGPROC entry");
+
+ # Detach the injection point so that nothing else can get stuck
+ # anymore.
+ $node->safe_psql('postgres',
+ q{SELECT injection_points_detach('xact-commit-after-record')});
+
+ if ($wake_victim)
+ {
+ # Close the window: let the victim's commit run to completion, so
+ # that it has left the procarray by the time the builder reaches
+ # the consistent state.
+ $node->safe_psql('postgres',
+ q{SELECT injection_points_wakeup('xact-commit-after-record')});
+
+ # Wait for the victim's COMMIT to actually return.
+ $t->query_safe('SELECT 1');
+ }
+
+ # End the second transaction. The builder wakes and logs a
+ # xl_running_xacts record. If the victim is still stopped, that record
+ # lists it as running (its PGPROC entry is still there), and decoding
+ # the record takes the builder from FULL_SNAPSHOT to CONSISTENT, a
+ # transition that does not wait for the transactions the record lists.
+ # The victim's commit record precedes that record in WAL, so the victim
+ # is in the committed list of the snapshot that gets exported. If the
+ # victim was woken up, the record lists no transactions and takes the
+ # builder to CONSISTENT only after the victim fully finished.
+ $s2->query_safe('ROLLBACK');
+
+ # The slot creation now returns.
+ my $slot_out = $ws->query_until(qr/slot_done/, '');
+ like($slot_out, qr/^\Q$slot_name\E\|/m, "$slot_name: slot creation returned");
+
+ my ($snapname) = $slot_out =~ /\|([0-9A-Fa-f]+-[0-9A-Fa-f]+-\d+)\|/m;
+ ok(defined $snapname, "$slot_name: snapshot was exported");
+ note("$slot_name: exported snapshot name: "
+ . (defined $snapname ? $snapname : '<none>'));
+
+ $s1->quit;
+ $s2->quit;
+
+ return ($snapname, $xt, $ws, $t);
+}
+
+# Scenario 1: the slot creation exports its snapshot while the victim is
+# stopped after its commit record was flushed, before cleaning its PGPROC
+# entry.
+my ($snapname, $xt, $ws1, $t1) =
+ export_snapshot_across_victim_commit('slot1', 1, 0);
+
+# The SQL-visible transaction status still reflects the unfinished commit:
+# pg_xact_status() deliberately checks the procarray before the CLOG (see
+# its comment in xid8funcs.c), like MVCC visibility checks do.
+is( $node->safe_psql('postgres', qq{SELECT pg_xact_status('$xt')}),
+ 'in progress',
+ 'pg_xact_status() still sees the transaction as in progress');
+
+# A concurrent backend still sees the victim as in progress: its row is
+# invisible to a fresh MVCC snapshot.
+is( $node->safe_psql('postgres', q{SELECT count(*) FROM t}),
+ '0',
+ 'concurrent MVCC snapshot sees the committing xact as in progress');
+
+# The exported snapshot, however, treats the victim as committed: the row
+# is visible through it. Note that this also proves that the victim's
+# commit was already recorded in the CLOG when the snapshot was exported:
+# otherwise HeapTupleSatisfiesMVCC() would have concluded from
+# TransactionIdDidCommit() that the transaction "must have aborted or
+# crashed" and the row would be invisible here, too. The two results above
+# are inconsistent with each other, which is the point of this test.
+is( $node->safe_psql(
+ 'postgres',
+ qq{BEGIN ISOLATION LEVEL REPEATABLE READ;
+ SET TRANSACTION SNAPSHOT '$snapname';
+ SELECT count(*) FROM t}),
+ '1',
+ 'exported snapshot treats the still-in-procarray xact as committed');
+
+# Let the victim finish its commit, and keep the slot.
+$node->safe_psql('postgres',
+ q{SELECT injection_points_wakeup('xact-commit-after-record')});
+$t1->query_safe('SELECT 1');
+$t1->quit;
+$ws1->quit;
+
+# Sanity check: once the commit has fully finished, all snapshots agree
+# again.
+is( $node->safe_psql('postgres', q{SELECT count(*) FROM t}),
+ '1', 'row is visible everywhere once the commit finished');
+
+# Scenario 2 (control): same choreography, but the victim is woken up
+# before the builder reaches the consistent state, so the exported snapshot
+# is taken after the victim's commit has fully finished.
+($snapname, $xt, my $ws2, my $t2) =
+ export_snapshot_across_victim_commit('slot2', 2, 1);
+
+# Both kinds of snapshots now agree: the row is visible through each.
+is( $node->safe_psql('postgres', q{SELECT count(*) FROM t}),
+ '2',
+ 'control: concurrent MVCC snapshot sees the finished xact as committed');
+is( $node->safe_psql(
+ 'postgres',
+ qq{BEGIN ISOLATION LEVEL REPEATABLE READ;
+ SET TRANSACTION SNAPSHOT '$snapname';
+ SELECT count(*) FROM t}),
+ '2',
+ 'control: exported snapshot agrees with concurrent MVCC snapshots');
+
+$t2->quit;
+$ws2->quit;
+
+done_testing();
--
2.43.0
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-16 07:59 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
@ 2026-09-17 09:52 ` Antonin Houska <ah@cybertec.at>
2026-09-17 10:10 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-09-17 09:52 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: alvherre@kurilemu.de, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
> On Wednesday, September 9, 2026 6:20 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > On 2026-Aug-22, Zhijie Hou (Fujitsu) wrote:
> > >
> > > Besides, just to confirm one note: IIUC, for exported snapshots by
> > > logicalrep, a transaction could be treated as committed while still in
> > > PGPROC, while concurrent MVCC snapshots still see it as in progress which
> > looks inconsistent.
> > > I understand that waiting for ProcArray removal in the general case
> > > could deadlock against synchronous replication, so it's probably
> > > acceptable to leave it unchanged for internal usage in active replication processes.
> >
> > OK. TBH I'm somewhat unease about this inconsistency; I wondered about
> > doing the CLOG-based test only in sync replication and using XidIsInProgress
> > otherwise, but didn't really try (which is to say: I'm not even sure if it's
> > _possible_ at all.)
>
> I experimented with this a bit and confirmed that the inconsistency exists,
> though it doesn't affect REPACK (CONCURRENTLY), the command takes an exclusive
> lock on the table when switching the old and new heap, which forces any
> concurrent transactions on that table to finish first. However, the
> inconsistency can be observed if a user directly uses the exported snapshot, as
> shown in the attachment (generated with AI assistance).
...
> diff --git a/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl b/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
> new file mode 100644
> index 00000000000..4b6d8dd8353
> --- /dev/null
> +++ b/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
> @@ -0,0 +1,293 @@
> +# Copyright (c) 2026, PostgreSQL Global Development Group
> +#
> +# Test that a snapshot exported by CREATE_REPLICATION_SLOT ... (SNAPSHOT
> +# 'export') can treat a transaction as committed while that transaction is
> +# still in the procarray, so that concurrent MVCC snapshots taken by other
> +# backends still see it as in progress.
As far as I understand, what you demonstrate here is that different backends
can have a different view of the database. Isn't that pretty common situation?
What I'd consider a problem would be a single backend (and single transaction)
seeing inconsistent data.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* RE: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-16 07:59 ` RE: Race conditions in logical decoding Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-17 09:52 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-09-17 10:10 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 0 replies; 38+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-09-17 10:10 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: alvherre@kurilemu.de <alvherre@kurilemu.de>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; Andres Freund <andres@anarazel.de>
Hi,
On Thursday, September 17, 2026 5:53 PM Antonin Houska <ah@cybertec.at> wrote:
> Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
>
> > On Wednesday, September 9, 2026 6:20 PM Álvaro Herrera
> <alvherre@kurilemu.de> wrote:
> > > OK. TBH I'm somewhat unease about this inconsistency; I wondered
> > > about doing the CLOG-based test only in sync replication and using
> > > XidIsInProgress otherwise, but didn't really try (which is to say:
> > > I'm not even sure if it's _possible_ at all.)
> >
> > I experimented with this a bit and confirmed that the inconsistency
> > exists, though it doesn't affect REPACK (CONCURRENTLY), the command
> > takes an exclusive lock on the table when switching the old and new
> > heap, which forces any concurrent transactions on that table to finish
> > first. However, the inconsistency can be observed if a user directly
> > uses the exported snapshot, as shown in the attachment (generated with AI
> assistance).
>
> ...
> > --- /dev/null
> > +++ b/src/test/recovery/t/058_exported_snapshot_pgproc_window.pl
> > @@ -0,0 +1,293 @@
> > +# Copyright (c) 2026, PostgreSQL Global Development Group # # Test
> > +that a snapshot exported by CREATE_REPLICATION_SLOT ... (SNAPSHOT #
> > +'export') can treat a transaction as committed while that transaction
> > +is # still in the procarray, so that concurrent MVCC snapshots taken
> > +by other # backends still see it as in progress.
>
> As far as I understand, what you demonstrate here is that different backends
> can have a different view of the database. Isn't that pretty common situation?
I'm personally not comfortable with the fact that an MVCC snapshot could ever
see changes from a transaction that is still considered in progress from
PGPROC's perspective. This situation can only arise when using a snapshot
exported by logical decoding. IIUC, any other normal MVCC snapshot follows the
same rule: it cannot see changes whose PGPROC entries have not been cleaned,
regardless of when the snapshot is created or used, as long as the PGPROC xid
has not been cleaned. (I may be missing some cases, please correct me if so.)
> What I'd consider a problem would be a single backend (and single
> transaction) seeing inconsistent data.
Best Regards,
Zhijie Hou
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-08-25 17:25 ` Antonin Houska <ah@cybertec.at>
3 siblings, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-08-25 17:25 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Mar-20, Álvaro Herrera wrote:
>
> > Failing other ideas, I think we should just go with 0001. We'd need more
> > commentary on why is TransactionIdDidCommit() OK, when we haven't
> > scanned PGPROC for that xid, though.
>
> I spent some more time stepping through the motions here. In the test I
> saw, the problem is caused by the check for latestCompletedXid. The
> transaction we saw as committed in WAL has not yet been removed from
> ProcArray, which is what updates latestCompletedXid. So that makes
> TransactionIdIsInProgress() report that yes, the transaction is in
> progress, therefore we continue to wait in a loop forever, at least in
> synchronous replication.
> To recap: the problem was that returned a snapshot with a transaction
> recorded as committed, but which was not yet marked as such in CLOG, so
> when we did things like HeapTupleSatisfiesMVCC() with the snapshot so
> obtained, it would run TransactionIdDidCommit(), get false from it, and
> conclude that the transaction "must have aborted or crashed", therefore
> marking the tuple as HEAP_XMIN_INVALID. So what we do here is ensure
> that TransactionIdDidCommit() will return the correct value before
> giving the snapshot back.
>
>
> The other problem with this patch in the back of my mind was that we may
> be doing TransactionIdDidCommit() potentially for a lot of transactions.
> Instrumenting these code paths I saw that some tests in the suite would
> call the transam.c routine several thousand times, and some XIDs would
> repeat over and over. This may not sound like much, but we don't
> actually know what happens in production systems; and every transam.c
> call has the potential to do I/O to get the relevant CLOG page. And
> because we do this snapshot building in places like
> SnapBuildProcessChange(), it has the potential to do nasty. So I added
> a quick and dirty process-local cache: the list of transactions we
> tested on the previous cycle. We don't test nor wait for any
> transaction that's on that list, since evidently we must have tested it
> already and it cannot become uncommitted after that. All in all, we
> test for each potentially in-progress transaction just once per backend.
>
> So, what do you think of the attached?
I appreciate it that you performed the tests. I considered the race condition
pretty rarely, however it does not imply anything about the cost of the
checks: yes they can be quite frequent.
I'm just thinking if the 'xids_already_tested' variable name is
appropriate. Since you only add XIDs known to be committed, how about
something like 'xids_known_committed'?
Besides, that, it occurred to me that a sorted array might be appropriate
instead of a list, so that bsearch() can be used, but I'm not sure about that.
> (On second thought, it may be a good idea to plant some of my
> explanation above in the new comment in SnapBuildBuildSnapshot. No time
> for that right now though.)
I think it's worth mentioning at least the synchronous replication problem you
mentioned above, so it's easier to understand why we cannot use
TransactionIdIsInProgress():
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-10 23:49 ` Noah Misch <noah@leadboat.com>
2026-09-11 09:03 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
3 siblings, 1 reply; 38+ messages in thread
From: Noah Misch @ 2026-09-10 23:49 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Andres Freund <andres@anarazel.de>; Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On Mon, Jan 19, 2026 at 05:29:25PM +0100, Antonin Houska wrote:
> A stress test [1] for the REPACK patch [1] revealed data
> corruption. Eventually I found out that the problem is in postgres core. In
> particular, it can happen that a COMMIT record is decoded, but before the
> commit could be recorded in CLOG, a snapshot that takes the commit into
> account is created and even used. Visibility checks then work incorrectly
> until the CLOG gets updated.
On Fri, Aug 21, 2026 at 08:16:02PM +0200, Álvaro Herrera wrote:
> So, what do you think of the attached?
I see $SUBJECT lost open item status because released versions have the same
defect. However, REPACK (CONCURRENTLY) increases the importance of $SUBJECT
and other logical replication data loss causes. In v18, one can recreate the
replica or run integrity checks before a cutover. REPACK (CONCURRENTLY)
automates the chain: one command starts replication, waits for consistency,
and deletes the last known good copy.
If v19 ships without a fix for $SUBJECT, I think REPACK (CONCURRENTLY) docs
need to warn about the situation. How do you see it?
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-10 23:49 ` Re: Race conditions in logical decoding Noah Misch <noah@leadboat.com>
@ 2026-09-11 09:03 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-11 09:43 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2026-09-11 09:03 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Andres Freund <andres@anarazel.de>; Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-10, Noah Misch wrote:
> On Fri, Aug 21, 2026 at 08:16:02PM +0200, Álvaro Herrera wrote:
> > So, what do you think of the attached?
>
> I see $SUBJECT lost open item status because released versions have the same
> defect. However, REPACK (CONCURRENTLY) increases the importance of $SUBJECT
> and other logical replication data loss causes. In v18, one can recreate the
> replica or run integrity checks before a cutover. REPACK (CONCURRENTLY)
> automates the chain: one command starts replication, waits for consistency,
> and deletes the last known good copy.
>
> If v19 ships without a fix for $SUBJECT, I think REPACK (CONCURRENTLY) docs
> need to warn about the situation. How do you see it?
I think this kind of bug makes logical decoding effectively unusable,
because you can never predict when this bug is going to hit and
therefore when you're going to silently lose data. I agree that REPACK
(CONCURRENTLY) having automated the potential for data loss is severe.
I doubt it'd make sense to release it with this bug. So my intention is
to get it fixed before release.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-10 23:49 ` Re: Race conditions in logical decoding Noah Misch <noah@leadboat.com>
2026-09-11 09:03 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-11 09:43 ` Antonin Houska <ah@cybertec.at>
0 siblings, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-09-11 09:43 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: Noah Misch <noah@leadboat.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Sep-10, Noah Misch wrote:
>
> > On Fri, Aug 21, 2026 at 08:16:02PM +0200, Álvaro Herrera wrote:
> > > So, what do you think of the attached?
> >
> > I see $SUBJECT lost open item status because released versions have the same
> > defect. However, REPACK (CONCURRENTLY) increases the importance of $SUBJECT
> > and other logical replication data loss causes. In v18, one can recreate the
> > replica or run integrity checks before a cutover. REPACK (CONCURRENTLY)
> > automates the chain: one command starts replication, waits for consistency,
> > and deletes the last known good copy.
> >
> > If v19 ships without a fix for $SUBJECT, I think REPACK (CONCURRENTLY) docs
> > need to warn about the situation. How do you see it?
>
> I think this kind of bug makes logical decoding effectively unusable,
> because you can never predict when this bug is going to hit and
> therefore when you're going to silently lose data. I agree that REPACK
> (CONCURRENTLY) having automated the potential for data loss is severe.
> I doubt it'd make sense to release it with this bug. So my intention is
> to get it fixed before release.
Actually even the impact on logical replication is worse than described
above. As I pointed out at the beginning of this thread, even the data on the
publisher can get corrupted: if visibility checks work incorrectly, hint bits
can be set incorrectly as well. The isolation tester spec file in [1] explains
the problem more in detail.
I'm not sure if such corruption was already reported - the race is probably
pretty rare - but it's possible. As for REPACK (CONCURRENTLY), I think it
makes the corruption more likely to happen because it's an additional use case
for the snapshot built by the logical decoding system.
[1] https://www.postgresql.org/message-id/flat/aqPBWUkBniZcxRl-%40alvherre.pgsql#828c33540c873236a693bfe...
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-12 17:35 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
3 siblings, 1 reply; 38+ messages in thread
From: Rui Zhao @ 2026-09-12 17:35 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Andres Freund <andres@anarazel.de>; Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Hi,
v3 applies to master (3a3524e9ac) and builds warning-free here. I think
the wait can go somewhere cheaper, and that makes the questions about the
cache go away. Three patches attached: 0001 replaces v3, 0002 and 0003
are tests that fail on master and pass with 0001.
1. The wait belongs in SnapBuildInitialSnapshot() and nowhere else:
SnapBuildBuildSnapshot() does not need it, and in
SnapBuildInitialSnapshot() it can be a wait on the transaction lock.
0001 does that.
SnapBuildInitialSnapshot() is the only place where the builder's list of
committed transactions turns into a regular MVCC snapshot, and it is
HeapTupleSatisfiesMVCC() on that snapshot that asks CLOG about a
transaction between xmin and xmax. The historic snapshots that
SnapBuildBuildSnapshot() hands to the reorder buffer never do:
HeapTupleSatisfiesHistoricMVCC() decides the range [xmin, xmax) by the
xip array alone and consults CLOG only below xmin, and builder->xmin is
always the oldestRunningXid of an xl_running_xacts record, so a
transaction below it had left the procarray, and so updated CLOG, before
that record was written. The streaming walsender therefore needs no
check at all: no CLOG read per snapshot build, no cache of tested xids
and no question of where it lives or what XID wraparound does to it, and
nothing to sort.
What is left is one read of the running transactions and a wait on the
transaction lock of those among snap->xip that are still running, the
same wait SnapBuildWaitSnapshot() does earlier in the same slot creation;
the others have left the procarray and so have updated CLOG. The two
callers, CREATE_REPLICATION_SLOT before START_REPLICATION and the
REPACK worker, stream to nobody, so the synchronous replication deadlock
of the streaming walsender does not apply to them. Waiting on the lock
rather than polling CLOG also settles Hou's point: once the wait returns,
the transaction has left the procarray, and the exported snapshot agrees
with any snapshot another backend takes afterwards.
A transaction stuck in its synchronous commit is already waited for
today: with a subscription as the synchronous standby, the subscription
disabled and an INSERT waiting in SyncRepWaitForLSN(), a slot creation
with USE_SNAPSHOT on master sits in SnapBuildWaitSnapshot() on that
transaction's lock, since the xl_running_xacts record lists it, and comes
back with the row visible once the subscription is enabled again. Same
with 0001. make check-world passes with the three patches, including
010_truncate.pl, which runs synchronous logical replication.
Before settling on 0001 I went through the side effects I could think
of. Here is the list, so you can see what was considered.
(a) The wait gets wider. Slot creation and REPACK (CONCURRENTLY) now also
wait for transactions whose commit record is decoded but which have not
left the procarray. Normally that is microseconds. Under synchronous
replication with a slow or missing standby it is as long as the commit's
own wait, and in one position where today's code does not wait at all: a
transaction listed in the xl_running_xacts record that takes the builder
to CONSISTENT, since that transition does not call
SnapBuildWaitSnapshot(). Today the snapshot then shows the rows of a
transaction that no other session can see yet; with 0001 it waits like
everybody else.
(b) The cost is one GetRunningTransactionData(), that is one acquisition
of ProcArrayLock and XidGenLock, the same call LogStandbySnapshot() makes
in SnapBuildWaitSnapshot(), and a bsearch() in snap->xip per running
xid. snap->xip itself can be large: while an exportable snapshot is being
built every commit is tracked, not only the catalog-changing ones, so it
holds every commit since the oldest transaction still running at the
last xl_running_xacts record started. Its size hardly enters the cost:
with 100000 and 1000000 such commits in the list, and one transaction
running, the read and the loop took 1 and 3 microseconds here. The wait
shows up as Lock/transactionid, so no new wait event is needed.
(c) Concerns that turned out not to apply. Deadlock: the transactions
waited for have written their commit record, so the only thing they can
still wait for is a synchronous standby's confirmation; confirmations
come from walsenders in STREAMING state, a walsender creating a slot is
in STARTUP, also when it streamed on the same connection before, and the
REPACK worker is no walsender. The worker is in its leader's lock group,
so the deadlock detector sees its wait as the leader's. Two-phase commit:
a prepared transaction's xid lock is held by its dummy PGPROC, and
FinishPreparedTransaction() removes it from the procarray before
releasing the locks. Standby: GetRunningTransactionData() is not made for
recovery, and nothing is needed there: a slot created on a standby
decodes only replayed WAL, and replaying a commit record updates CLOG
before the transaction stops being known as running, so 0001 skips the
wait during recovery; 056_standby_snapshot_export.pl passes. Waiting for
ourselves: USE_SNAPSHOT requires that no query ran in the transaction
yet, the REPACK worker asserts it has no xid, and our own xid cannot be
in the committed list anyway. Cancellation: the lock wait is
interruptible, and lock_timeout applies to it as it already does to
SnapBuildWaitSnapshot(); an error in the loop happens before
MyProc->xmin is set, so nothing is left half done. Asynchronous commit:
the walsender cannot read the commit record before it is flushed, and
CLOG is set by then, so there is no window in the first place.
(d) What I did not do. The standby case rests on 056 and the reasoning
above, not on a test of the race there.
All in all I think 0001 is light enough, and its side effects small
enough, for a fix that is to be backpatched.
2. 0002 is a test that fails on master and passes with 0001: an injection
point in RecordTransactionCommit() between the flush of the commit record
and the CLOG update, and an isolation spec in src/test/modules/
injection_points that runs REPACK (CONCURRENTLY) against it.
The table has two columns and starts with the rows 1|1 and 2|2. Two
sessions hold an XID so that the builder goes through BUILDING_SNAPSHOT
and FULL_SNAPSHOT; while the REPACK worker waits for the second one, a
third session inserts 3|3, changes row 1 to 1|2 and deletes row 2, and
stops at the injection point; then the second session rolls back. On
master the repacked table still has 1|1 and 2|2, that is, none of the
three changes, although the transaction commits fine afterwards. With
0001 REPACK waits for it and the table has 1|2 and 3|3. This is
the same scenario as Antonin's startup_race.spec from January, without
the hook in SET TRANSACTION SNAPSHOT; the 'snapbuild-full-snapshot'
injection point is not needed either, the builder's own wait for the
second session leaves the window open.
The isolation tester sees waits on heavyweight locks and injection
points, nothing else. With v3 in place of 0001 the REPACK step sits in
the latch loop, the tester waits for it and cancels it after 360
seconds. That is the problem Antonin ran into with the isolation tester
in January, and the lock wait of 0001 is what makes the test possible.
One change to the injection_points module is needed: injection_wait()
attaches to the module's shared memory before checking whether the point
is meant for this process, and the attach allocates memory. With the
point attached locally to one session, every other backend that commits
meanwhile, REPACK's own transaction and autovacuum in this test, fails
the allocation assertion inside the critical section. 0002 checks the
condition first.
3. 0003 is the same test through the replication protocol, as a TAP test
in src/test/recovery, since that is the path released branches have:
CREATE_REPLICATION_SLOT ... USE_SNAPSHOT on a database connection takes
the place of REPACK, the table and the other sessions are as in 2.
On master the slot comes back while the transaction still sits before its
CLOG update, a SELECT through the slot's snapshot shows 1|1 and 2|2, and
so does every later session: heap_page_items() shows that this one scan
set HEAP_XMAX_INVALID on the two old row versions and HEAP_XMIN_INVALID
on the inserted row, as if the transaction had aborted. That is the
publisher-side damage Antonin described. With 0001 the walsender waits
on the transaction's lock, and the snapshot and later sessions show 1|2
and 3|3.
Regards,
Rui
Attachments:
[application/octet-stream] 0001-Wait-for-the-transactions-of-an-initial-decoding-sna.patch (3.8K, ../../CAHWVJhHXyLtS-8mdL9WhEWfsERb=FN7JdPD0GYAXgTmCnqbYGw@mail.gmail.com/2-0001-Wait-for-the-transactions-of-an-initial-decoding-sna.patch)
download | inline diff:
From d75aedd8e85407a1cd5d1c103ecc938dfb64c1bd Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:00:27 +0800
Subject: [PATCH 1/3] Wait for the transactions of an initial decoding snapshot
to finish
SnapBuildInitialSnapshot() converts the snapshot builder's list of
committed transactions into a regular MVCC snapshot, which is then used
with HeapTupleSatisfiesMVCC(). That function consults CLOG about the
transactions the snapshot takes as not running, so each of them has to
have finished committing before the snapshot is handed out: the commit
record is written first, CLOG is updated afterwards, and the transaction
stays in the procarray until after that.
Read the set of running transactions once, and wait on the transaction
lock of those that are in the snapshot's list, as SnapBuildWaitSnapshot()
does in the same code path; the others have left the procarray and so
have updated CLOG. Historic snapshots built by SnapBuildBuildSnapshot()
need no such wait: they rely on the xip array for transactions between
xmin and xmax, and consult CLOG only for transactions below xmin, which
had left the procarray when the xl_running_xacts record that set xmin was
written.
---
src/backend/replication/logical/snapbuild.c | 49 +++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index de491ea0c4..76fcda55be 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -470,6 +470,55 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
snap = SnapBuildBuildSnapshot(builder);
+ /*
+ * The commit records of the transactions in snap->xip have been decoded,
+ * but the transactions themselves may not have finished committing: a
+ * transaction writes its commit record, then updates CLOG, then waits for
+ * synchronous replication if configured, and only then leaves the
+ * procarray. The snapshot built here is used by HeapTupleSatisfiesMVCC(),
+ * which takes these transactions as not running and consults CLOG about
+ * them, so every one of them has to have finished. Read the set of
+ * running transactions once and wait, on the transaction lock, for those
+ * of snap->xip that are still in it; the others have left the procarray
+ * and therefore have updated CLOG.
+ *
+ * A subtransaction is covered by its top-level transaction, which is in
+ * snap->xip as well, or was purged from it because it is below xmin and
+ * thus finished long ago.
+ *
+ * Historic snapshots do not need this: between xmin and xmax they rely on
+ * xip alone, and transactions below xmin had left the procarray by the
+ * time the xl_running_xacts record that set xmin was written.
+ *
+ * This is the same wait as in SnapBuildWaitSnapshot(). It is safe here
+ * because we are creating a slot or preparing REPACK, not streaming to a
+ * subscriber whose confirmation one of these transactions might be
+ * waiting for.
+ *
+ * During recovery the decoded commit record has been replayed already,
+ * and replaying it updates CLOG before the transaction stops being known
+ * as running, so there is nothing to wait for.
+ */
+ if (!RecoveryInProgress())
+ {
+ RunningTransactions running;
+ int nrunning;
+
+ running = GetRunningTransactionData();
+ nrunning = running->xcnt + running->subxcnt;
+ LWLockRelease(ProcArrayLock);
+ LWLockRelease(XidGenLock);
+
+ for (int i = 0; i < nrunning; i++)
+ {
+ TransactionId running_xid = running->xids[i];
+
+ if (bsearch(&running_xid, snap->xip, snap->xcnt,
+ sizeof(TransactionId), xidComparator) != NULL)
+ XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
+ }
+ }
+
/*
* Building an initial snapshot is expensive and an unenforced xmin
* horizon would have bad consequences, therefore always double-check that
--
2.43.7
[application/octet-stream] 0002-Test-the-initial-decoding-snapshot-against-a-commit-.patch (8.5K, ../../CAHWVJhHXyLtS-8mdL9WhEWfsERb=FN7JdPD0GYAXgTmCnqbYGw@mail.gmail.com/3-0002-Test-the-initial-decoding-snapshot-against-a-commit-.patch)
download | inline diff:
From 5c6fdcf847ea97bcfa7fce081c597b500d82d5f5 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:59:09 +0800
Subject: [PATCH 2/3] Test the initial decoding snapshot against a commit that
is not in CLOG yet
The snapshot builder counts a transaction as committed once it has decoded
its commit record, but the transaction updates CLOG only after writing that
record. An initial snapshot built in between and converted to a regular
MVCC snapshot makes HeapTupleSatisfiesMVCC() consult CLOG about a
transaction it takes as not running, and the transaction comes out as
aborted.
Add an injection point between the flush of the commit record and the
CLOG update, and an isolation test that stops a transaction there while
REPACK (CONCURRENTLY) builds its snapshot. Without a fix the repacked
table lacks the changes of that transaction.
---
src/backend/access/transam/xact.c | 10 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/repack_commit_race.out | 64 +++++++++++++
.../injection_points/injection_points.c | 11 ++-
src/test/modules/injection_points/meson.build | 1 +
.../specs/repack_commit_race.spec | 96 +++++++++++++++++++
6 files changed, 180 insertions(+), 3 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/repack_commit_race.out
create mode 100644 src/test/modules/injection_points/specs/repack_commit_race.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index aca92507eb..514b6a0079 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -1377,6 +1378,9 @@ RecordTransactionCommit(void)
&RelcacheInitFileInval);
wrote_xlog = (XactLastRecEnd != 0);
+ /* Load the injection point before entering the critical section */
+ INJECTION_POINT_LOAD("commit-before-clog-update");
+
/*
* If we haven't been assigned an XID yet, we neither can, nor do we want
* to write a COMMIT record.
@@ -1543,6 +1547,12 @@ RecordTransactionCommit(void)
{
XLogFlush(XactLastRecEnd);
+ /*
+ * The commit record is on disk, but not in CLOG yet. A test can stop
+ * here to see what others make of the transaction meanwhile.
+ */
+ INJECTION_POINT_CACHED("commit-before-clog-update", NULL);
+
/*
* Now we may update the CLOG, if we wrote a COMMIT record above
*/
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 408a35c3c2..0c00edc61e 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -18,6 +18,7 @@ ISOLATION = basic \
inplace \
reindex_concurrently_deferred \
repack \
+ repack_commit_race \
repack_decode \
repack_temporal \
repack_temporal_multirange \
diff --git a/src/test/modules/injection_points/expected/repack_commit_race.out b/src/test/modules/injection_points/expected/repack_commit_race.out
new file mode 100644
index 0000000000..4d0286029a
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_commit_race.out
@@ -0,0 +1,64 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s2_begin s1_repack s3_begin s2_rollback s4_changes s3_rollback s5_wakeup s1_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s2_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s1_repack:
+ REPACK (CONCURRENTLY) repack_race;
+ <waiting ...>
+step s3_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s2_rollback:
+ ROLLBACK;
+
+step s4_changes:
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+ <waiting ...>
+step s3_rollback:
+ ROLLBACK;
+
+step s5_wakeup:
+ SELECT injection_points_wakeup('commit-before-clog-update');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s1_repack: <... completed>
+step s4_changes: <... completed>
+step s1_check:
+ SELECT i, j FROM repack_race ORDER BY i;
+
+i|j
+-+-
+1|2
+3|3
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 66d8158d0c..5e1bbc2f5c 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -246,12 +246,17 @@ injection_wait(const char *name, const void *private_data, void *arg)
char *argstr = arg;
int delay_us = 0;
- if (inj_state == NULL)
- injection_init_shmem();
-
+ /*
+ * Check the condition before attaching to the shared state: attaching
+ * allocates memory, which a process that is not meant to wait here must
+ * not do if the injection point is in a critical section.
+ */
if (!injection_point_allowed(condition, argstr))
return;
+ if (inj_state == NULL)
+ injection_init_shmem();
+
/*
* Use the injection point name for this custom wait event. Note that
* this custom wait event name is not released, but we don't care much for
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index a7b40e084f..dda61f1756 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -47,6 +47,7 @@ tests += {
'inplace',
'reindex_concurrently_deferred',
'repack',
+ 'repack_commit_race',
'repack_decode',
'repack_temporal',
'repack_temporal_multirange',
diff --git a/src/test/modules/injection_points/specs/repack_commit_race.spec b/src/test/modules/injection_points/specs/repack_commit_race.spec
new file mode 100644
index 0000000000..a961d1c001
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_commit_race.spec
@@ -0,0 +1,96 @@
+# REPACK (CONCURRENTLY) takes its initial snapshot from the logical decoding
+# snapshot builder, which counts a transaction as committed as soon as it has
+# decoded its commit record. The transaction itself may still be between
+# writing that record and updating CLOG. The snapshot must not be used before
+# the transaction has finished committing, or the copy of the table takes it
+# as aborted and its changes are lost: decoding starts after its commit record.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_race(i int PRIMARY KEY, j int);
+ INSERT INTO repack_race(i, j) VALUES (1, 1), (2, 2);
+}
+
+teardown
+{
+ DROP TABLE repack_race;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+step s1_repack
+{
+ REPACK (CONCURRENTLY) repack_race;
+}
+step s1_check
+{
+ SELECT i, j FROM repack_race ORDER BY i;
+}
+
+# s2 and s3 keep a transaction with an XID open, so that the snapshot builder
+# has to go through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states instead of
+# becoming consistent right away.
+session s2
+step s2_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s2_rollback
+{
+ ROLLBACK;
+}
+
+session s3
+step s3_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s3_rollback
+{
+ ROLLBACK;
+}
+
+# s4 changes the table and stops after writing its commit record, before
+# updating CLOG.
+session s4
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait');
+}
+step s4_changes
+{
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+}
+teardown
+{
+ SELECT injection_points_detach('commit-before-clog-update');
+}
+
+session s5
+step s5_wakeup
+{
+ SELECT injection_points_wakeup('commit-before-clog-update');
+}
+
+# The snapshot builder waits for s2, then for s3. While it waits for s3, s4
+# writes its commit record: the builder will count s4 as committed and start
+# decoding after it, but CLOG does not know about s4 yet. REPACK must not use
+# its snapshot before s4 has finished committing.
+#
+# s4 cannot finish before s5 wakes it up, and s1 cannot finish before s4 does;
+# the marker on s4_changes keeps the reporting order stable.
+permutation
+ s2_begin
+ s1_repack
+ s3_begin
+ s2_rollback
+ s4_changes(s1_repack)
+ s3_rollback
+ s5_wakeup
+ s1_check
--
2.43.7
[application/octet-stream] 0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-commi.patch (6.1K, ../../CAHWVJhHXyLtS-8mdL9WhEWfsERb=FN7JdPD0GYAXgTmCnqbYGw@mail.gmail.com/4-0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-commi.patch)
download | inline diff:
From cf45cf9b32b0d947b05a2a62296078ece4988b1e Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sat, 12 Sep 2026 02:26:17 +0800
Subject: [PATCH 3/3] Test slot creation with USE_SNAPSHOT against a commit
that is not in CLOG yet
Same scenario as the REPACK (CONCURRENTLY) isolation test, through the
replication protocol: CREATE_REPLICATION_SLOT ... USE_SNAPSHOT has to wait
for a transaction whose commit record it has decoded but which has not
updated CLOG yet, and the snapshot and later sessions must see that
transaction's changes.
---
.../recovery/t/057_snapshot_commit_race.pl | 141 ++++++++++++++++++
1 file changed, 141 insertions(+)
create mode 100644 src/test/recovery/t/057_snapshot_commit_race.pl
diff --git a/src/test/recovery/t/057_snapshot_commit_race.pl b/src/test/recovery/t/057_snapshot_commit_race.pl
new file mode 100644
index 0000000000..af11ad2869
--- /dev/null
+++ b/src/test/recovery/t/057_snapshot_commit_race.pl
@@ -0,0 +1,141 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# The snapshot of CREATE_REPLICATION_SLOT ... USE_SNAPSHOT must not be handed
+# out while a transaction it takes as committed is still between writing its
+# commit record and updating CLOG. Checks that the slot creation waits for
+# such a transaction, and that neither the slot's snapshot nor a later
+# session loses the transaction's changes.
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+$node->safe_psql('postgres',
+ q(CREATE TABLE tab(i int PRIMARY KEY, j int);
+ INSERT INTO tab VALUES (1, 1), (2, 2)));
+
+# Wait until the given backend waits on the lock of the given transaction.
+sub wait_for_xact_lock_wait
+{
+ my ($pid, $xid, $what) = @_;
+
+ $node->poll_query_until('postgres',
+ "SELECT count(*) > 0 FROM pg_locks WHERE pid = $pid AND locktype = 'transactionid' AND transactionid = '$xid' AND NOT granted"
+ ) or die "$what did not wait on the lock of transaction $xid";
+}
+
+my $s2 = $node->background_psql('postgres');
+my $s3 = $node->background_psql('postgres');
+my $s4 = $node->background_psql('postgres');
+my $walsender = $node->background_psql('postgres', replication => 'database');
+
+my $walsender_pid = $walsender->query_safe('SELECT pg_backend_pid()');
+my $s4_pid = $s4->query_safe('SELECT pg_backend_pid()');
+
+# s2 and s3 hold transactions with an XID, so that the snapshot builder goes
+# through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states rather than becoming
+# consistent right away. The walsender waits for s2, then for s3.
+my $s2_xid = $s2->query_safe('BEGIN; SELECT pg_current_xact_id()');
+
+$walsender->query_until(
+ qr/started/, q(\echo started
+BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
+CREATE_REPLICATION_SLOT slot_race TEMPORARY LOGICAL test_decoding USE_SNAPSHOT;
+));
+wait_for_xact_lock_wait($walsender_pid, $s2_xid, 'walsender');
+
+my $s3_xid = $s3->query_safe('BEGIN; SELECT pg_current_xact_id()');
+$s2->query_safe('ROLLBACK');
+wait_for_xact_lock_wait($walsender_pid, $s3_xid, 'walsender');
+
+# While the walsender waits for s3, s4 changes the table and stops after
+# writing its commit record, before updating CLOG.
+$s4->query_safe(
+ q(SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait')));
+$s4->query_until(
+ qr/started/, q(\echo started
+BEGIN;
+INSERT INTO tab VALUES (3, 3);
+UPDATE tab SET j = j + 1 WHERE i = 1;
+DELETE FROM tab WHERE i = 2;
+COMMIT;
+));
+$node->poll_query_until('postgres',
+ "SELECT wait_event = 'commit-before-clog-update' FROM pg_stat_activity WHERE pid = $s4_pid"
+) or die "s4 did not reach the injection point";
+my $s4_xid = $node->safe_psql('postgres',
+ "SELECT backend_xid FROM pg_stat_activity WHERE pid = $s4_pid");
+
+# Now the walsender decodes s4's commit record and reaches a consistent
+# state. It must wait for s4 rather than build the snapshot.
+$s3->query_safe('ROLLBACK');
+my $state;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+ $state = $node->safe_psql('postgres',
+ "SELECT CASE WHEN a.state = 'idle in transaction' THEN 'slot created'
+ WHEN l.pid IS NOT NULL THEN 'waiting for s4' END
+ FROM pg_stat_activity a
+ LEFT JOIN pg_locks l ON l.pid = a.pid AND l.locktype = 'transactionid'
+ AND l.transactionid = '$s4_xid' AND NOT l.granted
+ WHERE a.pid = $walsender_pid");
+ last if $state ne '';
+ usleep(100_000);
+}
+is($state, 'waiting for s4',
+ 'slot creation waits for the transaction that has not updated CLOG');
+
+# If the slot got created without waiting, use its snapshot right away:
+# the scan takes s4 as aborted and sets hint bits accordingly, which is
+# what the last two checks then report.
+if ($state eq 'slot created')
+{
+ $walsender->query_until(qr/test_decoding/, '');
+ diag("rows seen through the slot's snapshot before s4 updated CLOG: "
+ . $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'));
+}
+
+$node->safe_psql('postgres',
+ "SELECT injection_points_wakeup('commit-before-clog-update')");
+$s4->quit;
+
+$node->poll_query_until('postgres',
+ "SELECT state = 'idle in transaction' FROM pg_stat_activity WHERE pid = $walsender_pid"
+) or die "slot creation did not finish";
+# drain the result of CREATE_REPLICATION_SLOT
+$walsender->query_until(qr/test_decoding/, '')
+ if $state ne 'slot created';
+
+is( $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "the slot's snapshot sees the transaction's changes");
+is( $node->safe_psql('postgres', 'SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "a new session sees the transaction's changes");
+
+$walsender->query_safe('ROLLBACK');
+$walsender->quit;
+$s2->quit;
+$s3->quit;
+$node->stop;
+
+done_testing();
--
2.43.7
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
@ 2026-09-17 08:37 ` Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-09-17 08:37 UTC (permalink / raw)
To: Rui Zhao <zhaorui126@gmail.com>; +Cc: alvherre@kurilemu.de, Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Rui Zhao <zhaorui126@gmail.com> wrote:
> 1. The wait belongs in SnapBuildInitialSnapshot() and nowhere else:
> SnapBuildBuildSnapshot() does not need it, and in
> SnapBuildInitialSnapshot() it can be a wait on the transaction lock.
> 0001 does that.
>
> SnapBuildInitialSnapshot() is the only place where the builder's list of
> committed transactions turns into a regular MVCC snapshot, and it is
> HeapTupleSatisfiesMVCC() on that snapshot that asks CLOG about a
> transaction between xmin and xmax. The historic snapshots that
> SnapBuildBuildSnapshot() hands to the reorder buffer never do:
> HeapTupleSatisfiesHistoricMVCC() decides the range [xmin, xmax) by the
> xip array alone and consults CLOG only below xmin, and builder->xmin is
> always the oldestRunningXid of an xl_running_xacts record, so a
> transaction below it had left the procarray, and so updated CLOG, before
> that record was written.
I initially thought that it's silly to rely on such tricky details, but not
consulting CLOG appears to be a design choice - see the header comment in
snapbuild.c.
* ........ Also, our snapshots need to be different in comparison to normal
* MVCC ones because in contrast to those we cannot fully rely on the clog and
* pg_subtrans for information about committed transactions because they might
* commit in the future from the POV of the WAL entry we're currently
* decoding. ...
And regarding snapshot's xmin, I agree that it's controlled by
xl_running_xacts WAL record and that it does not advance until the transaction
has been recorded in CLOG.
Thus I'm not opposed to the idea that it's enough to add the check to
SnapBuildInitialSnapshot().
> + if (!RecoveryInProgress())
> + {
> + RunningTransactions running;
> + int nrunning;
> +
> + running = GetRunningTransactionData();
> + nrunning = running->xcnt + running->subxcnt;
> + LWLockRelease(ProcArrayLock);
> + LWLockRelease(XidGenLock);
> +
> + for (int i = 0; i < nrunning; i++)
> + {
> + TransactionId running_xid = running->xids[i];
> +
> + if (bsearch(&running_xid, snap->xip, snap->xcnt,
> + sizeof(TransactionId), xidComparator) != NULL)
> + XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
> + }
> + }
I don't understand why you check all transactions in procarray, instead of
only those in snap->xip.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-09-18 12:28 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Alvaro Herrera @ 2026-09-18 12:28 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Rui Zhao <zhaorui126@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-17, Antonin Houska wrote:
> > + for (int i = 0; i < nrunning; i++)
> > + {
> > + TransactionId running_xid = running->xids[i];
> > +
> > + if (bsearch(&running_xid, snap->xip, snap->xcnt,
> > + sizeof(TransactionId), xidComparator) != NULL)
> > + XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
> > + }
> > + }
>
> I don't understand why you check all transactions in procarray, instead of
> only those in snap->xip.
Hmm, but he does: for all the transactions that are running, only those
that are found by bsearch() in the snap->xip array are waited for. Is
that not what we want?
I guess we could do it the other way around: iterate for each item on
snap->xip and search for those in running->xids. Is that what you
suggest?
We don't know offhand which array is largest; it would be better to
iterate on the smaller one and bsearch the largest. (Or maybe if both
are sorted, scan them simultaneously.) I don't find any reference to
say that running_xid is sorted.
I don't understand these two paragraphs:
* A subtransaction is covered by its top-level transaction, which is in
* snap->xip as well, or was purged from it because it is below xmin and
* thus finished long ago.
*
* Historic snapshots do not need this: between xmin and xmax they rely on
* xip alone, and transactions below xmin had left the procarray by the
* time the xl_running_xacts record that set xmin was written.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-18 13:16 ` Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-09-18 13:16 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Rui Zhao <zhaorui126@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Alvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Sep-17, Antonin Houska wrote:
>
> > > + for (int i = 0; i < nrunning; i++)
> > > + {
> > > + TransactionId running_xid = running->xids[i];
> > > +
> > > + if (bsearch(&running_xid, snap->xip, snap->xcnt,
> > > + sizeof(TransactionId), xidComparator) != NULL)
> > > + XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
> > > + }
> > > + }
> >
> > I don't understand why you check all transactions in procarray, instead of
> > only those in snap->xip.
>
> Hmm, but he does: for all the transactions that are running, only those
> that are found by bsearch() in the snap->xip array are waited for. Is
> that not what we want?
>
> I guess we could do it the other way around: iterate for each item on
> snap->xip and search for those in running->xids. Is that what you
> suggest?
>
> We don't know offhand which array is largest; it would be better to
> iterate on the smaller one and bsearch the largest. (Or maybe if both
> are sorted, scan them simultaneously.) I don't find any reference to
> say that running_xid is sorted.
Maybe I miss the point, but what's wrong about modifying the existing loop
that inverts the meaning of the ->xip array
/*
* snapbuild.c builds transactions in an "inverted" manner, which means it
* stores committed transactions in ->xip, not ones in progress. Build a
* classical snapshot by marking all non-committed transactions as
* in-progress. This can be expensive.
*/
for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
{
...
}
by calling XactLockTableWait() for each XID we find in the array (i.e. each
committed transaction)?
> I don't understand these two paragraphs:
>
> * A subtransaction is covered by its top-level transaction, which is in
> * snap->xip as well, or was purged from it because it is below xmin and
> * thus finished long ago.
Me neither. AFAIU SnapBuildCommitTxn() adds both top-level transaction and
subtransactions to the builder's array of committed transaction.
> * Historic snapshots do not need this: between xmin and xmax they rely on
> * xip alone, and transactions below xmin had left the procarray by the
> * time the xl_running_xacts record that set xmin was written.
I think this is related to the note that HeapTupleSatisfiesHistoricMVCC() does
not really use CLOG in the 3rd paragraph in [1].
[1] https://www.postgresql.org/message-id/CAHWVJhHXyLtS-8mdL9WhEWfsERb%3DFN7JdPD0GYAXgTmCnqbYGw%40mail.g...
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-09-18 14:29 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Alvaro Herrera @ 2026-09-18 14:29 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Rui Zhao <zhaorui126@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-18, Antonin Houska wrote:
> Maybe I miss the point, but what's wrong about modifying the existing loop
> that inverts the meaning of the ->xip array
>
> /*
> * snapbuild.c builds transactions in an "inverted" manner, which means it
> * stores committed transactions in ->xip, not ones in progress. Build a
> * classical snapshot by marking all non-committed transactions as
> * in-progress. This can be expensive.
> */
> for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
> {
> ...
> }
>
> by calling XactLockTableWait() for each XID we find in the array (i.e. each
> committed transaction)?
Ah, you mean something like the attached quick POC? This does pass the
two tests that Rui wrote, also attached. (I didn't test Zhijie's, which
AFAICT is written to pass with the bug and fail without it.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"They proved that being American is not just for some people"
(George Takei)
Attachments:
[text/x-diff] v5-0001-Wait-for-the-transactions-of-an-initial-decoding-.patch (2.3K, ../../aq1Jy9aVF-lIhKwe@alvherre.pgsql/2-v5-0001-Wait-for-the-transactions-of-an-initial-decoding-.patch)
download | inline diff:
From 33d6b7f2559dcb312de4cdcc3d75c93dad7dfe47 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:00:27 +0800
Subject: [PATCH v5 1/3] Wait for the transactions of an initial decoding
snapshot to finish
SnapBuildInitialSnapshot() converts the snapshot builder's list of
committed transactions into a regular MVCC snapshot, which is then used
with HeapTupleSatisfiesMVCC(). That function consults CLOG about the
transactions the snapshot takes as not running, so each of them has to
have finished committing before the snapshot is handed out: the commit
record is written first, CLOG is updated afterwards, and the transaction
stays in the procarray until after that.
Read the set of running transactions once, and wait on the transaction
lock of those that are in the snapshot's list, as SnapBuildWaitSnapshot()
does in the same code path; the others have left the procarray and so
have updated CLOG. Historic snapshots built by SnapBuildBuildSnapshot()
need no such wait: they rely on the xip array for transactions between
xmin and xmax, and consult CLOG only for transactions below xmin, which
had left the procarray when the xl_running_xacts record that set xmin was
written.
---
src/backend/replication/logical/snapbuild.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index de491ea0c4b..261f25a5cd7 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -517,8 +517,22 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("initial slot snapshot too large")));
- newxip[newxcnt++] = xid;
+ newxip[newxcnt] = xid;
}
+ else
+ {
+ /*
+ * The commit record of this transaction has been decoded, but the
+ * commit itself may not have finished, if it's still in the process
+ * of removing itself from the procarray or waiting for a synchronous
+ * standby. To avoid producing a snapshot that inconsistently shows
+ * this transaction as committed, wait until it actually is.
+ */
+ if (!RecoveryInProgress())
+ XactLockTableWait(xid, NULL, NULL, XLTW_None);
+ }
+
+ newxcnt++;
TransactionIdAdvance(xid);
}
--
2.47.3
[text/x-diff] v5-0002-Test-the-initial-decoding-snapshot-against-a-comm.patch (8.5K, ../../aq1Jy9aVF-lIhKwe@alvherre.pgsql/3-v5-0002-Test-the-initial-decoding-snapshot-against-a-comm.patch)
download | inline diff:
From c85bdd2fb2e17b2ecd734de421a94a5c46f5d5bb Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:59:09 +0800
Subject: [PATCH v5 2/3] Test the initial decoding snapshot against a commit
that is not in CLOG yet
The snapshot builder counts a transaction as committed once it has decoded
its commit record, but the transaction updates CLOG only after writing that
record. An initial snapshot built in between and converted to a regular
MVCC snapshot makes HeapTupleSatisfiesMVCC() consult CLOG about a
transaction it takes as not running, and the transaction comes out as
aborted.
Add an injection point between the flush of the commit record and the
CLOG update, and an isolation test that stops a transaction there while
REPACK (CONCURRENTLY) builds its snapshot. Without a fix the repacked
table lacks the changes of that transaction.
---
src/backend/access/transam/xact.c | 10 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/repack_commit_race.out | 64 +++++++++++++
.../injection_points/injection_points.c | 11 ++-
src/test/modules/injection_points/meson.build | 1 +
.../specs/repack_commit_race.spec | 96 +++++++++++++++++++
6 files changed, 180 insertions(+), 3 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/repack_commit_race.out
create mode 100644 src/test/modules/injection_points/specs/repack_commit_race.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index ebb010853cf..7b67db514ec 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -1377,6 +1378,9 @@ RecordTransactionCommit(void)
&RelcacheInitFileInval);
wrote_xlog = (XactLastRecEnd != 0);
+ /* Load the injection point before entering the critical section */
+ INJECTION_POINT_LOAD("commit-before-clog-update");
+
/*
* If we haven't been assigned an XID yet, we neither can, nor do we want
* to write a COMMIT record.
@@ -1543,6 +1547,12 @@ RecordTransactionCommit(void)
{
XLogFlush(XactLastRecEnd);
+ /*
+ * The commit record is on disk, but not in CLOG yet. A test can stop
+ * here to see what others make of the transaction meanwhile.
+ */
+ INJECTION_POINT_CACHED("commit-before-clog-update", NULL);
+
/*
* Now we may update the CLOG, if we wrote a COMMIT record above
*/
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 9d8b4b3540c..1c680abf7dd 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -18,6 +18,7 @@ ISOLATION = basic \
inplace \
reindex_concurrently_deferred \
repack \
+ repack_commit_race \
repack_decode \
repack_temporal \
repack_temporal_multirange \
diff --git a/src/test/modules/injection_points/expected/repack_commit_race.out b/src/test/modules/injection_points/expected/repack_commit_race.out
new file mode 100644
index 00000000000..4d0286029a5
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_commit_race.out
@@ -0,0 +1,64 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s2_begin s1_repack s3_begin s2_rollback s4_changes s3_rollback s5_wakeup s1_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s2_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s1_repack:
+ REPACK (CONCURRENTLY) repack_race;
+ <waiting ...>
+step s3_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s2_rollback:
+ ROLLBACK;
+
+step s4_changes:
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+ <waiting ...>
+step s3_rollback:
+ ROLLBACK;
+
+step s5_wakeup:
+ SELECT injection_points_wakeup('commit-before-clog-update');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s1_repack: <... completed>
+step s4_changes: <... completed>
+step s1_check:
+ SELECT i, j FROM repack_race ORDER BY i;
+
+i|j
+-+-
+1|2
+3|3
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 66d8158d0c2..5e1bbc2f5c9 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -246,12 +246,17 @@ injection_wait(const char *name, const void *private_data, void *arg)
char *argstr = arg;
int delay_us = 0;
- if (inj_state == NULL)
- injection_init_shmem();
-
+ /*
+ * Check the condition before attaching to the shared state: attaching
+ * allocates memory, which a process that is not meant to wait here must
+ * not do if the injection point is in a critical section.
+ */
if (!injection_point_allowed(condition, argstr))
return;
+ if (inj_state == NULL)
+ injection_init_shmem();
+
/*
* Use the injection point name for this custom wait event. Note that
* this custom wait event name is not released, but we don't care much for
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 80a09f34d78..45117b6ffec 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -47,6 +47,7 @@ tests += {
'inplace',
'reindex_concurrently_deferred',
'repack',
+ 'repack_commit_race',
'repack_decode',
'repack_temporal',
'repack_temporal_multirange',
diff --git a/src/test/modules/injection_points/specs/repack_commit_race.spec b/src/test/modules/injection_points/specs/repack_commit_race.spec
new file mode 100644
index 00000000000..a961d1c001d
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_commit_race.spec
@@ -0,0 +1,96 @@
+# REPACK (CONCURRENTLY) takes its initial snapshot from the logical decoding
+# snapshot builder, which counts a transaction as committed as soon as it has
+# decoded its commit record. The transaction itself may still be between
+# writing that record and updating CLOG. The snapshot must not be used before
+# the transaction has finished committing, or the copy of the table takes it
+# as aborted and its changes are lost: decoding starts after its commit record.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_race(i int PRIMARY KEY, j int);
+ INSERT INTO repack_race(i, j) VALUES (1, 1), (2, 2);
+}
+
+teardown
+{
+ DROP TABLE repack_race;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+step s1_repack
+{
+ REPACK (CONCURRENTLY) repack_race;
+}
+step s1_check
+{
+ SELECT i, j FROM repack_race ORDER BY i;
+}
+
+# s2 and s3 keep a transaction with an XID open, so that the snapshot builder
+# has to go through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states instead of
+# becoming consistent right away.
+session s2
+step s2_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s2_rollback
+{
+ ROLLBACK;
+}
+
+session s3
+step s3_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s3_rollback
+{
+ ROLLBACK;
+}
+
+# s4 changes the table and stops after writing its commit record, before
+# updating CLOG.
+session s4
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait');
+}
+step s4_changes
+{
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+}
+teardown
+{
+ SELECT injection_points_detach('commit-before-clog-update');
+}
+
+session s5
+step s5_wakeup
+{
+ SELECT injection_points_wakeup('commit-before-clog-update');
+}
+
+# The snapshot builder waits for s2, then for s3. While it waits for s3, s4
+# writes its commit record: the builder will count s4 as committed and start
+# decoding after it, but CLOG does not know about s4 yet. REPACK must not use
+# its snapshot before s4 has finished committing.
+#
+# s4 cannot finish before s5 wakes it up, and s1 cannot finish before s4 does;
+# the marker on s4_changes keeps the reporting order stable.
+permutation
+ s2_begin
+ s1_repack
+ s3_begin
+ s2_rollback
+ s4_changes(s1_repack)
+ s3_rollback
+ s5_wakeup
+ s1_check
--
2.47.3
[text/x-diff] v5-0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-co.patch (6.6K, ../../aq1Jy9aVF-lIhKwe@alvherre.pgsql/4-v5-0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-co.patch)
download | inline diff:
From c5fb1061d41e32f6c8fabd6802e12b29191ea3b4 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sat, 12 Sep 2026 02:26:17 +0800
Subject: [PATCH v5 3/3] Test slot creation with USE_SNAPSHOT against a commit
that is not in CLOG yet
Same scenario as the REPACK (CONCURRENTLY) isolation test, through the
replication protocol: CREATE_REPLICATION_SLOT ... USE_SNAPSHOT has to wait
for a transaction whose commit record it has decoded but which has not
updated CLOG yet, and the snapshot and later sessions must see that
transaction's changes.
---
src/test/recovery/meson.build | 1 +
.../recovery/t/057_snapshot_commit_race.pl | 141 ++++++++++++++++++
2 files changed, 142 insertions(+)
create mode 100644 src/test/recovery/t/057_snapshot_commit_race.pl
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 72113c5ac6e..ebb12dd8766 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -65,6 +65,7 @@ tests += {
't/054_unlogged_sequence_promotion.pl',
't/055_cascade_reconnect.pl',
't/056_standby_snapshot_export.pl',
+ 't/057_snapshot_commit_race.pl',
],
},
}
diff --git a/src/test/recovery/t/057_snapshot_commit_race.pl b/src/test/recovery/t/057_snapshot_commit_race.pl
new file mode 100644
index 00000000000..af11ad28693
--- /dev/null
+++ b/src/test/recovery/t/057_snapshot_commit_race.pl
@@ -0,0 +1,141 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# The snapshot of CREATE_REPLICATION_SLOT ... USE_SNAPSHOT must not be handed
+# out while a transaction it takes as committed is still between writing its
+# commit record and updating CLOG. Checks that the slot creation waits for
+# such a transaction, and that neither the slot's snapshot nor a later
+# session loses the transaction's changes.
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+$node->safe_psql('postgres',
+ q(CREATE TABLE tab(i int PRIMARY KEY, j int);
+ INSERT INTO tab VALUES (1, 1), (2, 2)));
+
+# Wait until the given backend waits on the lock of the given transaction.
+sub wait_for_xact_lock_wait
+{
+ my ($pid, $xid, $what) = @_;
+
+ $node->poll_query_until('postgres',
+ "SELECT count(*) > 0 FROM pg_locks WHERE pid = $pid AND locktype = 'transactionid' AND transactionid = '$xid' AND NOT granted"
+ ) or die "$what did not wait on the lock of transaction $xid";
+}
+
+my $s2 = $node->background_psql('postgres');
+my $s3 = $node->background_psql('postgres');
+my $s4 = $node->background_psql('postgres');
+my $walsender = $node->background_psql('postgres', replication => 'database');
+
+my $walsender_pid = $walsender->query_safe('SELECT pg_backend_pid()');
+my $s4_pid = $s4->query_safe('SELECT pg_backend_pid()');
+
+# s2 and s3 hold transactions with an XID, so that the snapshot builder goes
+# through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states rather than becoming
+# consistent right away. The walsender waits for s2, then for s3.
+my $s2_xid = $s2->query_safe('BEGIN; SELECT pg_current_xact_id()');
+
+$walsender->query_until(
+ qr/started/, q(\echo started
+BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
+CREATE_REPLICATION_SLOT slot_race TEMPORARY LOGICAL test_decoding USE_SNAPSHOT;
+));
+wait_for_xact_lock_wait($walsender_pid, $s2_xid, 'walsender');
+
+my $s3_xid = $s3->query_safe('BEGIN; SELECT pg_current_xact_id()');
+$s2->query_safe('ROLLBACK');
+wait_for_xact_lock_wait($walsender_pid, $s3_xid, 'walsender');
+
+# While the walsender waits for s3, s4 changes the table and stops after
+# writing its commit record, before updating CLOG.
+$s4->query_safe(
+ q(SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait')));
+$s4->query_until(
+ qr/started/, q(\echo started
+BEGIN;
+INSERT INTO tab VALUES (3, 3);
+UPDATE tab SET j = j + 1 WHERE i = 1;
+DELETE FROM tab WHERE i = 2;
+COMMIT;
+));
+$node->poll_query_until('postgres',
+ "SELECT wait_event = 'commit-before-clog-update' FROM pg_stat_activity WHERE pid = $s4_pid"
+) or die "s4 did not reach the injection point";
+my $s4_xid = $node->safe_psql('postgres',
+ "SELECT backend_xid FROM pg_stat_activity WHERE pid = $s4_pid");
+
+# Now the walsender decodes s4's commit record and reaches a consistent
+# state. It must wait for s4 rather than build the snapshot.
+$s3->query_safe('ROLLBACK');
+my $state;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+ $state = $node->safe_psql('postgres',
+ "SELECT CASE WHEN a.state = 'idle in transaction' THEN 'slot created'
+ WHEN l.pid IS NOT NULL THEN 'waiting for s4' END
+ FROM pg_stat_activity a
+ LEFT JOIN pg_locks l ON l.pid = a.pid AND l.locktype = 'transactionid'
+ AND l.transactionid = '$s4_xid' AND NOT l.granted
+ WHERE a.pid = $walsender_pid");
+ last if $state ne '';
+ usleep(100_000);
+}
+is($state, 'waiting for s4',
+ 'slot creation waits for the transaction that has not updated CLOG');
+
+# If the slot got created without waiting, use its snapshot right away:
+# the scan takes s4 as aborted and sets hint bits accordingly, which is
+# what the last two checks then report.
+if ($state eq 'slot created')
+{
+ $walsender->query_until(qr/test_decoding/, '');
+ diag("rows seen through the slot's snapshot before s4 updated CLOG: "
+ . $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'));
+}
+
+$node->safe_psql('postgres',
+ "SELECT injection_points_wakeup('commit-before-clog-update')");
+$s4->quit;
+
+$node->poll_query_until('postgres',
+ "SELECT state = 'idle in transaction' FROM pg_stat_activity WHERE pid = $walsender_pid"
+) or die "slot creation did not finish";
+# drain the result of CREATE_REPLICATION_SLOT
+$walsender->query_until(qr/test_decoding/, '')
+ if $state ne 'slot created';
+
+is( $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "the slot's snapshot sees the transaction's changes");
+is( $node->safe_psql('postgres', 'SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "a new session sees the transaction's changes");
+
+$walsender->query_safe('ROLLBACK');
+$walsender->quit;
+$s2->quit;
+$s3->quit;
+$node->stop;
+
+done_testing();
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-18 15:23 ` Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-09-18 15:23 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Rui Zhao <zhaorui126@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Alvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Sep-18, Antonin Houska wrote:
>
> > Maybe I miss the point, but what's wrong about modifying the existing loop
> > that inverts the meaning of the ->xip array
> >
> > /*
> > * snapbuild.c builds transactions in an "inverted" manner, which means it
> > * stores committed transactions in ->xip, not ones in progress. Build a
> > * classical snapshot by marking all non-committed transactions as
> > * in-progress. This can be expensive.
> > */
> > for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
> > {
> > ...
> > }
> >
> > by calling XactLockTableWait() for each XID we find in the array (i.e. each
> > committed transaction)?
>
> Ah, you mean something like the attached quick POC? This does pass the
> two tests that Rui wrote, also attached. (I didn't test Zhijie's, which
> AFAICT is written to pass with the bug and fail without it.)
Yes, I mean checking if those transactions have really ended.
Regarding [1], perhaps the idea is to avoid calling XactLockTableWait() if the
transaction is no longer in procarray. I'm not sure it's a problem to call
that function (possibly many times) for transactions that are no longer
running. (GetRunningTransactionData() is not free either.)
And regarding the deadlock with synchronous replica, my understanding is that
we avoid it by waiting in SnapBuildInitialSnapshot() instead of in
SnapBuildBuildSnapshot().
[1] https://www.postgresql.org/message-id/CAHWVJhHXyLtS-8mdL9WhEWfsERb%3DFN7JdPD0GYAXgTmCnqbYGw%40mail.g...
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-09-19 16:21 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-21 15:35 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Rui Zhao @ 2026-09-19 16:21 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Alvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-18 at 14:29 UTC, Alvaro Herrera wrote:
> This does pass the two tests that Rui wrote, also attached.
In v5-0001, newxcnt++ needs to stay in the test == NULL branch.
Otherwise committed XIDs increase the count without filling an entry
in newxip.
The attached v5-0004 applies on top of v5-0001 through v5-0003. It
puts the increment back with the assignment in 0001 and adds a
snapshot XID check to 0003's test.
With 0003's setup, after slot creation finishes, I ran this on the
connection holding the slot's snapshot:
SELECT pg_current_snapshot();
-- v5: 669:670:0
-- with the correction: 669:670:
The original REPACK and TAP tests pass with v5. The added check
requires each snapshot XID to lie in [xmin, xmax): it fails with v5
and passes with the correction. Both original tests still pass.
On 2026-Sep-17 at 08:37 UTC, Antonin Houska wrote:
> I don't understand why you check all transactions in procarray, instead of
> only those in snap->xip.
I first tried calling XactLockTableWait() for every XID in snap->xip,
the same per-XID waiting approach as v5. Even for an already finished
XID, that goes through the lock manager and calls
TransactionIdIsInProgress(). Unless its RecentXmin or cached-XID
checks suffice, that takes ProcArrayLock and scans procarray.
In the patch attached to my original mail, I instead read the
running-XID list once and used bsearch to wait only for XIDs also in
snap->xip. That was to avoid repeating this work for transactions
that had already finished.
On 2026-Sep-18 at 15:23 UTC, Antonin Houska wrote:
> I'm not sure it's a problem to call that function (possibly many times)
> for transactions that are no longer running.
The case I had in mind was a long-running transaction holding xmin
back while many other transactions commit. snap->xip can then be
much larger than the running set. GetSnapshotData() uses the same
ProcArrayLock and array, so my concern was the extra traffic on
shared state used for taking snapshots, not just slot creation time.
On 2026-Sep-12 at 17:35 UTC, I wrote:
> The two callers, CREATE_REPLICATION_SLOT before START_REPLICATION and
> the REPACK worker, stream to nobody, so the synchronous replication
> deadlock of the streaming walsender does not apply to them.
On 2026-Sep-18 at 15:23 UTC, Antonin Houska wrote:
> And regarding the deadlock with synchronous replica, my understanding is
> that we avoid it by waiting in SnapBuildInitialSnapshot() instead of in
> SnapBuildBuildSnapshot().
Yes, that's right. It is the placement of the wait, not the
procarray filtering, that avoids that deadlock.
On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> I don't understand these two paragraphs:
>
> * A subtransaction is covered by its top-level transaction, which is in
> * snap->xip as well, or was purged from it because it is below xmin and
> * thus finished long ago.
> *
> * Historic snapshots do not need this: between xmin and xmax they rely on
> * xip alone, and transactions below xmin had left the procarray by the
> * time the xl_running_xacts record that set xmin was written.
The first was meant to explain why we don't have to find every
subxid in the running-XID list. If any backend's subxid cache has
overflowed, GetRunningTransactionData() returns top-level XIDs but no
subxids. We still wait for the parent, which covers its children.
If the parent was purged from snap->xip because it is below xmin,
it has already finished, so no wait is needed.
The second was a different question: why wait only in
SnapBuildInitialSnapshot(), rather than in SnapBuildBuildSnapshot(),
which is also used to build historic snapshots? Here "this" meant
waiting for transactions to finish, not handling subtransactions.
Historic snapshots use xip for committed-XID checks in [xmin, xmax).
They can consult CLOG below xmin, but those transactions had already
finished when the running-xacts record supplying xmin was written.
So they need no extra wait.
The wait is needed when converting to a normal MVCC snapshot, whose
visibility checks can consult CLOG for transactions in [xmin, xmax)
as well. That is why I put it in SnapBuildInitialSnapshot().
Regards,
Rui
Attachments:
[application/octet-stream] v5-0004-Fix-XID-count-in-initial-decoding-snapshots.patch (2.5K, ../../CAHWVJhHwV4g-QCWDMaXPT8m-1hQ5=ZpwmNQDD3JKqmLvo7-8nA@mail.gmail.com/2-v5-0004-Fix-XID-count-in-initial-decoding-snapshots.patch)
download | inline diff:
From e8f3e9a4d71f0075af5e93abb9715f2fe0b268c2 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sat, 19 Sep 2026 23:53:05 +0800
Subject: [PATCH v5 4/4] Fix XID count in initial decoding snapshots
In v5-0001, newxcnt is incremented even for committed XIDs that are
not added to newxip. This counts uninitialized array entries as part
of the converted snapshot. Increment the count only when storing an
XID in the array.
Extend the USE_SNAPSHOT test to check that every XID in the resulting
snapshot lies in [xmin, xmax).
---
src/backend/replication/logical/snapbuild.c | 4 +---
src/test/recovery/t/057_snapshot_commit_race.pl | 10 +++++++++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 261f25a5cd..c03428b5e1 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -517,7 +517,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("initial slot snapshot too large")));
- newxip[newxcnt] = xid;
+ newxip[newxcnt++] = xid;
}
else
{
@@ -532,8 +532,6 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
XactLockTableWait(xid, NULL, NULL, XLTW_None);
}
- newxcnt++;
-
TransactionIdAdvance(xid);
}
diff --git a/src/test/recovery/t/057_snapshot_commit_race.pl b/src/test/recovery/t/057_snapshot_commit_race.pl
index af11ad2869..a0d23d9a8c 100644
--- a/src/test/recovery/t/057_snapshot_commit_race.pl
+++ b/src/test/recovery/t/057_snapshot_commit_race.pl
@@ -106,7 +106,7 @@ is($state, 'waiting for s4',
# If the slot got created without waiting, use its snapshot right away:
# the scan takes s4 as aborted and sets hint bits accordingly, which is
-# what the last two checks then report.
+# what the two row checks below then report.
if ($state eq 'slot created')
{
$walsender->query_until(qr/test_decoding/, '');
@@ -132,6 +132,14 @@ is( $node->safe_psql('postgres', 'SELECT i, j FROM tab ORDER BY i'),
"1|2\n3|3",
"a new session sees the transaction's changes");
+is( $walsender->query_safe(
+ q(WITH s AS (SELECT pg_current_snapshot() AS snap)
+ SELECT count(*) FROM s, LATERAL pg_snapshot_xip(snap) AS x(xid)
+ WHERE xid < pg_snapshot_xmin(snap) OR xid >= pg_snapshot_xmax(snap))
+ ),
+ '0',
+ 'snapshot XIDs are all within xmin and xmax');
+
$walsender->query_safe('ROLLBACK');
$walsender->quit;
$s2->quit;
--
2.43.7
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
@ 2026-09-21 15:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-21 16:56 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Alvaro Herrera @ 2026-09-21 15:35 UTC (permalink / raw)
To: Rui Zhao <zhaorui126@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-20, Rui Zhao wrote:
> On 2026-Sep-18 at 14:29 UTC, Alvaro Herrera wrote:
> > This does pass the two tests that Rui wrote, also attached.
>
> In v5-0001, newxcnt++ needs to stay in the test == NULL branch.
> Otherwise committed XIDs increase the count without filling an entry
> in newxip.
Eh, yeah.
> On 2026-Sep-17 at 08:37 UTC, Antonin Houska wrote:
> > I don't understand why you check all transactions in procarray, instead of
> > only those in snap->xip.
>
> I first tried calling XactLockTableWait() for every XID in snap->xip,
> the same per-XID waiting approach as v5. Even for an already finished
> XID, that goes through the lock manager and calls
> TransactionIdIsInProgress(). Unless its RecentXmin or cached-XID
> checks suffice, that takes ProcArrayLock and scans procarray.
>
> In the patch attached to my original mail, I instead read the
> running-XID list once and used bsearch to wait only for XIDs also in
> snap->xip. That was to avoid repeating this work for transactions
> that had already finished.
Yeah, maybe this approach isn't great after all. We could turn that
around and search for each loop around the snap->xmin..snap->xmax loop
that is found in snap->xip in the running->xids array. That reduces the
number of times we go through XactLockTableWait() to only running
transactions (same as in Rui's original patch [1]). However, the
running->xids array is not sorted, so we would have to qsort() it, or do
a plain array walk for each element. In the end, I think the code in
your (Rui's) first patch is the simplest approach.
It's possible that there's a slight performance difference between
scanning the running->xids array with bsearch() on snap->xip, versus
scanning the snap->xip array with bsearch on running->xids. However,
given the amount of code involved in the XactLockTableWait() that we
have to do on each item we find still running, I expect the difference
to be negligible. And doing it certainly beats ending up with corrupt
data anyway. So I'm going to take the code mostly from Rui's original
patch[1].
[1] https://postgr.es/m/CAHWVJhHXyLtS-8mdL9WhEWfsERb=FN7JdPD0GYAXgTmCnqbYGw@mail.gmail.com
However, the situation with comments is not completely settled for me.
I asked:
> On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> > I don't understand [this comment]:
> >
> > * A subtransaction is covered by its top-level transaction, which is in
> > * snap->xip as well, or was purged from it because it is below xmin and
> > * thus finished long ago.
and you said:
> The first was meant to explain why we don't have to find every
> subxid in the running-XID list. If any backend's subxid cache has
> overflowed, GetRunningTransactionData() returns top-level XIDs but no
> subxids. We still wait for the parent, which covers its children.
However, the code scans running->xids with a limit of
+ nrunning = running->xcnt + running->subxcnt;
which means we scan both main Xids as well as subxids, which seems to
contradict what you said. I think we should just go up to running->xcnt
only; if any subxids are in there, we can ignore that, because we'd
still do the XactLockTableWait with the parent xact. (We know, by
construction, that the array has the top-level XIDs first, followed by
subxids. This doesn't seem documented anywhere though. Perhaps if this
is ever broken, SnapBuildWaitSnapshot would be trouble. Maybe worth
adding a comment somewhere.)
I also asked:
> On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> > I don't understand [this other comment]:
> >
> > * Historic snapshots do not need this: between xmin and xmax they rely on
> > * xip alone, and transactions below xmin had left the procarray by the
> > * time the xl_running_xacts record that set xmin was written.
and you replied:
> The second was a different question: why wait only in
> SnapBuildInitialSnapshot(), rather than in SnapBuildBuildSnapshot(),
> which is also used to build historic snapshots? Here "this" meant
> waiting for transactions to finish, not handling subtransactions.
>
> Historic snapshots use xip for committed-XID checks in [xmin, xmax).
> They can consult CLOG below xmin, but those transactions had already
> finished when the running-xacts record supplying xmin was written.
> So they need no extra wait.
Ah, I see. It makes sense when explained like that, but I find it
difficult to understand in the broader context of the comment being
added. I don't disagree that this is worth commenting about, but I'm
not sure this is the best place to do it. Rather, maybe we should add
something in SnapBuildBuildSnapshot() to explain why we don't do this
there.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-21 15:35 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-21 16:56 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-23 17:33 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
0 siblings, 1 reply; 38+ messages in thread
From: Alvaro Herrera @ 2026-09-21 16:56 UTC (permalink / raw)
To: Rui Zhao <zhaorui126@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-21, Alvaro Herrera wrote:
> Yeah, maybe this approach isn't great after all. We could turn that
> around and search for each loop around the snap->xmin..snap->xmax loop
> that is found in snap->xip in the running->xids array. That reduces the
> number of times we go through XactLockTableWait() to only running
> transactions (same as in Rui's original patch [1]). However, the
> running->xids array is not sorted, so we would have to qsort() it, or do
> a plain array walk for each element.
BTW just to make it clear what I'm talking about, attached is the patch
for this approach, which I'm now thinking to throw away in favor of
Rui's earlier version. (The test contains the change Rui suggested
yesterday, and it's passing for me with this patch.) I didn't touch the
commit message.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
Attachments:
[text/x-diff] v6-0001-Wait-for-the-transactions-of-an-initial-decoding-.patch (3.9K, ../../arFgusAC7MFhUaAh@alvherre.pgsql/2-v6-0001-Wait-for-the-transactions-of-an-initial-decoding-.patch)
download | inline diff:
From 91f4c1fda343562f555fc473f925966b97dc7b12 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:00:27 +0800
Subject: [PATCH v6 1/3] Wait for the transactions of an initial decoding
snapshot to finish
SnapBuildInitialSnapshot() converts the snapshot builder's list of
committed transactions into a regular MVCC snapshot, which is then used
with HeapTupleSatisfiesMVCC(). That function consults CLOG about the
transactions the snapshot takes as not running, so each of them has to
have finished committing before the snapshot is handed out: the commit
record is written first, CLOG is updated afterwards, and the transaction
stays in the procarray until after that.
Read the set of running transactions once, and wait on the transaction
lock of those that are in the snapshot's list, as SnapBuildWaitSnapshot()
does in the same code path; the others have left the procarray and so
have updated CLOG. Historic snapshots built by SnapBuildBuildSnapshot()
need no such wait: they rely on the xip array for transactions between
xmin and xmax, and consult CLOG only for transactions below xmin, which
had left the procarray when the xl_running_xacts record that set xmin was
written.
---
src/backend/replication/logical/snapbuild.c | 33 +++++++++++++++++++++
src/backend/storage/ipc/procarray.c | 4 +--
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index de491ea0c4b..5c51c65bce4 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -448,6 +448,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
TransactionId safeXid;
TransactionId *newxip;
int newxcnt = 0;
+ RunningTransactions running = NULL;
Assert(XactIsoLevel == XACT_REPEATABLE_READ);
Assert(builder->building_full_snapshot);
@@ -493,6 +494,17 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
/* allocate in transaction context */
newxip = palloc_array(TransactionId, GetMaxSnapshotXidCount());
+ /*
+ * Avoid excessive traffic through TransactionIdIsInProgress() below by
+ * acquiring the list of running transactions once.
+ */
+ if (!RecoveryInProgress())
+ {
+ running = GetRunningTransactionData();
+ LWLockRelease(XidGenLock);
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* snapbuild.c builds transactions in an "inverted" manner, which means it
* stores committed transactions in ->xip, not ones in progress. Build a
@@ -520,6 +532,27 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
newxip[newxcnt++] = xid;
}
+ /*
+ * If a transaction is in the snapshot and reported as committed, we
+ * don't yet know for certain that the transaction was removed from
+ * procarray as opposed to merely got its WAL commit record written.
+ * For correctness reasons (involving hint-bit setting) we must not
+ * allow transactions in the latter state be reported as committed, so
+ * wait for them to end.
+ */
+ if (!RecoveryInProgress() && test != NULL)
+ {
+ /* XXX We could qsort() and bsearch() this array ... */
+ for (int i = 0; i < running->xcnt; i++)
+ {
+ if (xid == running->xids[i])
+ {
+ XactLockTableWait(xid, NULL, NULL, XLTW_None);
+ break;
+ }
+ }
+ }
+
TransactionIdAdvance(xid);
}
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index b7e03134ed8..fd678bea3b6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2663,8 +2663,8 @@ GetRunningTransactionData(void)
* the lock, so we can't look at numProcs. Likewise, we allocate much
* more subxip storage than is probably needed.
*
- * Should only be allocated in bgwriter, since only ever executed during
- * checkpoints.
+ * This is only called during checkpoint and during initial logical
+ * decoding snapshot build, so the impact is limited.
*/
if (CurrentRunningXacts->xids == NULL)
{
--
2.47.3
[text/x-diff] v6-0002-Test-the-initial-decoding-snapshot-against-a-comm.patch (8.5K, ../../arFgusAC7MFhUaAh@alvherre.pgsql/3-v6-0002-Test-the-initial-decoding-snapshot-against-a-comm.patch)
download | inline diff:
From e5a4b42698e064fa7d0afb1e3346d619fdbaccd1 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sun, 13 Sep 2026 00:59:09 +0800
Subject: [PATCH v6 2/3] Test the initial decoding snapshot against a commit
that is not in CLOG yet
The snapshot builder counts a transaction as committed once it has decoded
its commit record, but the transaction updates CLOG only after writing that
record. An initial snapshot built in between and converted to a regular
MVCC snapshot makes HeapTupleSatisfiesMVCC() consult CLOG about a
transaction it takes as not running, and the transaction comes out as
aborted.
Add an injection point between the flush of the commit record and the
CLOG update, and an isolation test that stops a transaction there while
REPACK (CONCURRENTLY) builds its snapshot. Without a fix the repacked
table lacks the changes of that transaction.
---
src/backend/access/transam/xact.c | 10 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/repack_commit_race.out | 64 +++++++++++++
.../injection_points/injection_points.c | 11 ++-
src/test/modules/injection_points/meson.build | 1 +
.../specs/repack_commit_race.spec | 96 +++++++++++++++++++
6 files changed, 180 insertions(+), 3 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/repack_commit_race.out
create mode 100644 src/test/modules/injection_points/specs/repack_commit_race.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index ebb010853cf..7b67db514ec 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -1377,6 +1378,9 @@ RecordTransactionCommit(void)
&RelcacheInitFileInval);
wrote_xlog = (XactLastRecEnd != 0);
+ /* Load the injection point before entering the critical section */
+ INJECTION_POINT_LOAD("commit-before-clog-update");
+
/*
* If we haven't been assigned an XID yet, we neither can, nor do we want
* to write a COMMIT record.
@@ -1543,6 +1547,12 @@ RecordTransactionCommit(void)
{
XLogFlush(XactLastRecEnd);
+ /*
+ * The commit record is on disk, but not in CLOG yet. A test can stop
+ * here to see what others make of the transaction meanwhile.
+ */
+ INJECTION_POINT_CACHED("commit-before-clog-update", NULL);
+
/*
* Now we may update the CLOG, if we wrote a COMMIT record above
*/
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 9d8b4b3540c..1c680abf7dd 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -18,6 +18,7 @@ ISOLATION = basic \
inplace \
reindex_concurrently_deferred \
repack \
+ repack_commit_race \
repack_decode \
repack_temporal \
repack_temporal_multirange \
diff --git a/src/test/modules/injection_points/expected/repack_commit_race.out b/src/test/modules/injection_points/expected/repack_commit_race.out
new file mode 100644
index 00000000000..4d0286029a5
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_commit_race.out
@@ -0,0 +1,64 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s2_begin s1_repack s3_begin s2_rollback s4_changes s3_rollback s5_wakeup s1_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s2_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s1_repack:
+ REPACK (CONCURRENTLY) repack_race;
+ <waiting ...>
+step s3_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+
+?column?
+--------
+t
+(1 row)
+
+step s2_rollback:
+ ROLLBACK;
+
+step s4_changes:
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+ <waiting ...>
+step s3_rollback:
+ ROLLBACK;
+
+step s5_wakeup:
+ SELECT injection_points_wakeup('commit-before-clog-update');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s1_repack: <... completed>
+step s4_changes: <... completed>
+step s1_check:
+ SELECT i, j FROM repack_race ORDER BY i;
+
+i|j
+-+-
+1|2
+3|3
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 66d8158d0c2..5e1bbc2f5c9 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -246,12 +246,17 @@ injection_wait(const char *name, const void *private_data, void *arg)
char *argstr = arg;
int delay_us = 0;
- if (inj_state == NULL)
- injection_init_shmem();
-
+ /*
+ * Check the condition before attaching to the shared state: attaching
+ * allocates memory, which a process that is not meant to wait here must
+ * not do if the injection point is in a critical section.
+ */
if (!injection_point_allowed(condition, argstr))
return;
+ if (inj_state == NULL)
+ injection_init_shmem();
+
/*
* Use the injection point name for this custom wait event. Note that
* this custom wait event name is not released, but we don't care much for
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 80a09f34d78..45117b6ffec 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -47,6 +47,7 @@ tests += {
'inplace',
'reindex_concurrently_deferred',
'repack',
+ 'repack_commit_race',
'repack_decode',
'repack_temporal',
'repack_temporal_multirange',
diff --git a/src/test/modules/injection_points/specs/repack_commit_race.spec b/src/test/modules/injection_points/specs/repack_commit_race.spec
new file mode 100644
index 00000000000..a961d1c001d
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_commit_race.spec
@@ -0,0 +1,96 @@
+# REPACK (CONCURRENTLY) takes its initial snapshot from the logical decoding
+# snapshot builder, which counts a transaction as committed as soon as it has
+# decoded its commit record. The transaction itself may still be between
+# writing that record and updating CLOG. The snapshot must not be used before
+# the transaction has finished committing, or the copy of the table takes it
+# as aborted and its changes are lost: decoding starts after its commit record.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_race(i int PRIMARY KEY, j int);
+ INSERT INTO repack_race(i, j) VALUES (1, 1), (2, 2);
+}
+
+teardown
+{
+ DROP TABLE repack_race;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+step s1_repack
+{
+ REPACK (CONCURRENTLY) repack_race;
+}
+step s1_check
+{
+ SELECT i, j FROM repack_race ORDER BY i;
+}
+
+# s2 and s3 keep a transaction with an XID open, so that the snapshot builder
+# has to go through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states instead of
+# becoming consistent right away.
+session s2
+step s2_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s2_rollback
+{
+ ROLLBACK;
+}
+
+session s3
+step s3_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL;
+}
+step s3_rollback
+{
+ ROLLBACK;
+}
+
+# s4 changes the table and stops after writing its commit record, before
+# updating CLOG.
+session s4
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait');
+}
+step s4_changes
+{
+ INSERT INTO repack_race(i, j) VALUES (3, 3);
+ UPDATE repack_race SET j = j + 1 WHERE i = 1;
+ DELETE FROM repack_race WHERE i = 2;
+}
+teardown
+{
+ SELECT injection_points_detach('commit-before-clog-update');
+}
+
+session s5
+step s5_wakeup
+{
+ SELECT injection_points_wakeup('commit-before-clog-update');
+}
+
+# The snapshot builder waits for s2, then for s3. While it waits for s3, s4
+# writes its commit record: the builder will count s4 as committed and start
+# decoding after it, but CLOG does not know about s4 yet. REPACK must not use
+# its snapshot before s4 has finished committing.
+#
+# s4 cannot finish before s5 wakes it up, and s1 cannot finish before s4 does;
+# the marker on s4_changes keeps the reporting order stable.
+permutation
+ s2_begin
+ s1_repack
+ s3_begin
+ s2_rollback
+ s4_changes(s1_repack)
+ s3_rollback
+ s5_wakeup
+ s1_check
--
2.47.3
[text/x-diff] v6-0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-co.patch (6.5K, ../../arFgusAC7MFhUaAh@alvherre.pgsql/4-v6-0003-Test-slot-creation-with-USE_SNAPSHOT-against-a-co.patch)
download | inline diff:
From b445a316b9b1d34018d63af4f3ab8b1c38d2ff64 Mon Sep 17 00:00:00 2001
From: Rui Zhao <zhaorui126@gmail.com>
Date: Sat, 12 Sep 2026 02:26:17 +0800
Subject: [PATCH v6 3/3] Test slot creation with USE_SNAPSHOT against a commit
that is not in CLOG yet
Same scenario as the REPACK (CONCURRENTLY) isolation test, through the
replication protocol: CREATE_REPLICATION_SLOT ... USE_SNAPSHOT has to wait
for a transaction whose commit record it has decoded but which has not
updated CLOG yet, and the snapshot and later sessions must see that
transaction's changes.
---
.../recovery/t/057_snapshot_commit_race.pl | 149 ++++++++++++++++++
1 file changed, 149 insertions(+)
create mode 100644 src/test/recovery/t/057_snapshot_commit_race.pl
diff --git a/src/test/recovery/t/057_snapshot_commit_race.pl b/src/test/recovery/t/057_snapshot_commit_race.pl
new file mode 100644
index 00000000000..f2e8c7b5e88
--- /dev/null
+++ b/src/test/recovery/t/057_snapshot_commit_race.pl
@@ -0,0 +1,149 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# The snapshot of CREATE_REPLICATION_SLOT ... USE_SNAPSHOT must not be handed
+# out while a transaction it takes as committed is still between writing its
+# commit record and updating CLOG. Checks that the slot creation waits for
+# such a transaction, and that neither the slot's snapshot nor a later
+# session loses the transaction's changes.
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+$node->safe_psql('postgres',
+ q(CREATE TABLE tab(i int PRIMARY KEY, j int);
+ INSERT INTO tab VALUES (1, 1), (2, 2)));
+
+# Wait until the given backend waits on the lock of the given transaction.
+sub wait_for_xact_lock_wait
+{
+ my ($pid, $xid, $what) = @_;
+
+ $node->poll_query_until('postgres',
+ "SELECT count(*) > 0 FROM pg_locks WHERE pid = $pid AND locktype = 'transactionid' AND transactionid = '$xid' AND NOT granted"
+ ) or die "$what did not wait on the lock of transaction $xid";
+}
+
+my $s2 = $node->background_psql('postgres');
+my $s3 = $node->background_psql('postgres');
+my $s4 = $node->background_psql('postgres');
+my $walsender = $node->background_psql('postgres', replication => 'database');
+
+my $walsender_pid = $walsender->query_safe('SELECT pg_backend_pid()');
+my $s4_pid = $s4->query_safe('SELECT pg_backend_pid()');
+
+# s2 and s3 hold transactions with an XID, so that the snapshot builder goes
+# through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states rather than becoming
+# consistent right away. The walsender waits for s2, then for s3.
+my $s2_xid = $s2->query_safe('BEGIN; SELECT pg_current_xact_id()');
+
+$walsender->query_until(
+ qr/started/, q(\echo started
+BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
+CREATE_REPLICATION_SLOT slot_race TEMPORARY LOGICAL test_decoding USE_SNAPSHOT;
+));
+wait_for_xact_lock_wait($walsender_pid, $s2_xid, 'walsender');
+
+my $s3_xid = $s3->query_safe('BEGIN; SELECT pg_current_xact_id()');
+$s2->query_safe('ROLLBACK');
+wait_for_xact_lock_wait($walsender_pid, $s3_xid, 'walsender');
+
+# While the walsender waits for s3, s4 changes the table and stops after
+# writing its commit record, before updating CLOG.
+$s4->query_safe(
+ q(SELECT injection_points_set_local();
+ SELECT injection_points_attach('commit-before-clog-update', 'wait')));
+$s4->query_until(
+ qr/started/, q(\echo started
+BEGIN;
+INSERT INTO tab VALUES (3, 3);
+UPDATE tab SET j = j + 1 WHERE i = 1;
+DELETE FROM tab WHERE i = 2;
+COMMIT;
+));
+$node->poll_query_until('postgres',
+ "SELECT wait_event = 'commit-before-clog-update' FROM pg_stat_activity WHERE pid = $s4_pid"
+) or die "s4 did not reach the injection point";
+my $s4_xid = $node->safe_psql('postgres',
+ "SELECT backend_xid FROM pg_stat_activity WHERE pid = $s4_pid");
+
+# Now the walsender decodes s4's commit record and reaches a consistent
+# state. It must wait for s4 rather than build the snapshot.
+$s3->query_safe('ROLLBACK');
+my $state;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+ $state = $node->safe_psql('postgres',
+ "SELECT CASE WHEN a.state = 'idle in transaction' THEN 'slot created'
+ WHEN l.pid IS NOT NULL THEN 'waiting for s4' END
+ FROM pg_stat_activity a
+ LEFT JOIN pg_locks l ON l.pid = a.pid AND l.locktype = 'transactionid'
+ AND l.transactionid = '$s4_xid' AND NOT l.granted
+ WHERE a.pid = $walsender_pid");
+ last if $state ne '';
+ usleep(100_000);
+}
+is($state, 'waiting for s4',
+ 'slot creation waits for the transaction that has not updated CLOG');
+
+# If the slot got created without waiting, use its snapshot right away:
+# the scan takes s4 as aborted and sets hint bits accordingly, which is
+# what the last two checks then report.
+if ($state eq 'slot created')
+{
+ $walsender->query_until(qr/test_decoding/, '');
+ diag("rows seen through the slot's snapshot before s4 updated CLOG: "
+ . $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'));
+}
+
+$node->safe_psql('postgres',
+ "SELECT injection_points_wakeup('commit-before-clog-update')");
+$s4->quit;
+
+$node->poll_query_until('postgres',
+ "SELECT state = 'idle in transaction' FROM pg_stat_activity WHERE pid = $walsender_pid"
+) or die "slot creation did not finish";
+# drain the result of CREATE_REPLICATION_SLOT
+$walsender->query_until(qr/test_decoding/, '')
+ if $state ne 'slot created';
+
+is( $walsender->query_safe('SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "the slot's snapshot sees the transaction's changes");
+is( $node->safe_psql('postgres', 'SELECT i, j FROM tab ORDER BY i'),
+ "1|2\n3|3",
+ "a new session sees the transaction's changes");
+
+is( $walsender->query_safe(
+ q(WITH s AS (SELECT pg_current_snapshot() AS snap)
+ SELECT count(*) FROM s, LATERAL pg_snapshot_xip(snap) AS x(xid)
+ WHERE xid < pg_snapshot_xmin(snap) OR xid >= pg_snapshot_xmax(snap))
+ ),
+ '0',
+ 'snapshot XIDs are all within xmin and xmax');
+
+$walsender->query_safe('ROLLBACK');
+$walsender->quit;
+$s2->quit;
+$s3->quit;
+$node->stop;
+
+done_testing();
--
2.47.3
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-21 15:35 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-21 16:56 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-23 17:33 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-24 10:23 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Rui Zhao @ 2026-09-23 17:33 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Antonin Houska <ah@cybertec.at>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Thanks. Going back to my original loop with the limit changed to
running->xcnt makes sense.
On 2026-Sep-21 at 15:35 UTC, Alvaro Herrera wrote:
> I think we should just go up to running->xcnt
> only;
Yes, scanning the subxids was unnecessary. My previous explanation
addressed overflow, but waiting for the parent covers the children in
the non-overflow case too. A note on the top-level-first ordering in
RunningTransactionsData would make that dependency explicit.
> maybe we should add
> something in SnapBuildBuildSnapshot()
Agreed. The explanation of why historic snapshots need no wait for
transactions to finish belongs there. The comment in
SnapBuildInitialSnapshot() can then focus on why the conversion to a
normal MVCC snapshot needs the wait.
Regards,
Rui
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Re: Race conditions in logical decoding Andres Freund <andres@anarazel.de>
2026-03-20 15:55 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-08-21 18:16 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-09-12 17:35 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
2026-09-21 15:35 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-21 16:56 ` Re: Race conditions in logical decoding Alvaro Herrera <alvherre@kurilemu.de>
2026-09-23 17:33 ` Re: Race conditions in logical decoding Rui Zhao <zhaorui126@gmail.com>
@ 2026-09-24 10:23 ` Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 38+ messages in thread
From: Alvaro Herrera @ 2026-09-24 10:23 UTC (permalink / raw)
To: Rui Zhao <zhaorui126@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
On 2026-Sep-24, Rui Zhao wrote:
> Thanks. Going back to my original loop with the limit changed to
> running->xcnt makes sense.
OK, I have pushed this to all branches, including the test cases on
branches where they work. Thanks all for the work on this issue!
I'm going to mark the two pg19 open items (!!) as done.
> On 2026-Sep-21 at 15:35 UTC, Alvaro Herrera wrote:
> > I think we should just go up to running->xcnt
> > only;
>
> Yes, scanning the subxids was unnecessary. My previous explanation
> addressed overflow, but waiting for the parent covers the children in
> the non-overflow case too. A note on the top-level-first ordering in
> RunningTransactionsData would make that dependency explicit.
I didn't add this ... let's consider that as follow-on work, but we
don't need it to be backpatched.
> > maybe we should add
> > something in SnapBuildBuildSnapshot()
>
> Agreed. The explanation of why historic snapshots need no wait for
> transactions to finish belongs there. The comment in
> SnapBuildInitialSnapshot() can then focus on why the conversion to a
> normal MVCC snapshot needs the wait.
Done that way -- I hope the explanations are clear.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-01-22 19:59 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-23 06:33 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2026-01-22 19:59 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers@lists.postgresql.org
On 2026-Jan-20, Antonin Houska wrote:
> Antonin Houska <ah@cybertec.at> wrote:
>
> > I'm not sure yet how to fix the problem. I tried to call XactLockTableWait()
> > from SnapBuildAddCommittedTxn() (like it happens in SnapBuildWaitSnapshot()),
> > but it made at least one regression test (subscription/t/010_truncate.pl)
> > stuck - probably a deadlock. I can spend more time on it, but maybe someone
> > can come up with a good idea sooner than me.
>
> Attached here is what I consider a possible fix - simply wait for the CLOG
> update before building a new snapshot.
>
> Unfortunately I have no idea right now how to test it using the isolation
> tester. With the fix, the additional waiting makes the current test
> block. (And if a step is added that unblock the session, it will not reliably
> catch failure to wait.)
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>
> @@ -400,6 +400,47 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
> snapshot->xmin = builder->xmin;
> snapshot->xmax = builder->xmax;
>
> + /*
> + * Although it's very unlikely, it's possible that a commit WAL record was
> + * decoded but CLOG is not aware of the commit yet. Should the CLOG update
> + * be delayed even more, visibility checks that use this snapshot could
> + * work incorrectly. Therefore we check the CLOG status here.
> + */
> + while (true)
> + {
> + bool found = false;
> +
> + for (int i = 0; i < builder->committed.xcnt; i++)
> + {
> + /*
> + * XXX Is it worth remembering the XIDs that appear to be
> + * committed per CLOG and skipping them in the next iteration of
> + * the outer loop? Not sure it's worth the effort - a single
> + * iteration is enough in most cases.
> + */
> + if (unlikely(!TransactionIdDidCommit(builder->committed.xip[i])))
> + {
> + found = true;
> +
> + /*
> + * Wait a bit before going to the next iteration of the outer
> + * loop. The race conditions we address here is pretty rare,
> + * so we shouldn't need to wait too long.
> + */
> + (void) WaitLatch(MyLatch,
> + WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> + 10L,
> + WAIT_EVENT_SNAPBUILD_CLOG);
> + ResetLatch(MyLatch);
> +
> + break;
> + }
> + }
> +
> + if (!found)
> + break;
> + }
I think this algorithm is strange -- if you do have to wait more than
once for one transaction, it would lead to doing the
TransactionIdDidCommit again times for _all_ transactions by starting
the inner loop from scratch, which sounds really wasteful. Why not nest
the for() loops the other way around? Something like this perhaps,
for (int i = 0; i < builder->committed.xcnt; i++)
{
for (;;)
{
if (TransactionIdDidCommit(builder->committed.xip[i]))
break;
else
{
(void) WaitLatch(MyLatch,
WL_LATCH_SET, WL_TIMEOUT, WL_EXIT_ON_PM_DEATH,
10L,
WAIT_EVENT_SNAPBUILD_CLOG);
ResetLatch(MyLatch);
}
CHECK_FOR_INTERRUPTS();
}
}
This way you wait repeatedly for one transaction until it is marked
committed; and once it does, you don't test it again.
I also wondered if it would make sense to get rid of the memcpy, given
that we're doing so much work per xid anyway it won't make any visible
difference (I believe), and do the copy per XID there, like
if (TransactionIdDidCommit(builder->committed.xip[i]))
{
snapshot->xip[i] = builder->committed.xip[i];
break;
}
else
...
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-22 19:59 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-01-23 06:33 ` Antonin Houska <ah@cybertec.at>
2026-01-28 21:48 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 38+ messages in thread
From: Antonin Houska @ 2026-01-23 06:33 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: pgsql-hackers@lists.postgresql.org
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> I think this algorithm is strange -- if you do have to wait more than
> once for one transaction, it would lead to doing the
> TransactionIdDidCommit again times for _all_ transactions by starting
> the inner loop from scratch, which sounds really wasteful. Why not nest
> the for() loops the other way around?
I'm quite sure I wanted to iterate through committed.xnt in the outer loop,
but probably got distracted by something else and messed things up.
> Something like this perhaps,
>
> for (int i = 0; i < builder->committed.xcnt; i++)
> {
> for (;;)
> {
> if (TransactionIdDidCommit(builder->committed.xip[i]))
> break;
> else
> {
> (void) WaitLatch(MyLatch,
> WL_LATCH_SET, WL_TIMEOUT, WL_EXIT_ON_PM_DEATH,
> 10L,
> WAIT_EVENT_SNAPBUILD_CLOG);
> ResetLatch(MyLatch);
> }
> CHECK_FOR_INTERRUPTS();
> }
> }
>
> This way you wait repeatedly for one transaction until it is marked
> committed; and once it does, you don't test it again.
Sure, that's much beter. Thanks.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-22 19:59 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-01-23 06:33 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
@ 2026-01-28 21:48 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-29 19:52 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2026-01-28 21:48 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers@lists.postgresql.org
On 2026-Jan-23, Antonin Houska wrote:
> > This way you wait repeatedly for one transaction until it is marked
> > committed; and once it does, you don't test it again.
>
> Sure, that's much beter. Thanks.
Actually, I wonder if it would make sense to sleep just once after
testing all the transactions for whether they are marked committed (not
once per transaction); and after sleeping, we only test again those that
were not marked committed in the previous iteration. I think you would
end up doing less tests overall. Something like this
/*
* Although it's very unlikely, it's possible that a commit WAL record was
* decoded but CLOG is not aware of the commit yet. Should the CLOG update
* be delayed even more, visibility checks that use this snapshot could
* work incorrectly. Therefore we check the CLOG status here.
*/
{
TransactionId *stillrunning;
int nstillrunning = builder->committed.xcnt;
stillrunning = palloc(sizeof(TransactionId) * builder->committed.xcnt);
nstillrunning = builder->committed.xcnt;
memcpy(stillrunning, builder->committed.xip, sizeof(TransactionId) * nstillrunning);
for (;;)
{
int next = 0;
if (nstillrunning == 0)
break;
for (int i = 0; i < nstillrunning; i++)
{
if (!TransactionIdDidCommit(stillrunning[i]))
stillrunning[next++] = stillrunning[i];
}
if (next == 0)
break;
nstillrunning = next;
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
10L,
WAIT_EVENT_SNAPBUILD_CLOG);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
}
pfree(stillrunning);
}
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Race conditions in logical decoding
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-22 19:59 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
2026-01-23 06:33 ` Re: Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-28 21:48 ` Re: Race conditions in logical decoding Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-01-29 19:52 ` Antonin Houska <ah@cybertec.at>
0 siblings, 0 replies; 38+ messages in thread
From: Antonin Houska @ 2026-01-29 19:52 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: pgsql-hackers@lists.postgresql.org
Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Jan-23, Antonin Houska wrote:
>
> > > This way you wait repeatedly for one transaction until it is marked
> > > committed; and once it does, you don't test it again.
> >
> > Sure, that's much beter. Thanks.
>
> Actually, I wonder if it would make sense to sleep just once after
> testing all the transactions for whether they are marked committed (not
> once per transaction); and after sleeping, we only test again those that
> were not marked committed in the previous iteration. I think you would
> end up doing less tests overall. Something like this
I suppose that TransactionIdDidCommit() returns false pretty rarely, so one
iteration is sufficient in almost all cases. Besides that, I preferred simpler
code because it's easier to test (It's not trivial to have debugger reach the
conditions.) However it's just my preference - no real objections to your
approach.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 38+ messages in thread
end of thread, other threads:[~2026-09-24 10:23 UTC | newest]
Thread overview: 38+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-01-19 16:29 Race conditions in logical decoding Antonin Houska <ah@cybertec.at>
2026-01-20 08:30 ` Antonin Houska <ah@cybertec.at>
2026-01-20 17:50 ` Andres Freund <andres@anarazel.de>
2026-01-22 00:24 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-01-22 10:32 ` Antonin Houska <ah@cybertec.at>
2026-01-22 18:58 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-22 19:50 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-23 08:02 ` Antonin Houska <ah@cybertec.at>
2026-03-20 15:55 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-03-23 09:58 ` Antonin Houska <ah@cybertec.at>
2026-06-03 16:37 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-08-19 13:15 ` Antonin Houska <ah@cybertec.at>
2026-08-21 18:16 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-08-22 16:22 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-09 10:20 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-09 18:08 ` Antonin Houska <ah@cybertec.at>
2026-09-16 07:59 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-17 09:52 ` Antonin Houska <ah@cybertec.at>
2026-09-17 10:10 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-08-25 17:25 ` Antonin Houska <ah@cybertec.at>
2026-09-10 23:49 ` Noah Misch <noah@leadboat.com>
2026-09-11 09:03 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-11 09:43 ` Antonin Houska <ah@cybertec.at>
2026-09-12 17:35 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-17 08:37 ` Antonin Houska <ah@cybertec.at>
2026-09-18 12:28 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 13:16 ` Antonin Houska <ah@cybertec.at>
2026-09-18 14:29 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-18 15:23 ` Antonin Houska <ah@cybertec.at>
2026-09-19 16:21 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-21 15:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-21 16:56 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-23 17:33 ` Rui Zhao <zhaorui126@gmail.com>
2026-09-24 10:23 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-01-22 19:59 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-23 06:33 ` Antonin Houska <ah@cybertec.at>
2026-01-28 21:48 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-01-29 19:52 ` Antonin Houska <ah@cybertec.at>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox