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