MIME-Version: 1.0
References: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>
 <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
In-Reply-To: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 10 Sep 2024 23:24:35 +0530
Message-ID: <CALj2ACVh8otuX5HS1bB5=ikVk4UNHCkOaw2CbqTv1-E4J82p=g@mail.gmail.com>
Subject: Re: Disallow altering invalidated replication slots
To: Peter Smith <smithpb2250@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000bd88e80621c791f2"
Archived-At: <https://www.postgresql.org/message-id/CALj2ACVh8otuX5HS1bB5%3DikVk4UNHCkOaw2CbqTv1-E4J82p%3Dg%40mail.gmail.com>
Precedence: bulk

--000000000000bd88e80621c791f2
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

Thanks for reviewing.

On Tue, Sep 10, 2024 at 8:40=E2=80=AFAM 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.

> =3D=3D=3D=3D=3D=3D
> 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.

> =3D=3D=3D=3D=3D=3D
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated !=3D 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.

> =3D=3D=3D=3D=3D=3D
> 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.

--=20
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--000000000000bd88e80621c791f2
Content-Type: application/octet-stream; 
	name="v2-0001-Disallow-altering-invalidated-replication-slots.patch"
Content-Disposition: attachment; 
	filename="v2-0001-Disallow-altering-invalidated-replication-slots.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_m0wqeds40>
X-Attachment-Id: f_m0wqeds40

