From dacda92357f397354a63aa5418f9bae802af06d3 Mon Sep 17 00:00:00 2001 From: nkey Date: Sat, 23 Nov 2024 13:25:11 +0100 Subject: [PATCH v10 1/2] This patch introduces new injection points and TAP tests to reproduce and verify conflict detection issues that arise during SNAPSHOT_DIRTY index scans in logical replication and check_exclusion_or_unique_constraint. --- src/backend/access/index/indexam.c | 8 + src/backend/executor/execIndexing.c | 3 + src/test/modules/injection_points/Makefile | 2 +- .../expected/dirty_index_scan.out | 27 ++++ src/test/modules/injection_points/meson.build | 1 + .../specs/dirty_index_scan.spec | 37 +++++ src/test/subscription/Makefile | 1 + src/test/subscription/meson.build | 8 +- .../subscription/t/036_delete_missing_race.pl | 137 +++++++++++++++++ .../subscription/t/037_update_missing_race.pl | 139 +++++++++++++++++ .../t/038_update_missing_with_retain.pl | 141 ++++++++++++++++++ 11 files changed, 502 insertions(+), 2 deletions(-) create mode 100644 src/test/modules/injection_points/expected/dirty_index_scan.out create mode 100644 src/test/modules/injection_points/specs/dirty_index_scan.spec create mode 100644 src/test/subscription/t/036_delete_missing_race.pl create mode 100644 src/test/subscription/t/037_update_missing_race.pl create mode 100644 src/test/subscription/t/038_update_missing_with_retain.pl diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index 1a4f36fe0a9..2e65750979e 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -57,6 +57,7 @@ #include "utils/ruleutils.h" #include "utils/snapmgr.h" #include "utils/syscache.h" +#include "utils/injection_point.h" /* ---------------------------------------------------------------- @@ -741,6 +742,13 @@ index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot * * the index. */ Assert(ItemPointerIsValid(&scan->xs_heaptid)); +#ifdef USE_INJECTION_POINTS + if (scan->xs_snapshot->snapshot_type == SNAPSHOT_DIRTY) + { + INJECTION_POINT("index_getnext_slot_before_fetch_apply_dirty", NULL); + } +#endif + if (index_fetch_heap(scan, slot)) return true; } diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index ca33a854278..c07ba230946 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -117,6 +117,7 @@ #include "utils/multirangetypes.h" #include "utils/rangetypes.h" #include "utils/snapmgr.h" +#include "utils/injection_point.h" /* waitMode argument to check_exclusion_or_unique_constraint() */ typedef enum @@ -943,6 +944,8 @@ retry: ExecDropSingleTupleTableSlot(existing_slot); + if (!conflict) + INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict", NULL); return !conflict; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index fc82cd67f6c..15f5e6d23d0 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -14,7 +14,7 @@ PGFILEDESC = "injection_points - facility for injection points" REGRESS = injection_points hashagg reindex_conc vacuum REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress -ISOLATION = basic inplace syscache-update-pruned +ISOLATION = basic inplace syscache-update-pruned dirty_index_scan TAP_TESTS = 1 diff --git a/src/test/modules/injection_points/expected/dirty_index_scan.out b/src/test/modules/injection_points/expected/dirty_index_scan.out new file mode 100644 index 00000000000..82d46397d61 --- /dev/null +++ b/src/test/modules/injection_points/expected/dirty_index_scan.out @@ -0,0 +1,27 @@ +Parsed test spec with 3 sessions + +starting permutation: s1_s1 s2_s1 s3_s1 +injection_points_attach +----------------------- + +(1 row) + +step s1_s1: INSERT INTO test.tbl VALUES(42, 1) on conflict(i) do update set n = EXCLUDED.n + 1; +step s2_s1: UPDATE test.tbl SET n = n + 1 WHERE i = 42; +step s3_s1: + SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty'); + SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty'); + +step s1_s1: <... completed> +step s2_s1: <... completed> +step s3_s1: <... completed> +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 20390d6b4bf..a126fe20c2d 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -48,6 +48,7 @@ tests += { 'basic', 'inplace', 'syscache-update-pruned', + 'dirty_index_scan', ], 'runningcheck': false, # see syscache-update-pruned }, diff --git a/src/test/modules/injection_points/specs/dirty_index_scan.spec b/src/test/modules/injection_points/specs/dirty_index_scan.spec new file mode 100644 index 00000000000..91d20ab4612 --- /dev/null +++ b/src/test/modules/injection_points/specs/dirty_index_scan.spec @@ -0,0 +1,37 @@ +setup +{ + CREATE EXTENSION injection_points; + CREATE SCHEMA test; + CREATE UNLOGGED TABLE test.tbl(i int primary key, n int); + CREATE INDEX tbl_n_idx ON test.tbl(n); + INSERT INTO test.tbl VALUES(42,1); +} + +teardown +{ + DROP SCHEMA test CASCADE; + DROP EXTENSION injection_points; +} + +session s1 +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'error'); + SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait'); +} + +step s1_s1 { INSERT INTO test.tbl VALUES(42, 1) on conflict(i) do update set n = EXCLUDED.n + 1; } + +session s2 +step s2_s1 { UPDATE test.tbl SET n = n + 1 WHERE i = 42; } + +session s3 +step s3_s1 { + SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty'); + SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty'); +} + +permutation + s1_s1 + s2_s1(*) + s3_s1(s1_s1) \ No newline at end of file diff --git a/src/test/subscription/Makefile b/src/test/subscription/Makefile index 50b65d8f6ea..51d28eca091 100644 --- a/src/test/subscription/Makefile +++ b/src/test/subscription/Makefile @@ -16,6 +16,7 @@ include $(top_builddir)/src/Makefile.global EXTRA_INSTALL = contrib/hstore 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..49f52db4dd1 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,9 @@ tests += { 't/033_run_as_table_owner.pl', 't/034_temporal.pl', 't/035_conflicts.pl', + 't/036_delete_missing_race.pl', + 't/037_update_missing_race.pl', + 't/038_update_missing_with_retain.pl', 't/100_bugs.pl', ], }, diff --git a/src/test/subscription/t/036_delete_missing_race.pl b/src/test/subscription/t/036_delete_missing_race.pl new file mode 100644 index 00000000000..82e16af9be3 --- /dev/null +++ b/src/test/subscription/t/036_delete_missing_race.pl @@ -0,0 +1,137 @@ +# Copyright (c) 2025, PostgreSQL Global Development Group + +# Test the conflict detection and resolution in logical replication +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'; +} + +############################## Set it to 0 to make set success; TODO: delete that for commit +my $simulate_race_condition = 1; +############################## + +############################### +# Setup +############################### + +# Initialize publisher node +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_publisher->start; + + +# Create subscriber node with track_commit_timestamp enabled +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init; +$node_subscriber->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_subscriber->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_subscriber->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +# Create table on publisher +$node_publisher->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text);"); + +# Create similar table on subscriber with additional index to disable HOT updates +$node_subscriber->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text); + CREATE INDEX data_index ON conf_tab(data);"); + +# Set up extension to simulate race condition +$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# Setup logical replication +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub FOR TABLE conf_tab"); + +# Insert row to be updated later +$node_publisher->safe_psql('postgres', + "INSERT INTO conf_tab(a, data) VALUES (1,'frompub')"); + +# Create the subscription +my $appname = 'tap_sub'; +$node_subscriber->safe_psql( + 'postgres', + "CREATE SUBSCRIPTION tap_sub + CONNECTION '$publisher_connstr application_name=$appname' + PUBLICATION tap_pub"); + +# Wait for initial table sync to finish +$node_subscriber->wait_for_subscription_sync($node_publisher, $appname); + +############################################ +# Race condition because of DirtySnapshot +############################################ + +my $psql_session_subscriber = $node_subscriber->background_psql('postgres'); +if ($simulate_race_condition) +{ + $node_subscriber->safe_psql('postgres', + "SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')"); +} + +my $log_offset = -s $node_subscriber->logfile; + +# Delete tuple on publisher +$node_publisher->safe_psql('postgres', "DELETE FROM conf_tab WHERE a=1;"); + +if ($simulate_race_condition) +{ + # Wait apply worker to start the search for the tuple using index + $node_subscriber->wait_for_event('logical replication apply worker', + 'index_getnext_slot_before_fetch_apply_dirty'); +} + +# Updater tuple on subscriber +$psql_session_subscriber->query_until( + qr/start/, qq[ + \\echo start + UPDATE conf_tab SET data = 'fromsubnew' WHERE (a=1); +]); + + +if ($simulate_race_condition) +{ + # Wake up apply worker + $node_subscriber->safe_psql('postgres'," + SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty'); + SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty'); + "); +} + +# Tuple was updated - so, we have conflict +$node_subscriber->wait_for_log( + qr/conflict detected on relation \"public.conf_tab\"/, + $log_offset); + +# But tuple should be deleted on subscriber any way +is($node_subscriber->safe_psql('postgres', 'SELECT count(*) from conf_tab'), 0, 'record deleted on subscriber'); + +ok(!$node_subscriber->log_contains( + qr/LOG: conflict detected on relation \"public.conf_tab\": conflict=delete_missing/, + $log_offset), 'invalid conflict detected'); + +ok($node_subscriber->log_contains( + qr/LOG: conflict detected on relation "public.conf_tab": conflict=delete_origin_differs/, + $log_offset), 'correct conflict detected'); + +done_testing(); diff --git a/src/test/subscription/t/037_update_missing_race.pl b/src/test/subscription/t/037_update_missing_race.pl new file mode 100644 index 00000000000..e29ad771d8e --- /dev/null +++ b/src/test/subscription/t/037_update_missing_race.pl @@ -0,0 +1,139 @@ +# Copyright (c) 2025, PostgreSQL Global Development Group + +# Test the conflict detection and resolution in logical replication +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'; +} + +############################## Set it to 0 to make set success; TODO: delete that for commit +my $simulate_race_condition = 1; +############################## + +############################### +# Setup +############################### + +# Initialize publisher node +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_publisher->start; + + +# Create subscriber node with track_commit_timestamp enabled +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init; +$node_subscriber->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_subscriber->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_subscriber->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +# Create table on publisher +$node_publisher->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text);"); + +# Create similar table on subscriber with additional index to disable HOT updates and additional column +$node_subscriber->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text, i int DEFAULT 0); + CREATE INDEX i_index ON conf_tab(i);"); + +# Set up extension to simulate race condition +$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# Setup logical replication +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub FOR TABLE conf_tab"); + +# Insert row to be updated later +$node_publisher->safe_psql('postgres', + "INSERT INTO conf_tab(a, data) VALUES (1,'frompub')"); + +# Create the subscription +my $appname = 'tap_sub'; +$node_subscriber->safe_psql( + 'postgres', + "CREATE SUBSCRIPTION tap_sub + CONNECTION '$publisher_connstr application_name=$appname' + PUBLICATION tap_pub"); + +# Wait for initial table sync to finish +$node_subscriber->wait_for_subscription_sync($node_publisher, $appname); + +############################################ +# Race condition because of DirtySnapshot +############################################ + +my $psql_session_subscriber = $node_subscriber->background_psql('postgres'); +if ($simulate_race_condition) +{ + $node_subscriber->safe_psql('postgres', "SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')"); +} + +my $log_offset = -s $node_subscriber->logfile; + +# Update tuple on publisher +$node_publisher->safe_psql('postgres', + "UPDATE conf_tab SET data = 'frompubnew' WHERE (a=1);"); + + +if ($simulate_race_condition) +{ + # Wait apply worker to start the search for the tuple using index + $node_subscriber->wait_for_event('logical replication apply worker', 'index_getnext_slot_before_fetch_apply_dirty'); +} + +# Update additional(!) column on the subscriber +$psql_session_subscriber->query_until( + qr/start/, qq[ + \\echo start + UPDATE conf_tab SET i = 1 WHERE (a=1); +]); + + +if ($simulate_race_condition) +{ + # Wake up apply worker + $node_subscriber->safe_psql('postgres'," + SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty'); + SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty'); + "); +} + +# Tuple was updated - so, we have conflict +$node_subscriber->wait_for_log( + qr/conflict detected on relation \"public.conf_tab\"/, + $log_offset); + +# We need new column value be synced with subscriber +is($node_subscriber->safe_psql('postgres', 'SELECT data from conf_tab WHERE a = 1'), 'frompubnew', 'record updated on subscriber'); +# And additional column maintain updated value +is($node_subscriber->safe_psql('postgres', 'SELECT i from conf_tab WHERE a = 1'), 1, 'column record updated on subscriber'); + +ok(!$node_subscriber->log_contains( + qr/LOG: conflict detected on relation \"public.conf_tab\": conflict=update_missing/, + $log_offset), 'invalid conflict detected'); + +ok($node_subscriber->log_contains( + qr/LOG: conflict detected on relation "public.conf_tab": conflict=update_origin_differs/, + $log_offset), 'correct conflict detected'); + +done_testing(); diff --git a/src/test/subscription/t/038_update_missing_with_retain.pl b/src/test/subscription/t/038_update_missing_with_retain.pl new file mode 100644 index 00000000000..13769aa1c11 --- /dev/null +++ b/src/test/subscription/t/038_update_missing_with_retain.pl @@ -0,0 +1,141 @@ +# Copyright (c) 2025, PostgreSQL Global Development Group + +# Test the conflict detection and resolution in logical replication +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'; +} + +############################## Set it to 0 to make set success; TODO: delete that for commit +my $simulate_race_condition = 1; +############################## + +############################### +# Setup +############################### + +# Initialize publisher node +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_publisher->start; + + +# Create subscriber node with track_commit_timestamp enabled +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init; +$node_subscriber->append_conf('postgresql.conf', + qq(track_commit_timestamp = on)); +$node_subscriber->append_conf('postgresql.conf', + qq(wal_level = 'replica')); +$node_subscriber->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_subscriber->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +# Create table on publisher +$node_publisher->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text);"); + +# Create similar table on subscriber with additional index to disable HOT updates and additional column +$node_subscriber->safe_psql( + 'postgres', + "CREATE TABLE conf_tab(a int PRIMARY key, data text, i int DEFAULT 0); + CREATE INDEX i_index ON conf_tab(i);"); + +# Set up extension to simulate race condition +$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# Setup logical replication +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub FOR TABLE conf_tab"); + +# Insert row to be updated later +$node_publisher->safe_psql('postgres', + "INSERT INTO conf_tab(a, data) VALUES (1,'frompub')"); + +# Create the subscription +my $appname = 'tap_sub'; +$node_subscriber->safe_psql( + 'postgres', + "CREATE SUBSCRIPTION tap_sub + CONNECTION '$publisher_connstr application_name=$appname' + PUBLICATION tap_pub WITH (retain_dead_tuples = true)"); + +# Wait for initial table sync to finish +$node_subscriber->wait_for_subscription_sync($node_publisher, $appname); + +############################################ +# Race condition because of DirtySnapshot +############################################ + +my $psql_session_subscriber = $node_subscriber->background_psql('postgres'); +if ($simulate_race_condition) +{ + $node_subscriber->safe_psql('postgres', "SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')"); +} + +my $log_offset = -s $node_subscriber->logfile; + +# Update tuple on publisher +$node_publisher->safe_psql('postgres', + "UPDATE conf_tab SET data = 'frompubnew' WHERE (a=1);"); + + +if ($simulate_race_condition) +{ + # Wait apply worker to start the search for the tuple using index + $node_subscriber->wait_for_event('logical replication apply worker', 'index_getnext_slot_before_fetch_apply_dirty'); +} + +# Update additional(!) column on the subscriber +$psql_session_subscriber->query_until( + qr/start/, qq[ + \\echo start + UPDATE conf_tab SET i = 1 WHERE (a=1); +]); + + +if ($simulate_race_condition) +{ + # Wake up apply worker + $node_subscriber->safe_psql('postgres'," + SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty'); + SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty'); + "); +} + +# Tuple was updated - so, we have conflict +$node_subscriber->wait_for_log( + qr/conflict detected on relation \"public.conf_tab\"/, + $log_offset); + +# We need new column value be synced with subscriber +is($node_subscriber->safe_psql('postgres', 'SELECT data from conf_tab WHERE a = 1'), 'frompubnew', 'record updated on subscriber'); +# And additional column maintain updated value +is($node_subscriber->safe_psql('postgres', 'SELECT i from conf_tab WHERE a = 1'), 1, 'column record updated on subscriber'); + +ok(!$node_subscriber->log_contains( + qr/LOG: conflict detected on relation \"public.conf_tab\": conflict=update_deleted/, + $log_offset), 'invalid conflict detected'); + +ok($node_subscriber->log_contains( + qr/LOG: conflict detected on relation "public.conf_tab": conflict=update_origin_differs/, + $log_offset), 'correct conflict detected'); + +done_testing(); -- 2.43.0