public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bharath Rupireddy <[email protected]>
To: Peter Smith <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Disallow altering invalidated replication slots
Date: Tue, 10 Sep 2024 23:24:35 +0530
Message-ID: <CALj2ACVh8otuX5HS1bB5=ikVk4UNHCkOaw2CbqTv1-E4J82p=g@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
References: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>
<CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
Hi,
Thanks for reviewing.
On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <[email protected]> wrote:
>
> Commit message
>
> 1.
> ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
> as there is no way...
>
> suggestion:
> ALTER_REPLICATION_SLOT for invalid replication slots should not be
> allowed because there is no way...
Modified.
> ======
> 2. Missing docs update
>
> Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
> not allowed for invalid slots?
Haven't noticed for other ERROR cases in the docs, e.g. slots being
synced, temporary slots. Not sure if it's worth adding every ERROR
case to the docs.
> ======
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot alter replication slot \"%s\"", name),
> + errdetail("This replication slot was invalidated due to \"%s\".",
> + SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
> +
>
> I thought including the reason "invalid" (e.g. "cannot alter invalid
> replication slot \"%s\"") in the message might be better, but OTOH I
> see the patch message is the same as an existing one. Maybe see what
> others think.
Changed.
> ======
> src/test/recovery/t/035_standby_logical_decoding.pl
>
> 3.
> There is already a comment about this test:
> ##################################################
> # Recovery conflict: Invalidate conflicting slots, including in-use slots
> # Scenario 1: hot_standby_feedback off and vacuum FULL
> #
> # In passing, ensure that replication slot stats are not removed when the
> # active slot is invalidated.
> ##################################################
>
> IMO we should update that "In passing..." sentence to something like:
>
> In passing, ensure that replication slot stats are not removed when
> the active slot is invalidated, and check that an error occurs when
> attempting to alter the invalid slot.
Added. But, keeping it closer to the test case doesn't hurt.
Please find the attached v2 patch also having Shveta's review comments
addressed.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v2-0001-Disallow-altering-invalidated-replication-slots.patch (3.0K, ../CALj2ACVh8otuX5HS1bB5=ikVk4UNHCkOaw2CbqTv1-E4J82p=g@mail.gmail.com/2-v2-0001-Disallow-altering-invalidated-replication-slots.patch)
download | inline diff:
From 80f9ee78ad72675fb1e2e1d28f1cfe75a1d1e18b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 10 Sep 2024 17:51:56 +0000
Subject: [PATCH v2] Disallow altering invalidated replication slots
ALTER_REPLICATION_SLOT for invalid replication slots should not be
allowed because there is no way to get back the invalidated
(logical) slot to working. This commit adds an error.
Author: Bharath Rupireddy
Reviewed-by: Peter Smith, shveta malik
Discussion: https://www.postgresql.org/message-id/CAA4eK1%2Bszcosq0nS109mMSxPWyNT1Q%3DUNYCJgXKYuCceaPS%2BhA%40mail.gmail.com
---
src/backend/replication/slot.c | 7 +++++++
src/test/recovery/t/035_standby_logical_decoding.pl | 12 +++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 0a03776156..6828100cf1 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -820,6 +820,13 @@ ReplicationSlotAlter(const char *name, const bool *failover,
errmsg("cannot use %s with a physical replication slot",
"ALTER_REPLICATION_SLOT"));
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter invalid replication slot \"%s\"", name),
+ errdetail("This replication slot has been invalidated due to \"%s\".",
+ SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
+
if (RecoveryInProgress())
{
/*
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index 4185b803b2..d658fdf417 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -495,7 +495,8 @@ $node_subscriber->stop;
# Scenario 1: hot_standby_feedback off and vacuum FULL
#
# In passing, ensure that replication slot stats are not removed when the
-# active slot is invalidated.
+# active slot is invalidated, and check that an error occurs when
+# attempting to alter the invalid slot.
##################################################
# One way to produce recovery conflict is to create/drop a relation and
@@ -526,6 +527,15 @@ check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class');
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('vacuum_full_', 'rows_removed');
+# Attempting to alter an invalidated slot should result in an error
+($result, $stdout, $stderr) = $node_standby->psql(
+ 'postgres',
+ qq[ALTER_REPLICATION_SLOT vacuum_full_inactiveslot (failover);],
+ replication => 'database');
+ok($stderr =~ /ERROR: cannot alter invalid replication slot "vacuum_full_inactiveslot"/ &&
+ $stderr =~ /DETAIL: This replication slot has been invalidated due to "rows_removed"./,
+ "invalidated slot cannot be altered");
+
# Ensure that replication slot stats are not removed after invalidation.
is( $node_standby->safe_psql(
'testdb',
--
2.43.0
view thread (4+ messages)
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]
Subject: Re: Disallow altering invalidated replication slots
In-Reply-To: <CALj2ACVh8otuX5HS1bB5=ikVk4UNHCkOaw2CbqTv1-E4J82p=g@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