Subject: Re: Disallow altering invalidated replication slots

From: Peter Smith <[email-masked]>
Date: Tue, 10 Sep 2024 13:09:45 +1000

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 10 Sep 2024 10:53:07 +0530

On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <[email-masked]> wrote:
> 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.

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 10 Sep 2024 23:24:35 +0530

Hi,

Thanks for reviewing.

> allowed because there is no way...

Modified.

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

> see the patch message is the same as an existing one. Maybe see what

Changed.

> There is already a comment about this test:

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