Disallow altering invalidated replication slots

Started by Bharath Rupireddyover 1 year ago9 messages

bharath.rupireddyforpostgres@gmail.com

over 1 year ago

1 attachment(s)

Hi,

ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
as there is no way to get the invalidated (logical) slot to work.
Please find the patch to add an error in such cases. Relevant
discussion is at [1]/messages/by-id/CAA4eK1+szcosq0nS109mMSxPWyNT1Q=UNYCJgXKYuCceaPS+hA@mail.gmail.com.

Thoughts?

[1]: /messages/by-id/CAA4eK1+szcosq0nS109mMSxPWyNT1Q=UNYCJgXKYuCceaPS+hA@mail.gmail.com

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachments:

v1-0001-Disallow-altering-invalidated-replication-slots.patchapplication/octet-stream; name=v1-0001-Disallow-altering-invalidated-replication-slots.patchDownload

From 8b2a7e7c629c5d21fdb801739d9661722904dd7e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 9 Sep 2024 18:25:16 +0000
Subject: [PATCH v1] Disallow altering invalidated replication slots

ALTER_REPLICATION_SLOT on invalidated replication slots is
unnecessary as there is no way to get the invalidated
(logical) slot to work. This commit adds an error.

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 | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 0a03776156..35652d9ad8 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 replication slot \"%s\"", name),
+				errdetail("This replication slot was 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..75ccfca409 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -526,6 +526,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 replication slot "vacuum_full_inactiveslot"/ &&
+   $stderr =~ /DETAIL:  This replication slot was 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

Peter Smith

smithpb2250@gmail.com

over 1 year ago

In reply to: Bharath Rupireddy (#1)

Re: Disallow altering invalidated replication slots

Hi, here are some review comments for patch v1.

======
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...

======
2. Missing docs update

Should this docs page [1]docs - https://www.postgresql.org/docs/devel/protocol-replication.html be updated to say ALTER_REPLICATION_SLOT is
not allowed for invalid slots?

======
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.

======
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.

======
[1]: docs - https://www.postgresql.org/docs/devel/protocol-replication.html

Kind Regards,
Peter Smith.
Fujitsu Austalia

shveta malik

shveta.malik@gmail.com

over 1 year ago

In reply to: Bharath Rupireddy (#1)

Re: Disallow altering invalidated replication slots

On Tue, Sep 10, 2024 at 12:11 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
as there is no way to get the invalidated (logical) slot to work.
Please find the patch to add an error in such cases. Relevant
discussion is at [1].

Thoughts?

+1 on the idea.

+ errmsg("cannot alter replication slot \"%s\"", name),
+ errdetail("This replication slot was invalidated due to \"%s\".",

Maybe we shall have: "This slot has been invalidated due to ..."
This is similar to all other occurrences where such errors are raised,
see logical.c for instance.

thanks
Shveta

Amit Kapila

amit.kapila16@gmail.com

over 1 year ago

In reply to: Peter Smith (#2)

Re: Disallow altering invalidated replication slots

On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <smithpb2250@gmail.com> wrote:

Hi, here are some review comments for patch v1.

======
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...

======
2. Missing docs update

Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
not allowed for invalid slots?

======
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,

Agreed, I could see a similar case with a message ("cannot alter
invalid database \"%s\"") in the code. Additionally, we should also
include Shveta's suggestion to change the detailed message to other
similar messages in logical.c

--
With Regards,
Amit Kapila.

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

over 1 year ago

In reply to: Peter Smith (#2)

1 attachment(s)

Re: Disallow altering invalidated replication slots

Hi,

Thanks for reviewing.

On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <smithpb2250@gmail.com> 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:

v2-0001-Disallow-altering-invalidated-replication-slots.patchapplication/octet-stream; name=v2-0001-Disallow-altering-invalidated-replication-slots.patchDownload

From 80f9ee78ad72675fb1e2e1d28f1cfe75a1d1e18b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
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

Peter Smith

smithpb2250@gmail.com

over 1 year ago

In reply to: Bharath Rupireddy (#5)

Re: Disallow altering invalidated replication slots

On Wed, Sep 11, 2024 at 3:54 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Thanks for reviewing.

On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <smithpb2250@gmail.com> 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.

OK.

======
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.

The v2 patch looks OK to me.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

shveta malik

shveta.malik@gmail.com

over 1 year ago

In reply to: Bharath Rupireddy (#5)

Re: Disallow altering invalidated replication slots

On Tue, Sep 10, 2024 at 11:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Please find the attached v2 patch also having Shveta's review comments
addressed.

The v2 patch looks good to me.

thanks
Shveta

Amit Kapila

amit.kapila16@gmail.com

over 1 year ago

In reply to: shveta malik (#7)

Re: Disallow altering invalidated replication slots

On Wed, Sep 11, 2024 at 8:41 AM shveta malik <shveta.malik@gmail.com> wrote:

On Tue, Sep 10, 2024 at 11:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Please find the attached v2 patch also having Shveta's review comments
addressed.

The v2 patch looks good to me.

LGTM as well. I'll push this tomorrow morning unless there are more
comments or suggestions.

--
With Regards,
Amit Kapila.

Amit Kapila

amit.kapila16@gmail.com

over 1 year ago

In reply to: Amit Kapila (#8)

Re: Disallow altering invalidated replication slots

On Thu, Sep 12, 2024 at 4:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Sep 11, 2024 at 8:41 AM shveta malik <shveta.malik@gmail.com> wrote:

On Tue, Sep 10, 2024 at 11:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Please find the attached v2 patch also having Shveta's review comments
addressed.

The v2 patch looks good to me.

LGTM as well. I'll push this tomorrow morning unless there are more
comments or suggestions.

Pushed.

--
With Regards,
Amit Kapila.