Обсуждение: 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] 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

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.

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

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

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.

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.