Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Prohibit altering invalidated replication slots.
- 4d8489f4f1fa 18.0 landed
-
Disallow altering invalidated replication slots
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-09-09T18:41:01Z
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] https://www.postgresql.org/message-id/CAA4eK1%2Bszcosq0nS109mMSxPWyNT1Q%3DUNYCJgXKYuCceaPS%2BhA%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
-
Re: Disallow altering invalidated replication slots
Peter Smith <smithpb2250@gmail.com> — 2024-09-10T03:09:45Z
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 -
Re: Disallow altering invalidated replication slots
shveta malik <shveta.malik@gmail.com> — 2024-09-10T05:07:19Z
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 -
Re: Disallow altering invalidated replication slots
Amit Kapila <amit.kapila16@gmail.com> — 2024-09-10T05:23:07Z
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. -
Re: Disallow altering invalidated replication slots
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-09-10T17:54:35Z
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 -
Re: Disallow altering invalidated replication slots
Peter Smith <smithpb2250@gmail.com> — 2024-09-11T01:29:03Z
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 -
Re: Disallow altering invalidated replication slots
shveta malik <shveta.malik@gmail.com> — 2024-09-11T03:11:35Z
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
-
Re: Disallow altering invalidated replication slots
Amit Kapila <amit.kapila16@gmail.com> — 2024-09-12T10:54:53Z
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.
-
Re: Disallow altering invalidated replication slots
Amit Kapila <amit.kapila16@gmail.com> — 2024-09-13T06:16:27Z
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.