From bf2cfb8c410da60128aa7a3c082cd2bb393afa16 Mon Sep 17 00:00:00 2001 From: Nisha Moond Date: Tue, 27 May 2025 17:05:02 +0530 Subject: [PATCH v2 2/2] Add a race condition test An injection point has been added to ensure that the conflict-relevant is not prematurely removed when a concurrent prepared transaction is being committed on the publisher. --- src/backend/access/transam/twophase.c | 6 + src/test/subscription/Makefile | 4 +- src/test/subscription/meson.build | 6 +- .../t/036_confl_after_delay_chkpt.pl | 186 ++++++++++++++++++ 4 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 src/test/subscription/t/036_confl_after_delay_chkpt.pl diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 06a41ec70c8..39faf032b06 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -103,6 +103,7 @@ #include "storage/proc.h" #include "storage/procarray.h" #include "utils/builtins.h" +#include "utils/injection_point.h" #include "utils/memutils.h" #include "utils/timestamp.h" @@ -2332,12 +2333,17 @@ RecordTransactionCommitPrepared(TransactionId xid, replorigin = (replorigin_session_origin != InvalidRepOriginId && replorigin_session_origin != DoNotReplicateId); + /* Load the injection point before entering the critical section */ + INJECTION_POINT_LOAD("commit-after-delay-checkpoint"); + START_CRIT_SECTION(); /* See notes in RecordTransactionCommit */ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_IN_COMMIT) == 0); MyProc->delayChkptFlags |= DELAY_CHKPT_IN_COMMIT; + INJECTION_POINT_CACHED("commit-after-delay-checkpoint", NULL); + /* * Ensures the DELAY_CHKPT_IN_COMMIT flag write is globally visible before * commit time is written. diff --git a/src/test/subscription/Makefile b/src/test/subscription/Makefile index 50b65d8f6ea..9d97e7d5c0d 100644 --- a/src/test/subscription/Makefile +++ b/src/test/subscription/Makefile @@ -13,9 +13,11 @@ subdir = src/test/subscription top_builddir = ../../.. include $(top_builddir)/src/Makefile.global -EXTRA_INSTALL = contrib/hstore +EXTRA_INSTALL = contrib/hstore \ + src/test/modules/injection_points export with_icu +export enable_injection_points check: $(prove_check) diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build index 586ffba434e..65b8493eedc 100644 --- a/src/test/subscription/meson.build +++ b/src/test/subscription/meson.build @@ -5,7 +5,10 @@ tests += { 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), 'tap': { - 'env': {'with_icu': icu.found() ? 'yes' : 'no'}, + 'env': { + 'with_icu': icu.found() ? 'yes' : 'no', + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, 'tests': [ 't/001_rep_changes.pl', 't/002_types.pl', @@ -42,6 +45,7 @@ tests += { 't/033_run_as_table_owner.pl', 't/034_temporal.pl', 't/035_conflicts.pl', + 't/036_confl_after_delay_chkpt.pl', 't/100_bugs.pl', ], }, diff --git a/src/test/subscription/t/036_confl_after_delay_chkpt.pl b/src/test/subscription/t/036_confl_after_delay_chkpt.pl new file mode 100644 index 00000000000..d9de2a64be3 --- /dev/null +++ b/src/test/subscription/t/036_confl_after_delay_chkpt.pl @@ -0,0 +1,186 @@ +# Copyright (c) 2025, PostgreSQL Global Development Group + +# Test that publisher's transactions marked with DELAY_CHKPT_IN_COMMIT prevent +# concurrently deleted tuples on the subscriber from being removed. This test +# also acts as a safeguard to prevent developers from moving the commit +# timestamp acquisition before marking DELAY_CHKPT_IN_COMMIT in +# RecordTransactionCommitPrepared. +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# This test depends on an injection point to block the prepared transaction +# commit after marking DELAY_CHKPT_IN_COMMIT flag. +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Create a publisher node. Disable autovacuum to stabilize the tests related to +# manual VACUUM and transaction ID. +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->append_conf( + 'postgresql.conf', qq{ +autovacuum = off +track_commit_timestamp = on +max_prepared_transactions = 1 +shared_preload_libraries = 'injection_points' +}); +$node_publisher->start; + +# Check if the 'injection_points' extension 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_publisher->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + +# Create a subscriber node +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init(allows_streaming => 'logical'); +$node_subscriber->append_conf( + 'postgresql.conf', qq{ +autovacuum = off +track_commit_timestamp = on +}); +$node_subscriber->start; + +# Create a table and publication on publisher +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab (a int PRIMARY KEY, b int)"); +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tab_pub FOR TABLE tab"); + +# Insert some data +$node_publisher->safe_psql('postgres', "INSERT INTO tab VALUES (1, 1);"); + +# Create the same table on subscriber and create a subscription +my $subname = 'tab_sub'; +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab (a int PRIMARY KEY, b int)"); +$node_subscriber->safe_psql( + 'postgres', " + CREATE SUBSCRIPTION $subname + CONNECTION '$node_publisher_connstr application_name=$subname' + PUBLICATION tab_pub + WITH (retain_dead_tuples = true)"); + +# Wait for initial table sync to finish +$node_subscriber->wait_for_subscription_sync($node_publisher, $subname); + +ok( $node_subscriber->poll_query_until( + 'postgres', + "SELECT xmin IS NOT NULL from pg_replication_slots WHERE slot_name = 'pg_conflict_detection'" + ), + "the xmin value of slot 'pg_conflict_detection' is valid on subscriber"); + +# Create the injection_points extension on the publisher node and attach to the +# commit-after-delay-checkpoint injection point. +$node_publisher->safe_psql( + 'postgres', + "CREATE EXTENSION injection_points; + SELECT injection_points_attach('commit-after-delay-checkpoint', 'wait');" +); + +# Start a background session on the publisher node to perform an update and +# pause at the injection point. +my $pub_session = $node_publisher->background_psql('postgres'); +$pub_session->query_until( + qr/starting_bg_psql/, + q{ + \echo starting_bg_psql + BEGIN; + UPDATE tab SET b = 2 WHERE a = 1; + PREPARE TRANSACTION 'txn_with_later_commit_ts'; + COMMIT PREPARED 'txn_with_later_commit_ts'; + } +); + +# Confirm the update is suspended +my $result = + $node_publisher->safe_psql('postgres', 'SELECT * FROM tab WHERE a = 1'); +is($result, qq(1|1), 'publisher sees the old row'); + +# Delete the row on the subscriber. The deleted row should be retained due to a +# transaction on the publisher, which is currently marked with the +# DELAY_CHKPT_IN_COMMIT flag. +$node_subscriber->safe_psql('postgres', "DELETE FROM tab WHERE a = 1;"); + +# Get the commit timestamp for the delete +my $sub_ts = $node_subscriber->safe_psql('postgres', + "SELECT timestamp FROM pg_last_committed_xact();"); + +# Confirm that the dead tuple cannot be removed +my ($cmdret, $stdout, $stderr) = + $node_subscriber->psql('postgres', qq(VACUUM (verbose) public.tab;)); + +ok($stderr =~ qr/1 are dead but not yet removable/, + 'the deleted column is non-removable'); + +my $log_location = -s $node_subscriber->logfile; + +# Wakeup and detach the injection point on the publisher node. The prepared +# transaction should now commit. +$node_publisher->safe_psql( + 'postgres', + "SELECT injection_points_wakeup('commit-after-delay-checkpoint'); + SELECT injection_points_detach('commit-after-delay-checkpoint');" +); + +# Close the background session on the publisher node +ok($pub_session->quit, "close publisher session"); + +# Confirm that the transaction committed +$result = + $node_publisher->safe_psql('postgres', 'SELECT * FROM tab WHERE a = 1'); +is($result, qq(1|2), 'publisher sees the new row'); + +# Ensure the UPDATE is replayed on subscriber +$node_publisher->wait_for_catchup($subname); + +my $logfile = slurp_file($node_subscriber->logfile(), $log_location); +ok( $logfile =~ + qr/conflict detected on relation "public.tab": conflict=update_deleted.* +.*DETAIL:.* The row to be updated was deleted locally in transaction [0-9]+ at .* +.*Remote row \(1, 2\); replica identity \(a\)=\(1\)/, + 'update target row was deleted in tab'); + +# Remember the next transaction ID to be assigned +my $next_xid = + $node_subscriber->safe_psql('postgres', "SELECT txid_current() + 1;"); + +# Confirm that the xmin value is advanced to the latest nextXid even if there is +# one transaction on the publisher that has not committed. +ok( $node_subscriber->poll_query_until( + 'postgres', + "SELECT xmin = $next_xid from pg_replication_slots WHERE slot_name = 'pg_conflict_detection'" + ), + "the xmin value of slot 'pg_conflict_detection' is updated on subscriber" +); + +# Confirm that the dead tuple can be removed now +($cmdret, $stdout, $stderr) = + $node_subscriber->psql('postgres', qq(VACUUM (verbose) public.tab;)); + +ok($stderr =~ qr/1 removed, 0 remain, 0 are dead but not yet removable/, + 'the deleted column is removed'); + +# Get the commit timestamp for the publisher's update +my $pub_ts = $node_publisher->safe_psql('postgres', + "SELECT pg_xact_commit_timestamp(xmin) from tab where a=1;"); + +# Check that the commit timestamp for the update on the publisher is later than +# or equal to the timestamp of the local deletion, as the commit timestamp +# should be assigned after marking the DELAY_CHKPT_IN_COMMIT flag. +$result = $node_publisher->safe_psql('postgres', + "SELECT '$pub_ts'::timestamp >= '$sub_ts'::timestamp"); +is($result, qq(t), + "pub UPDATE's timestamp is later than that of sub's DELETE"); + +done_testing(); -- 2.51.0.windows.1