public inbox for [email protected]
help / color / mirror / Atom feedFrom: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Amit Kapila' <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: Fix 035_standby_logical_decoding.pl race conditions
Date: Thu, 3 Apr 2025 05:34:10 +0000
Message-ID: <OSCPR01MB14966608F78B5BFC907CF8E82F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAA4eK1Krrie2X40qdj9Qu8D109DjjqS4nH-woJCheBtgdNyR_w@mail.gmail.com>
References: <OS7PR01MB14968CC605A5FCDD678AD7661F5A62@OS7PR01MB14968.jpnprd01.prod.outlook.com>
<CAA4eK1+bC-Qx7nrxxVUp8efKF5b549bfEKm3QiGnjmOY5nKLqQ@mail.gmail.com>
<OSCPR01MB1496683CB3BD0B4ACD1BADAAFF5A02@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966351EDCB504D63A2D41D0F5AC2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAA4eK1+x5-eOn5+MW6FiUjB_1bBCH8jCCARC1uMrx6erZ3J73w@mail.gmail.com>
<OSCPR01MB14966755BC3C534A0058EA07FF5AF2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<CAA4eK1KvnWZtXh0p8MOnXdz7ts1iHJ0yjb-F3ejk3mGLV6v__A@mail.gmail.com>
<OSCPR01MB1496631866E979BD510BC5448F5AF2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<Z+1RJElgG/[email protected]>
<CAA4eK1Krrie2X40qdj9Qu8D109DjjqS4nH-woJCheBtgdNyR_w@mail.gmail.com>
Dear Bertrand, Amit,
> > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > keep the slots active and only avoid doing the checks for them (they are
> invalidated
> > that's fine, they are not that's fine too).
> >
>
> I don't mind doing that, but there is no benefit in making slots
> active unless we can validate them. And we will end up adding some
> more checks, as in function check_slots_conflict_reason without any
> advantage. I feel Kuroda-San's second patch is simple, and we have
> fewer chances to make mistakes and easy to maintain in the future as
> well.
I have concerns for Bertrand's patch that it could introduce another timing
issue. E.g., if the activated slots are not invalidated, dropping slots is keep
being activated so the dropping might be fail. I did not reproduce this but
something like this can happen if we activate slots.
Attached patch has a conclusion of these discussions, slots are created but
it seldomly be activated.
Naming of patches are bit different, but please ignore...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v5-PG16-0001-Stabilize-035_standby_logical_decoding.pl.patch (5.8K, ../OSCPR01MB14966608F78B5BFC907CF8E82F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v5-PG16-0001-Stabilize-035_standby_logical_decoding.pl.patch)
download | inline diff:
From c69b5b2d0b53c28ac99705b2c1507be24658104b Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Wed, 26 Mar 2025 19:03:50 +0900
Subject: [PATCH v5-PG16] Stabilize 035_standby_logical_decoding.pl
This test tries to invalidate slots on standby server, by running VACUUM on
primary and discarding needed tuples for slots. The problem is that
xl_running_xacts records are sotimetimes generated while testing, it advances
the catalog_xmin so that the invalidation might not happen in some cases.
The fix is to skip using the active slots for some testcases.
---
.../t/035_standby_logical_decoding.pl | 41 +++++++++++--------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index 8120dfc2132..1cf58f453f5 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -205,9 +205,6 @@ sub reactive_slots_change_hfs_and_wait_for_xmins
change_hot_standby_feedback_and_wait_for_xmins($hsf, $invalidated);
- $handle =
- make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
-
# reset stat: easier to check for confl_active_logicalslot in pg_stat_database_conflicts
$node_standby->psql('testdb', q[select pg_stat_reset();]);
}
@@ -215,7 +212,7 @@ sub reactive_slots_change_hfs_and_wait_for_xmins
# Check invalidation in the logfile and in pg_stat_database_conflicts
sub check_for_invalidation
{
- my ($slot_prefix, $log_start, $test_name) = @_;
+ my ($slot_prefix, $log_start, $test_name, $checks_active_slot) = @_;
my $active_slot = $slot_prefix . 'activeslot';
my $inactive_slot = $slot_prefix . 'inactiveslot';
@@ -231,13 +228,17 @@ sub check_for_invalidation
$log_start),
"activeslot slot invalidation is logged $test_name");
- # Verify that pg_stat_database_conflicts.confl_active_logicalslot has been updated
- ok( $node_standby->poll_query_until(
- 'postgres',
- "select (confl_active_logicalslot = 1) from pg_stat_database_conflicts where datname = 'testdb'",
- 't'),
- 'confl_active_logicalslot updated'
- ) or die "Timed out waiting confl_active_logicalslot to be updated";
+ if ($checks_active_slot)
+ {
+ # Verify that pg_stat_database_conflicts.confl_active_logicalslot has
+ # been updated
+ ok( $node_standby->poll_query_until(
+ 'postgres',
+ "select (confl_active_logicalslot = 1) from pg_stat_database_conflicts where datname = 'testdb'",
+ 't'),
+ 'confl_active_logicalslot updated'
+ ) or die "Timed out waiting confl_active_logicalslot to be updated";
+ }
}
# Launch $sql query, wait for a new snapshot that has a newer horizon and
@@ -250,7 +251,8 @@ sub check_for_invalidation
# seeing a xl_running_xacts that would advance an active replication slot's
# catalog_xmin. Advancing the active replication slot's catalog_xmin
# would break some tests that expect the active slot to conflict with
-# the catalog xmin horizon.
+# the catalog xmin horizon. We ensure that replication slots are not activated
+# for tests that might produce this race condition though.
sub wait_until_vacuum_can_remove
{
my ($vac_option, $sql, $to_vac) = @_;
@@ -550,7 +552,7 @@ wait_until_vacuum_can_remove(
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class');
+check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class', 0);
# Verify slots are reported as conflicting in pg_replication_slots
check_slots_conflicting_status(1);
@@ -632,7 +634,7 @@ wait_until_vacuum_can_remove(
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class');
+check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class', 0);
# Verify slots are reported as conflicting in pg_replication_slots
check_slots_conflicting_status(1);
@@ -668,7 +670,7 @@ $node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
check_for_invalidation('shared_row_removal_', $logstart,
- 'with vacuum on pg_authid');
+ 'with vacuum on pg_authid', 0);
# Verify slots are reported as conflicting in pg_replication_slots
check_slots_conflicting_status(1);
@@ -692,6 +694,11 @@ $logstart = -s $node_standby->logfile;
reactive_slots_change_hfs_and_wait_for_xmins('shared_row_removal_',
'no_conflict_', 0, 1);
+# This scenario won't produce the race condition by a xl_running_xacts, so
+# activate the slot. See comments atop wait_until_vacuum_can_remove().
+make_slot_active($node_standby, 'no_conflict_', 1, \$stdout,
+ \$stderr);
+
# This should not trigger a conflict
wait_until_vacuum_can_remove(
'', 'CREATE TABLE conflict_test(x integer, y text);
@@ -754,7 +761,7 @@ $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'E';]);
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('pruning_', $logstart, 'with on-access pruning');
+check_for_invalidation('pruning_', $logstart, 'with on-access pruning', 0);
# Verify slots are reported as conflicting in pg_replication_slots
check_slots_conflicting_status(1);
@@ -798,7 +805,7 @@ $node_primary->restart;
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('wal_level_', $logstart, 'due to wal_level');
+check_for_invalidation('wal_level_', $logstart, 'due to wal_level', 1);
# Verify slots are reported as conflicting in pg_replication_slots
check_slots_conflicting_status(1);
--
2.43.5
[application/octet-stream] v5-PG17-0001-Stabilize-035_standby_logical_decoding.pl.patch (6.9K, ../OSCPR01MB14966608F78B5BFC907CF8E82F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v5-PG17-0001-Stabilize-035_standby_logical_decoding.pl.patch)
download | inline diff:
From 5dce899cb4856908ea41a8817fa71135b505c0c2 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Wed, 26 Mar 2025 19:03:50 +0900
Subject: [PATCH v5-PG17] Stabilize 035_standby_logical_decoding.pl
This test tries to invalidate slots on standby server, by running VACUUM on
primary and discarding needed tuples for slots. The problem is that
xl_running_xacts records are sotimetimes generated while testing, it advances
the catalog_xmin so that the invalidation might not happen in some cases.
The fix is to skip activating slots for some testcases.
---
.../t/035_standby_logical_decoding.pl | 53 +++++++++----------
1 file changed, 24 insertions(+), 29 deletions(-)
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index aeb79f51e71..752e31960ea 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -205,9 +205,6 @@ sub reactive_slots_change_hfs_and_wait_for_xmins
change_hot_standby_feedback_and_wait_for_xmins($hsf, $invalidated);
- $handle =
- make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
-
# reset stat: easier to check for confl_active_logicalslot in pg_stat_database_conflicts
$node_standby->psql('testdb', q[select pg_stat_reset();]);
}
@@ -215,7 +212,7 @@ sub reactive_slots_change_hfs_and_wait_for_xmins
# Check invalidation in the logfile and in pg_stat_database_conflicts
sub check_for_invalidation
{
- my ($slot_prefix, $log_start, $test_name) = @_;
+ my ($slot_prefix, $log_start, $test_name, $checks_active_slot) = @_;
my $active_slot = $slot_prefix . 'activeslot';
my $inactive_slot = $slot_prefix . 'inactiveslot';
@@ -231,13 +228,17 @@ sub check_for_invalidation
$log_start),
"activeslot slot invalidation is logged $test_name");
- # Verify that pg_stat_database_conflicts.confl_active_logicalslot has been updated
- ok( $node_standby->poll_query_until(
- 'postgres',
- "select (confl_active_logicalslot = 1) from pg_stat_database_conflicts where datname = 'testdb'",
- 't'),
- 'confl_active_logicalslot updated'
- ) or die "Timed out waiting confl_active_logicalslot to be updated";
+ if ($checks_active_slot)
+ {
+ # Verify that pg_stat_database_conflicts.confl_active_logicalslot has
+ # been updated
+ ok( $node_standby->poll_query_until(
+ 'postgres',
+ "select (confl_active_logicalslot = 1) from pg_stat_database_conflicts where datname = 'testdb'",
+ 't'),
+ 'confl_active_logicalslot updated'
+ ) or die "Timed out waiting confl_active_logicalslot to be updated";
+ }
}
# Launch $sql query, wait for a new snapshot that has a newer horizon and
@@ -250,7 +251,8 @@ sub check_for_invalidation
# seeing a xl_running_xacts that would advance an active replication slot's
# catalog_xmin. Advancing the active replication slot's catalog_xmin
# would break some tests that expect the active slot to conflict with
-# the catalog xmin horizon.
+# the catalog xmin horizon. We ensure that replication slots are not activated
+# for tests that might produce this race condition though.
sub wait_until_vacuum_can_remove
{
my ($vac_option, $sql, $to_vac) = @_;
@@ -550,10 +552,6 @@ reactive_slots_change_hfs_and_wait_for_xmins('behaves_ok_', 'vacuum_full_',
$node_primary->safe_psql('testdb',
qq[INSERT INTO decoding_test(x,y) SELECT 100,'100';]);
-$node_standby->poll_query_until('testdb',
- qq[SELECT total_txns > 0 FROM pg_stat_replication_slots WHERE slot_name = 'vacuum_full_activeslot']
-) or die "replication slot stats of vacuum_full_activeslot not updated";
-
# This should trigger the conflict
wait_until_vacuum_can_remove(
'full', 'CREATE TABLE conflict_test(x integer, y text);
@@ -562,19 +560,11 @@ wait_until_vacuum_can_remove(
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class');
+check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class', 0);
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('vacuum_full_', 'rows_removed');
-# Ensure that replication slot stats are not removed after invalidation.
-is( $node_standby->safe_psql(
- 'testdb',
- qq[SELECT total_txns > 0 FROM pg_stat_replication_slots WHERE slot_name = 'vacuum_full_activeslot']
- ),
- 't',
- 'replication slot stats not removed after invalidation');
-
$handle =
make_slot_active($node_standby, 'vacuum_full_', 0, \$stdout, \$stderr);
@@ -651,7 +641,7 @@ wait_until_vacuum_can_remove(
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class');
+check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class', 0);
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('row_removal_', 'rows_removed');
@@ -687,7 +677,7 @@ $node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
check_for_invalidation('shared_row_removal_', $logstart,
- 'with vacuum on pg_authid');
+ 'with vacuum on pg_authid', 0);
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('shared_row_removal_', 'rows_removed');
@@ -711,6 +701,11 @@ $logstart = -s $node_standby->logfile;
reactive_slots_change_hfs_and_wait_for_xmins('shared_row_removal_',
'no_conflict_', 0, 1);
+# This scenario won't produce the race condition by a xl_running_xacts, so
+# activate the slot. See comments atop wait_until_vacuum_can_remove().
+make_slot_active($node_standby, 'no_conflict_', 1, \$stdout,
+ \$stderr);
+
# This should not trigger a conflict
wait_until_vacuum_can_remove(
'', 'CREATE TABLE conflict_test(x integer, y text);
@@ -779,7 +774,7 @@ $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'E';]);
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('pruning_', $logstart, 'with on-access pruning');
+check_for_invalidation('pruning_', $logstart, 'with on-access pruning', 0);
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('pruning_', 'rows_removed');
@@ -823,7 +818,7 @@ $node_primary->restart;
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
-check_for_invalidation('wal_level_', $logstart, 'due to wal_level');
+check_for_invalidation('wal_level_', $logstart, 'due to wal_level', 1);
# Verify reason for conflict is 'wal_level_insufficient' in pg_replication_slots
check_slots_conflict_reason('wal_level_', 'wal_level_insufficient');
--
2.43.5
[application/octet-stream] 0001-Fix-invalid-referring-of-hash-ref-for-replication-sl.patch (2.4K, ../OSCPR01MB14966608F78B5BFC907CF8E82F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com/4-0001-Fix-invalid-referring-of-hash-ref-for-replication-sl.patch)
download | inline diff:
From 47249c139c7fe7671d02657c6fb5f9bed128af14 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Thu, 3 Apr 2025 12:12:12 +0900
Subject: [PATCH] Fix invalid referring of hash-ref for replication slots
hash-ref gerenated by slot() did not have key 'slot_name', but some codes
referred it. Fix it by referring 'plugin' instead.
---
src/test/recovery/t/006_logical_decoding.pl | 8 ++++----
src/test/recovery/t/010_logical_decoding_timelines.pl | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index a5678bc4dc4..2137c4e5e30 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -161,8 +161,8 @@ SKIP:
is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
3, 'dropping a DB with active logical slots fails');
$pg_recvlogical->kill_kill;
- is($node_primary->slot('otherdb_slot')->{'slot_name'},
- undef, 'logical slot still exists');
+ is($node_primary->slot('otherdb_slot')->{'plugin'},
+ 'test_decoding', 'logical slot still exists');
}
$node_primary->poll_query_until('otherdb',
@@ -171,8 +171,8 @@ $node_primary->poll_query_until('otherdb',
is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
0, 'dropping a DB with inactive logical slots succeeds');
-is($node_primary->slot('otherdb_slot')->{'slot_name'},
- undef, 'logical slot was actually dropped with DB');
+is($node_primary->slot('otherdb_slot')->{'plugin'},
+ '', 'logical slot was actually dropped with DB');
# Test logical slot advancing and its durability.
# Passing failover=true (last arg) should not have any impact on advancing.
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl
index 08615f1fca8..0199ae95abf 100644
--- a/src/test/recovery/t/010_logical_decoding_timelines.pl
+++ b/src/test/recovery/t/010_logical_decoding_timelines.pl
@@ -94,8 +94,8 @@ is( $node_replica->safe_psql(
'postgres', q[SELECT 1 FROM pg_database WHERE datname = 'dropme']),
'',
'dropped DB dropme on standby');
-is($node_primary->slot('dropme_slot')->{'slot_name'},
- undef, 'logical slot was actually dropped on standby');
+is($node_primary->slot('dropme_slot')->{'plugin'},
+ '', 'logical slot was actually dropped on standby');
# Back to testing failover...
$node_primary->safe_psql('postgres',
--
2.43.5
view thread (63+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: RE: Fix 035_standby_logical_decoding.pl race conditions
In-Reply-To: <OSCPR01MB14966608F78B5BFC907CF8E82F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox