public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Vitaly Davydov <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly
Date: Wed, 18 Jun 2025 19:47:09 +0300
Message-ID: <CAPpHfdv3UEUBjsLhB_CwJT0xX9LmN6U+__myYopq4KcgvCSbTg@mail.gmail.com> (raw)
In-Reply-To: <21b44-6852e000-3-a556820@87831171>
References: <1d12d2-67235980-35-19a406a0@63439497>
<CAPpHfdtp61pSt72KxxOnnV_Oj4OTAiKYCSTek0Pm87hVDod4Bg@mail.gmail.com>
<CAA4eK1Lmd-stXWrNqX9iXugjY=ahhe7zRfyq+-vN512xvF4G2w@mail.gmail.com>
<CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com>
<CAPpHfdvk-nfu5xzJyPArxWctofWPR+1L9uJ+yyWFA34n6b5rGQ@mail.gmail.com>
<CAA4eK1Ladnm40UO-YC+c7eBYqjyFQSwg0dn1w_CiaMoBTsJUDA@mail.gmail.com>
<CAPpHfduX--KKX853UwaJ8Cjo5bxbh19V_+McKQ3p9aS6b8T1Wg@mail.gmail.com>
<CAA4eK1+VbBxYY-rG4B7Jq6TsYvdjt3d2nbUrs7p9-1sP5mH+sw@mail.gmail.com>
<CAPpHfdvxqjE+RZdoOODQZAmQip6NpvSH40DuvVFWuf_00ek-yQ@mail.gmail.com>
<1d97ec-6841b600-3-b7b4780@40747246>
<CAA4eK1LvssAcp61qg957xRF0=K4xF0hfJQjQ6c9V7EGHSqraPg@mail.gmail.com>
<28c8bf-68470780-3-51b29480@89454035>
<CAPpHfdt77k3BqD=anh4b7UiRevfPOu2Hcf3rvSZXntxH+4G=Ug@mail.gmail.com>
<[email protected]>
<CAPpHfdvPpg5VUTNOqfKSNx66ij3ZJNZbf4kn6SDcPkdq9FTvng@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAPpHfdvJMe4i0_wp1tq2ZGSDKqUHKPwkfD_Y6KyJ3XQRFvc0cQ@mail.gmail.com>
<OSCPR01MB1496667BEA93302F8903EE512F570A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAPpHfdsdJ4YPTzH=cnany5_LjCNb_Z9e+Oucv53qYQHaHzJhag@mail.gmail.com>
<CALDaNm3s-jpQTe1MshsvQ8GO=TLj233JCdkQ7uZ6pwqRVpxAdw@mail.gmail.com>
<3f242e-68518380-11-78125f00@134027639>
<CAPpHfds0ScFbExsT+pnY7K0JGH6pFPHMOSfCOknC-1s7ahmRFA@mail.gmail.com>
<214ba-6852df80-9-7e0df800@134698799>
<21b44-6852e000-3-a556820@87831171>
On Wed, Jun 18, 2025 at 6:50 PM Vitaly Davydov <[email protected]> wrote:
> > I think, it is a good idea. Once we do not use the generated data, it is ok
> > just to generate WAL segments using the proposed function. I've tested this
> > function. The tests worked as expected with and without the fix. The attached
> > patch does the change.
>
> Sorry, forgot to attach the patch. It is created on the current master branch.
> It may conflict with your corrections. I hope, it could be useful.
Thank you. I've integrated this into a patch to improve these tests.
Regarding assertion failure, I've found that assert in
PhysicalConfirmReceivedLocation() conflicts with restart_lsn
previously set by ReplicationSlotReserveWal(). As I can see,
ReplicationSlotReserveWal() just picks fresh XLogCtl->RedoRecPtr lsn.
So, it doesn't seems there is a guarantee that restart_lsn never goes
backward. The commit in ReplicationSlotReserveWal() even states there
is a "chance that we have to retry". Thus, I propose to remove the
assertion introduced by ca307d5cec90.
Any objection from backpatching 0001 though 17 and pushing 0002 to the head?
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v4-0002-Remove-excess-assert-from-InvalidatePossiblyObsol.patch (1.3K, ../CAPpHfdv3UEUBjsLhB_CwJT0xX9LmN6U+__myYopq4KcgvCSbTg@mail.gmail.com/2-v4-0002-Remove-excess-assert-from-InvalidatePossiblyObsol.patch)
download | inline diff:
From a805b5fc6320069b33c3f60037f32f3e679db5cd Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Wed, 18 Jun 2025 19:34:00 +0300
Subject: [PATCH v4 2/2] Remove excess assert from
InvalidatePossiblyObsoleteSlot()
ca307d5cec90 introduced keeping WAL segments by slot's last saved restart
LSN. It also added an assertion that the slot's restart LSN never goes
backward. As stated in the ReplicationSlotReserveWal() comment, this is not
always true. Additionally, this issue has been spotted by some buildfarm
members.
Vitaly Davydov <[email protected]> proposed the fix idea.
Reported-by: Vignesh C <[email protected]>
Reported-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/CALDaNm3s-jpQTe1MshsvQ8GO%3DTLj233JCdkQ7uZ6pwqRVpxAdw%40mail.gmail.com
---
src/backend/replication/slot.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c64f020742f..c11e588d632 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1810,8 +1810,6 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
*/
SpinLockAcquire(&s->mutex);
- Assert(s->data.restart_lsn >= s->last_saved_restart_lsn);
-
restart_lsn = s->data.restart_lsn;
/* we do nothing if the slot is already invalid */
--
2.39.5 (Apple Git-154)
[application/octet-stream] v4-0001-Improve-runtime-and-output-of-tests-for-replicati.patch (6.4K, ../CAPpHfdv3UEUBjsLhB_CwJT0xX9LmN6U+__myYopq4KcgvCSbTg@mail.gmail.com/3-v4-0001-Improve-runtime-and-output-of-tests-for-replicati.patch)
download | inline diff:
From 0b28994d5780bb6ca4b381de9dd8b96ea5dda3c0 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Wed, 18 Jun 2025 19:32:05 +0300
Subject: [PATCH v4 1/2] Improve runtime and output of tests for replication
slots checkpointing.
The TAP tests that verify logical and physical replication slot behavior
during checkpoints (046_checkpoint_logical_slot.pl and
047_checkpoint_physical_slot.pl) inserted two batches of 2 million rows each,
generating approximately 520 MB of WAL. On slow machines, or when compiled
with '-DRELCACHE_FORCE_RELEASE -DCATCACHE_FORCE_RELEASE', this caused the
tests to run for 8-9 minutes and occasionally time out, as seen on the
buildfarm animal prion.
This commit modifies the mentioned tests to utilize the $node->advance_wal()
function, thereby reducing runtime. Once we do not use the generated data,
the proposed function is a good alternative, which cuts the total wall-clock
run time.
While here, remove superfluous '\n' characters from several note() calls;
these appeared literally in the build-farm logs and looked odd. Also, remove
excessive 'shared_preload_libraries' GUC from the config and add a check for
'injection_points' extension availability.
Reported-by: Alexander Lakhin <[email protected]>
Reported-by: Tom Lane <[email protected]>
Author: Alexander Korotkov <[email protected]>
Author: Vitaly Davydov <[email protected]>
Reviewed-by: Hayato Kuroda <[email protected]>
Discussion: https://postgr.es/m/fbc5d94e-6fbd-4a64-85d4-c9e284a58eb2%40gmail.com
Backpatch-through: 17
---
.../recovery/t/046_checkpoint_logical_slot.pl | 31 +++++++++----------
.../t/047_checkpoint_physical_slot.pl | 23 +++++++-------
2 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/src/test/recovery/t/046_checkpoint_logical_slot.pl b/src/test/recovery/t/046_checkpoint_logical_slot.pl
index b4265c4a6a5..d67c5108d78 100644
--- a/src/test/recovery/t/046_checkpoint_logical_slot.pl
+++ b/src/test/recovery/t/046_checkpoint_logical_slot.pl
@@ -21,15 +21,18 @@ my ($node, $result);
$node = PostgreSQL::Test::Cluster->new('mike');
$node->init;
-$node->append_conf('postgresql.conf',
- "shared_preload_libraries = 'injection_points'");
$node->append_conf('postgresql.conf', "wal_level = 'logical'");
$node->start;
-$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
-# Create a simple table to generate data into.
-$node->safe_psql('postgres',
- q{create table t (id serial primary key, b text)});
+# 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->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
# Create the two slots we'll need.
$node->safe_psql('postgres',
@@ -58,23 +61,17 @@ SELECT 1 \watch 0.1
\q
));
-# Insert 2M rows; that's about 260MB (~20 segments) worth of WAL.
-$node->safe_psql('postgres',
- q{insert into t (b) select md5(i::text) from generate_series(1,1000000) s(i)}
-);
+$node->advance_wal(20);
# Run another checkpoint to set a new restore LSN.
$node->safe_psql('postgres', q{checkpoint});
-# Another 2M rows; that's about 260MB (~20 segments) worth of WAL.
-$node->safe_psql('postgres',
- q{insert into t (b) select md5(i::text) from generate_series(1,1000000) s(i)}
-);
+$node->advance_wal(20);
# Run another checkpoint, this time in the background, and make it wait
# on the injection point) so that the checkpoint stops right before
# removing old WAL segments.
-note('starting checkpoint\n');
+note('starting checkpoint');
my $checkpoint = $node->background_psql('postgres');
$checkpoint->query_safe(
@@ -88,7 +85,7 @@ checkpoint;
));
# Wait until the checkpoint stops right before removing WAL segments.
-note('waiting for injection_point\n');
+note('waiting for injection_point');
$node->wait_for_event('checkpointer', 'checkpoint-before-old-wal-removal');
note('injection_point is reached');
@@ -107,7 +104,7 @@ select count(*) from pg_logical_slot_get_changes('slot_logical', null, null) \wa
));
# Wait until the slot's restart_lsn points to the next WAL segment.
-note('waiting for injection_point\n');
+note('waiting for injection_point');
$node->wait_for_event('client backend',
'logical-replication-slot-advance-segment');
note('injection_point is reached');
diff --git a/src/test/recovery/t/047_checkpoint_physical_slot.pl b/src/test/recovery/t/047_checkpoint_physical_slot.pl
index 454e56b9bd2..a1332b5d44c 100644
--- a/src/test/recovery/t/047_checkpoint_physical_slot.pl
+++ b/src/test/recovery/t/047_checkpoint_physical_slot.pl
@@ -21,15 +21,18 @@ my ($node, $result);
$node = PostgreSQL::Test::Cluster->new('mike');
$node->init;
-$node->append_conf('postgresql.conf',
- "shared_preload_libraries = 'injection_points'");
$node->append_conf('postgresql.conf', "wal_level = 'replica'");
$node->start;
-$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
-# Create a simple table to generate data into.
-$node->safe_psql('postgres',
- q{create table t (id serial primary key, b text)});
+# 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->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
# Create a physical replication slot.
$node->safe_psql('postgres',
@@ -44,9 +47,7 @@ $node->safe_psql('postgres',
$node->safe_psql('postgres', q{checkpoint});
# Insert 2M rows; that's about 260MB (~20 segments) worth of WAL.
-$node->safe_psql('postgres',
- q{insert into t (b) select md5(i::text) from generate_series(1,100000) s(i)}
-);
+$node->advance_wal(20);
# Advance slot to the current position, just to have everything "valid".
$node->safe_psql('postgres',
@@ -57,9 +58,7 @@ $node->safe_psql('postgres',
$node->safe_psql('postgres', q{checkpoint});
# Another 2M rows; that's about 260MB (~20 segments) worth of WAL.
-$node->safe_psql('postgres',
- q{insert into t (b) select md5(i::text) from generate_series(1,1000000) s(i)}
-);
+$node->advance_wal(20);
my $restart_lsn_init = $node->safe_psql('postgres',
q{select restart_lsn from pg_replication_slots where slot_name = 'slot_physical'}
--
2.39.5 (Apple Git-154)
view thread (73+ 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], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly
In-Reply-To: <CAPpHfdv3UEUBjsLhB_CwJT0xX9LmN6U+__myYopq4KcgvCSbTg@mail.gmail.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