On Sun, Oct 4, 2026 at 8:35 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Bertrand, Sawada-san, > > > I wonder if it wouldn't make more sense to keep this after > > XactTopFullTransactionId > > is reset, as in v1? > > > > Indeed, couldn't a barrier be processed during AtCleanup_Memory(), while the > > XID is still valid, and set XLogLogicalInfoUpdatePending again after this > > call? > > Per my research the function calls MemoryContextCallResetCallbacks(), which > can > calls the arbitrary functions from core and extensions. So It's possible that > a > random callback calls CHECK_FOR_INTERRUPTS() and consumes a barrier. > > I still prefer to put after resetting the XactTopFullTransactionId. >
Thank you for the comments. I'm now convinced that we can call it after resetting the top-level XID. I was concerned about the inconsistency between the AtEOXact_LogicalCtl() calls in CommitTransaction() and CleanupTransaction(), but it's fine for them to differ, since we're holding interrupts in the former but not in the latter. Calling AtEOXact_LogicalCtl() after resetting the top-level XID would be more bullet-proof, so let's go with this approach. I've attached the updated patch. Please review it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
From c42e94aa93356b37aa6dac9f0401f5a94de2c9a9 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada <[email protected]> Date: Wed, 30 Sep 2026 20:06:20 -0700 Subject: [PATCH v3] Fix deferred XLogLogicalInfo update after a failed transaction block. Previously, a backend with an assigned XID deferred the XLogLogicalInfo update requested by a procsignal barrier, and we applied it in AbortTransaction() on abort. However, a failed transaction block keeps its XID until ROLLBACK, so a barrier absorbed while idle in that state was deferred, and CleanupTransaction() did not apply it. The next transaction then ran entirely with the stale value. If logical decoding had just been enabled, that transaction's changes were written without logical information and silently skipped by decoding. Fix by applying the pending update in CleanupTransaction() instead of AbortTransaction(). We call it after resetting the top-level XID because, unlike the commit and prepare paths, CleanupTransaction() isn't always called with interrupts held, so a barrier absorbed before the reset would be deferred again and carried into the next transaction. Patch by Sergei Patiakin, with tests added by me. Oversight in commit 67c20979ce7. Reported-by: Sergei Patiakin <[email protected]> Author: Sergei Patiakin <[email protected]> Reviewed-by: Shlok Kyal <[email protected]> Reviewed-by: Hayato Kuroda <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Bertrand Drouvot <[email protected]> Discussion: https://postgr.es/m/cane55rapenafaqxlwrvm-nc0y5gkrbfzfvahvyp89ags2sy...@mail.gmail.com Backpatch-through: 19 --- src/backend/access/transam/xact.c | 12 +++++++- src/backend/replication/logical/logicalctl.c | 6 +++- .../recovery/t/051_effective_wal_level.pl | 30 +++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 1ea972f93ce..cab99f25e1e 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -3054,7 +3054,6 @@ AbortTransaction(void) AtEOXact_PgStat(false, is_parallel_worker); AtEOXact_ApplyLauncher(false); AtEOXact_LogicalRepWorkers(false); - AtEOXact_LogicalCtl(); pgstat_report_xact_timestamp(0); } @@ -3107,6 +3106,17 @@ CleanupTransaction(void) XactTopFullTransactionId = InvalidFullTransactionId; nParallelCurrentXids = 0; + /* + * Apply any pending XLogLogicalInfo update. This must be done here + * rather than in AbortTransaction(), because a failed transaction block + * keeps its XID until ROLLBACK, so a barrier absorbed meanwhile is + * deferred. Unlike CommitTransaction() and PrepareTransaction(), we are + * not necessarily holding interrupts here, so do this after resetting the + * top-level XID; otherwise a barrier absorbed in between would be left + * pending into the next transaction. + */ + AtEOXact_LogicalCtl(); + /* * done with abort processing, set current transaction state back to * default diff --git a/src/backend/replication/logical/logicalctl.c b/src/backend/replication/logical/logicalctl.c index 642d965bd1c..6e11e889689 100644 --- a/src/backend/replication/logical/logicalctl.c +++ b/src/backend/replication/logical/logicalctl.c @@ -227,7 +227,11 @@ IsXLogLogicalInfoEnabled(void) } /* - * Reset the local cache at end of the transaction. + * Apply a pending XLogLogicalInfo update at end of the top-level transaction. + * + * This is called from CommitTransaction(), PrepareTransaction(), and + * CleanupTransaction(), which are the only places where the top-level XID is + * reset, so the next transaction always starts with the latest value. */ void AtEOXact_LogicalCtl(void) diff --git a/src/test/recovery/t/051_effective_wal_level.pl b/src/test/recovery/t/051_effective_wal_level.pl index b11690863d9..2f2719d4286 100644 --- a/src/test/recovery/t/051_effective_wal_level.pl +++ b/src/test/recovery/t/051_effective_wal_level.pl @@ -78,6 +78,36 @@ wait_for_logical_decoding_disabled($primary); test_wal_level($primary, "replica|replica", "logical decoding disabled after repack"); +# Test that a backend applies an XLogLogicalInfo update that it received +# while its transaction block was in the failed state, once the block ends. +# The failed transaction keeps its XID until ROLLBACK, so the update is +# deferred; it must not be carried over into the next transaction. +my $psql_aborted = $primary->background_psql('postgres', on_error_stop => 0); +$psql_aborted->query_safe(q[begin; select pg_current_xact_id();]); +my ($aborted_out, $aborted_ret) = $psql_aborted->query(q[select 1/0;]); +is($aborted_ret, 1, "transaction block failed"); +$psql_aborted->{stderr} = ''; + +# Enable logical decoding while the backend is idle in the failed transaction +# block. This waits for all backends to absorb the barrier. +$primary->safe_psql('postgres', + qq[select pg_create_logical_replication_slot('test_aborted_slot', 'test_decoding')] +); + +# Check the value in the transaction right after ROLLBACK. Note that both +# commands need to be sent together, as query_safe() appends an empty query +# that would run in its own transaction and apply the deferred update. +is( $psql_aborted->query_safe( + q[rollback; select current_setting('effective_wal_level');]), + 'logical', + "effective_wal_level is updated after rolling back a failed transaction block" +); +$psql_aborted->quit; + +$primary->safe_psql('postgres', + qq[select pg_drop_replication_slot('test_aborted_slot')]); +wait_for_logical_decoding_disabled($primary); + # Create a new logical slot and check that effective_wal_level must be increased # to 'logical'. $primary->safe_psql('postgres', -- 2.55.0
