Re: Disallow altering invalidated replication slots

Hi,

Thanks for reviewing.

On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <smithpb2250(at)gmail(dot)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