v2-0002-Add-a-race-condition-test.patch

From bf2cfb8c410da60128aa7a3c082cd2bb393afa16 Mon Sep 17 00:00:00 2001
From: Nisha Moond <nisha.moond412@gmail.com>
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