RnJvbSA4MGY5ZWU3OGFkNzI2NzVmYjFlMmUxZDI4ZjFjZmU3NWExZDFlMThiIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBCaGFyYXRoIFJ1cGlyZWRkeSA8YmhhcmF0aC5ydXBpcmVkZHlm
b3Jwb3N0Z3Jlc0BnbWFpbC5jb20+CkRhdGU6IFR1ZSwgMTAgU2VwIDIwMjQgMTc6NTE6NTYgKzAw
MDAKU3ViamVjdDogW1BBVENIIHYyXSBEaXNhbGxvdyBhbHRlcmluZyBpbnZhbGlkYXRlZCByZXBs
aWNhdGlvbiBzbG90cwoKQUxURVJfUkVQTElDQVRJT05fU0xPVCBmb3IgaW52YWxpZCByZXBsaWNh
dGlvbiBzbG90cyBzaG91bGQgbm90IGJlCmFsbG93ZWQgYmVjYXVzZSB0aGVyZSBpcyBubyB3YXkg
dG8gZ2V0IGJhY2sgdGhlIGludmFsaWRhdGVkCihsb2dpY2FsKSBzbG90IHRvIHdvcmtpbmcuIFRo
aXMgY29tbWl0IGFkZHMgYW4gZXJyb3IuCgpBdXRob3I6IEJoYXJhdGggUnVwaXJlZGR5ClJldmll
d2VkLWJ5OiBQZXRlciBTbWl0aCwgc2h2ZXRhIG1hbGlrCkRpc2N1c3Npb246IGh0dHBzOi8vd3d3
LnBvc3RncmVzcWwub3JnL21lc3NhZ2UtaWQvQ0FBNGVLMSUyQnN6Y29zcTBuUzEwOW1NU3hQV3lO
VDFRJTNEVU5ZQ0pnWEtZdUNjZWFQUyUyQmhBJTQwbWFpbC5nbWFpbC5jb20KLS0tCiBzcmMvYmFj
a2VuZC9yZXBsaWNhdGlvbi9zbG90LmMgICAgICAgICAgICAgICAgICAgICAgfCAgNyArKysrKysr
CiBzcmMvdGVzdC9yZWNvdmVyeS90LzAzNV9zdGFuZGJ5X2xvZ2ljYWxfZGVjb2RpbmcucGwgfCAx
MiArKysrKysrKysrKy0KIDIgZmlsZXMgY2hhbmdlZCwgMTggaW5zZXJ0aW9ucygrKSwgMSBkZWxl
dGlvbigtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3QuYyBiL3Ny
Yy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3QuYwppbmRleCAwYTAzNzc2MTU2Li42ODI4MTAwY2Yx
IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90LmMKKysrIGIvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vc2xvdC5jCkBAIC04MjAsNiArODIwLDEzIEBAIFJlcGxpY2F0aW9u
U2xvdEFsdGVyKGNvbnN0IGNoYXIgKm5hbWUsIGNvbnN0IGJvb2wgKmZhaWxvdmVyLAogCQkJCWVy
cm1zZygiY2Fubm90IHVzZSAlcyB3aXRoIGEgcGh5c2ljYWwgcmVwbGljYXRpb24gc2xvdCIsCiAJ
CQkJCSAgICJBTFRFUl9SRVBMSUNBVElPTl9TTE9UIikpOwogCisJaWYgKE15UmVwbGljYXRpb25T
bG90LT5kYXRhLmludmFsaWRhdGVkICE9IFJTX0lOVkFMX05PTkUpCisJCWVyZXBvcnQoRVJST1Is
CisJCQkJZXJyY29kZShFUlJDT0RFX09CSkVDVF9OT1RfSU5fUFJFUkVRVUlTSVRFX1NUQVRFKSwK
KwkJCQllcnJtc2coImNhbm5vdCBhbHRlciBpbnZhbGlkIHJlcGxpY2F0aW9uIHNsb3QgXCIlc1wi
IiwgbmFtZSksCisJCQkJZXJyZGV0YWlsKCJUaGlzIHJlcGxpY2F0aW9uIHNsb3QgaGFzIGJlZW4g
aW52YWxpZGF0ZWQgZHVlIHRvIFwiJXNcIi4iLAorCQkJCQkJICBTbG90SW52YWxpZGF0aW9uQ2F1
c2VzW015UmVwbGljYXRpb25TbG90LT5kYXRhLmludmFsaWRhdGVkXSkpOworCiAJaWYgKFJlY292
ZXJ5SW5Qcm9ncmVzcygpKQogCXsKIAkJLyoKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlY292ZXJ5
L3QvMDM1X3N0YW5kYnlfbG9naWNhbF9kZWNvZGluZy5wbCBiL3NyYy90ZXN0L3JlY292ZXJ5L3Qv
MDM1X3N0YW5kYnlfbG9naWNhbF9kZWNvZGluZy5wbAppbmRleCA0MTg1YjgwM2IyLi5kNjU4ZmRm
NDE3IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9yZWNvdmVyeS90LzAzNV9zdGFuZGJ5X2xvZ2ljYWxf
ZGVjb2RpbmcucGwKKysrIGIvc3JjL3Rlc3QvcmVjb3ZlcnkvdC8wMzVfc3RhbmRieV9sb2dpY2Fs
X2RlY29kaW5nLnBsCkBAIC00OTUsNyArNDk1LDggQEAgJG5vZGVfc3Vic2NyaWJlci0+c3RvcDsK
ICMgU2NlbmFyaW8gMTogaG90X3N0YW5kYnlfZmVlZGJhY2sgb2ZmIGFuZCB2YWN1dW0gRlVMTAog
IwogIyBJbiBwYXNzaW5nLCBlbnN1cmUgdGhhdCByZXBsaWNhdGlvbiBzbG90IHN0YXRzIGFyZSBu
b3QgcmVtb3ZlZCB3aGVuIHRoZQotIyBhY3RpdmUgc2xvdCBpcyBpbnZhbGlkYXRlZC4KKyMgYWN0
aXZlIHNsb3QgaXMgaW52YWxpZGF0ZWQsIGFuZCBjaGVjayB0aGF0IGFuIGVycm9yIG9jY3VycyB3
aGVuCisjIGF0dGVtcHRpbmcgdG8gYWx0ZXIgdGhlIGludmFsaWQgc2xvdC4KICMjIyMjIyMjIyMj
IyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjCiAKICMgT25lIHdheSB0byBw
cm9kdWNlIHJlY292ZXJ5IGNvbmZsaWN0IGlzIHRvIGNyZWF0ZS9kcm9wIGEgcmVsYXRpb24gYW5k
CkBAIC01MjYsNiArNTI3LDE1IEBAIGNoZWNrX2Zvcl9pbnZhbGlkYXRpb24oJ3ZhY3V1bV9mdWxs
XycsIDEsICd3aXRoIHZhY3V1bSBGVUxMIG9uIHBnX2NsYXNzJyk7CiAjIFZlcmlmeSByZWFzb24g
Zm9yIGNvbmZsaWN0IGlzICdyb3dzX3JlbW92ZWQnIGluIHBnX3JlcGxpY2F0aW9uX3Nsb3RzCiBj
aGVja19zbG90c19jb25mbGljdF9yZWFzb24oJ3ZhY3V1bV9mdWxsXycsICdyb3dzX3JlbW92ZWQn
KTsKIAorIyBBdHRlbXB0aW5nIHRvIGFsdGVyIGFuIGludmFsaWRhdGVkIHNsb3Qgc2hvdWxkIHJl
c3VsdCBpbiBhbiBlcnJvcgorKCRyZXN1bHQsICRzdGRvdXQsICRzdGRlcnIpID0gJG5vZGVfc3Rh
bmRieS0+cHNxbCgKKyAgICAncG9zdGdyZXMnLAorICAgIHFxW0FMVEVSX1JFUExJQ0FUSU9OX1NM
T1QgdmFjdXVtX2Z1bGxfaW5hY3RpdmVzbG90IChmYWlsb3Zlcik7XSwKKyAgICByZXBsaWNhdGlv
biA9PiAnZGF0YWJhc2UnKTsKK29rKCRzdGRlcnIgPX4gL0VSUk9SOiAgY2Fubm90IGFsdGVyIGlu
dmFsaWQgcmVwbGljYXRpb24gc2xvdCAidmFjdXVtX2Z1bGxfaW5hY3RpdmVzbG90Ii8gJiYKKyAg
ICRzdGRlcnIgPX4gL0RFVEFJTDogIFRoaXMgcmVwbGljYXRpb24gc2xvdCBoYXMgYmVlbiBpbnZh
bGlkYXRlZCBkdWUgdG8gInJvd3NfcmVtb3ZlZCIuLywKKyAgICAiaW52YWxpZGF0ZWQgc2xvdCBj
YW5ub3QgYmUgYWx0ZXJlZCIpOworCiAjIEVuc3VyZSB0aGF0IHJlcGxpY2F0aW9uIHNsb3Qgc3Rh
dHMgYXJlIG5vdCByZW1vdmVkIGFmdGVyIGludmFsaWRhdGlvbi4KIGlzKCAkbm9kZV9zdGFuZGJ5
LT5zYWZlX3BzcWwoCiAJCSd0ZXN0ZGInLAotLSAKMi40My4wCgo=
--000000000000bd88e80621c791f2--