MIME-Version: 1.0
References: 
 <CAHg+QDeuf9tCq3ce=kgFMJP0m=PZC+wi6B=yS+7V0vNXjLS31w@mail.gmail.com>
 <CAHGQGwFZaWj8DctXuhWQZwSqi631=NKzQJyDV4yqT1Qapt8MFQ@mail.gmail.com>
 <CALDaNm1Jjun=by60V-4EpLZe4pAKy0qVZ7ptyHGVCuDyKfo2xQ@mail.gmail.com>
 <CAHg+QDcu2x0mjkBSqRxP_8EQ6UmpuX_jMgdKLDkAL1=N6wzZCQ@mail.gmail.com>
 <CAJpy0uCmW_NUZN8mw26onvfoFzH_oMrFSKhLUhz896nDgf8c7Q@mail.gmail.com>
 <CAHg+QDcf_9prAX=TaSO3UUiCLVD53bEw-KLqzAEXi+ud7h+Z4w@mail.gmail.com>
 <CAJpy0uBShUF_xm0=BVWivpWHt-4zs__k_3wL1RRjpi0Av8nsog@mail.gmail.com>
 <CAHg+QDf5PVyFgesBNs1GvOnuk_khoXifo96A7QW1EJ8zhhBxyw@mail.gmail.com>
 <CAJpy0uCCqFLY7pu0RQVcS9fRr0FimFMuHPsMBQ-KzEGX3BEGPA@mail.gmail.com>
 <CAJpy0uDHMvpUAdwXA3X7ugmO8S7kry-ZtrKUcugpX3WWp8hykw@mail.gmail.com>
 <CAHg+QDdEGGQrmQXwH0_Y+DRe_tx5jxv9K+7vpCnooQn2_6QykA@mail.gmail.com>
In-Reply-To: 
 <CAHg+QDdEGGQrmQXwH0_Y+DRe_tx5jxv9K+7vpCnooQn2_6QykA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Wed, 27 May 2026 09:23:54 +0530
Message-ID: 
 <CAJpy0uBo-OuzyZZ=LY8L48Udt8=1Mffh2rrpXaTbKK_F9sJx7A@mail.gmail.com>
Subject: Re: [PATCH] Release replication slot on error in SQL-callable slot
 functions
To: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Fujii Masao <masao.fujii@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 shveta malik <shvetamalik@gmail.com>
Content-Type: multipart/mixed; boundary="0000000000003920060652c49032"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uBo-OuzyZZ%3DLY8L48Udt8%3D1Mffh2rrpXaTbKK_F9sJx7A%40mail.gmail.com>
Precedence: bulk

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

On Tue, May 26, 2026 at 1:41=E2=80=AFPM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
>
> Hi,
>
> On Mon, May 25, 2026 at 10:50=E2=80=AFPM shveta malik <shveta.malik@gmail=
.com> wrote:
>>
>> On Tue, May 26, 2026 at 10:06=E2=80=AFAM shveta malik <shveta.malik@gmai=
l.com> wrote:
>> >
>> > On Tue, May 26, 2026 at 12:31=E2=80=AFAM SATYANARAYANA NARLAPURAM
>> > <satyanarlapuram@gmail.com> wrote:
>> > >
>> > > Hi,
>> > >
>> > > On Mon, May 25, 2026 at 2:58=E2=80=AFAM shveta malik <shveta.malik@g=
mail.com> wrote:
>> > >>
>> > >> On Mon, May 25, 2026 at 12:42=E2=80=AFPM SATYANARAYANA NARLAPURAM
>> > >> <satyanarlapuram@gmail.com> wrote:
>> > >> >
>> > >> > Hi
>> > >> >
>> > >> > On Fri, May 22, 2026 at 2:16=E2=80=AFAM shveta malik <shveta.mali=
k@gmail.com> wrote:
>> > >> >>
>> > >> >> Thanks for reporting the issue. I could reproduce the same issue=
 with
>> > >> >> all these as well:
>> > >> >>
>> > >> >> pg_logical_slot_peek_changes
>> > >> >> pg_logical_slot_get_binary_changes
>> > >> >> pg_logical_slot_peek_binary_changes
>> > >> >
>> > >> >
>> > >> > Please find the attached v2 patch that addressed these three case=
s as well.
>> > >> >
>> > >>
>> > >> Thank You for addressuing these cases. A few comments:
>> > >>
>> > >> 1)
>> > >>
>> > >> +-- Test 2: session remains usable after the error (MyReplicationSl=
ot cleared)
>> > >>
>> > >> It shoudl be part of 'Test 1' itself and thus should not be named a=
s 'Test 2'
>> > >>
>> > >> 2)
>> > >> --------
>> > >> +-- Test 4: copy_replication_slot with max_replication_slots exceed=
ed.
>> > >> +-- We reduce max_replication_slots artificially by filling all rem=
aining slots.
>> > >> +-- Instead, trigger an error by copying to an already-existing nam=
e.
>> > >> +DO $$
>> > >> +BEGIN
>> > >> +    PERFORM pg_copy_logical_replication_slot('regression_slot_t3',
>> > >> 'regression_slot_t3');
>> > >> +EXCEPTION WHEN OTHERS THEN
>> > >> +    RAISE NOTICE 'caught: %', SQLERRM;
>> > >> +END;
>> > >> +$$;
>> > >> +-- The original slot must still exist and be usable
>> > >> +SELECT count(*) =3D 1 AS orig_slot_ok FROM pg_replication_slots
>> > >> +    WHERE slot_name =3D 'regression_slot_t3';
>> > >> -----------
>> > >>
>> > >> I don't think we can hit the Assert with above test (at-least I cou=
ld
>> > >> not). Since creation of slot itself will fail as the slot with
>> > >> same-name already exists, MyReplicationSlot will never be set and t=
hus
>> > >> Assert will not be hit.  A better testcase will be below which fail=
s
>> > >> during LoadOutputPlugin() after slot-creation and MyReplicationSlot=
 is
>> > >> set already.
>> > >>
>> > >> SELECT pg_create_logical_replication_slot('src_slot', 'test_decodin=
g');
>> > >>
>> > >> DO $$
>> > >> BEGIN
>> > >> PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot',
>> > >> false, 'nonexistent_plugin');
>> > >> EXCEPTION WHEN others THEN
>> > >> RAISE NOTICE 'caught: %', SQLERRM;
>> > >> END $$;
>> > >>
>> > >> SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, =
NULL);
>> > >>
>> > >> 3)
>> > >> So overall these are the problematic APIs:
>> > >>
>> > >> pg_create_logical_replication_slot
>> > >> pg_replication_slot_advance
>> > >> pg_copy_logical_replication_slot
>> > >> pg_logical_slot_peek_binary_changes
>> > >> pg_logical_slot_peek_changes
>> > >> pg_logical_slot_get_changes
>> > >> pg_logical_slot_get_binary_changes
>> > >>
>> > >> First 3 are are mutually exclusive fixes fow which we have added
>> > >> testcases. Last 4 are addressed by fixing common function
>> > >> pg_logical_slot_get_changes_guts(). I think we should add a test ca=
se
>> > >> for at-least any one of these APIs to cover
>> > >> pg_logical_slot_get_changes_guts().
>> > >
>> > >
>> > > Thanks for reviewing. Please review the attached v3 patch.
>> > >
>> >
>> > A few trivial things:
>> >
>> > 1)
>> > pg_replication_slot_advance:
>> > + PG_TRY();
>> > + {
>> > + /* Acquire the slot so we "own" it */
>> > + ReplicationSlotAcquire(NameStr(*slotname), true, true);
>> > + /* A slot whose restart_lsn has never been reserved cannot be advanc=
ed */
>> > + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn))
>> >
>> >
>> > We can have a blank line after ReplicationSlotAcquire for better reada=
bility.
>> >
>> > 2)
>> >
>> > +SELECT 'init' FROM
>> > pg_create_logical_replication_slot('regression_slot_t3',
>> > 'test_decoding', true);
>> > +SELECT count(*) =3D 1 AS slot_exists FROM pg_replication_slots
>> > +    WHERE slot_name =3D 'regression_slot_t3';
>> >
>> > The intent is not clear why are we checking existence of
>> > regression_slot_t3? I think we can skip it (or else add a comment if
>> > really needed). The success of previous
>> > pg_create_logical_replication_slot is enough to confirm that session
>> > is healthy to run other slot related queries.
>> >
>> > 3)
>> > +SELECT pg_drop_replication_slot('regression_slot_phy');
>> > +
>> > +-- cleanup
>> > +SELECT pg_drop_replication_slot('regression_slot_t3');
>> >
>> > We can move drop of 'regression_slot_phy' too under '-- cleanup'
>> >
>> > ~~
>> >
>> > I have no further comments other than the trivial things mentioned abo=
ve.
>> >
>>
>> Missed to inform this earlier, I am not able to apply any version of
>> the patches shared so far with 'git am'. It gives error, 'patch -p1'
>> works.
>>
>> git am v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.p=
atch
>> Patch format detection failed.
>
> Thanks , Shveta! Please find the attached v4 patch that addressed your co=
mments.
>

Thank You for the patch.
I noticed that we are creating regression_slot_t3 as a a temporary
slot, is that intentional? I think creating a permanent slot here will
be a better testcase.

I have made a few cosmetic changes for better readability along with
creating the 'permanent' regression_slot_t3 slot. Please incorporate
what you think is okay. I have no more comments.

thanks
Shveta

--0000000000003920060652c49032
Content-Type: text/plain; charset="US-ASCII";
 name="0001-top-up-changes.patch.txt"
Content-Disposition: attachment; filename="0001-top-up-changes.patch.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_mpnj4jw80>
X-Attachment-Id: f_mpnj4jw80

RnJvbSBmZDRlOGY3MzFmYmZhNWNjZWY4ODMzOGQ2ZmEzOWZlMmIyYTE4MjBiIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBTaHZldGEgTWFsaWsgPHNodmV0YS5tYWxpa0BnbWFpbC5jb20+
CkRhdGU6IFdlZCwgMjcgTWF5IDIwMjYgMDk6MTg6NTAgKzA1MzAKU3ViamVjdDogW1BBVENIXSB0
b3AtdXAgY2hhbmdlcwoKLS0tCiBjb250cmliL3Rlc3RfZGVjb2Rpbmcvc3FsL3Nsb3Quc3FsICB8
IDExICsrKysrKysrKy0tCiBzcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYyB8ICAz
ICsrKwogMiBmaWxlcyBjaGFuZ2VkLCAxMiBpbnNlcnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQoK
ZGlmZiAtLWdpdCBhL2NvbnRyaWIvdGVzdF9kZWNvZGluZy9zcWwvc2xvdC5zcWwgYi9jb250cmli
L3Rlc3RfZGVjb2Rpbmcvc3FsL3Nsb3Quc3FsCmluZGV4IGVhYmJlMDVjYzkwLi5kOGUwYWRjY2Jm
YiAxMDA2NDQKLS0tIGEvY29udHJpYi90ZXN0X2RlY29kaW5nL3NxbC9zbG90LnNxbAorKysgYi9j
b250cmliL3Rlc3RfZGVjb2Rpbmcvc3FsL3Nsb3Quc3FsCkBAIC0yMDYsMTQgKzIwNiwxOCBAQCBF
WENFUFRJT04gV0hFTiBPVEhFUlMgVEhFTgogICAgIFJBSVNFIE5PVElDRSAnY2F1Z2h0OiAlJywg
U1FMRVJSTTsKIEVORDsKICQkOworCistLSB0aGUgY29uY2VybmVkIHNsb3QgbXVzdCBub3QgZXhp
c3QgKGl0IHdhcyBkcm9wcGVkIG9uIGVycm9yKQogU0VMRUNUIGNvdW50KCopID0gMCBBUyBzbG90
X3dhc19kcm9wcGVkIEZST00gcGdfcmVwbGljYXRpb25fc2xvdHMKICAgICBXSEVSRSBzbG90X25h
bWUgPSAncmVncmVzc2lvbl9zbG90X2Vycm9yJzsKIAotU0VMRUNUICdpbml0JyBGUk9NIHBnX2Ny
ZWF0ZV9sb2dpY2FsX3JlcGxpY2F0aW9uX3Nsb3QoJ3JlZ3Jlc3Npb25fc2xvdF90MycsICd0ZXN0
X2RlY29kaW5nJywgdHJ1ZSk7CistLSB0aGUgc2Vzc2lvbiBpcyBzdGlsbCB1c2FibGUKK1NFTEVD
VCAnaW5pdCcgRlJPTSBwZ19jcmVhdGVfbG9naWNhbF9yZXBsaWNhdGlvbl9zbG90KCdyZWdyZXNz
aW9uX3Nsb3RfdDMnLCAndGVzdF9kZWNvZGluZycsIGZhbHNlKTsKIAogLS0gcGdfcmVwbGljYXRp
b25fc2xvdF9hZHZhbmNlOiBlcnJvciBhZnRlciBhY3F1aXJpbmcgdGhlIHNsb3Qgc2hvdWxkCiAt
LSByZWxlYXNlIGl0IHNvIHRoZSBzZXNzaW9uIHN0YXlzIHVzYWJsZS4KIFNFTEVDVCBzbG90X25h
bWUgRlJPTSBwZ19yZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UoJ3JlZ3Jlc3Npb25fc2xvdF90Mycs
IHBnX2N1cnJlbnRfd2FsX2xzbigpKTsKKwogRE8gJCQKIEJFR0lOCiAgICAgUEVSRk9STSBwZ19y
ZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UoJ3JlZ3Jlc3Npb25fc2xvdF90MycsICcwLzEnKTsKQEAg
LTIyMSw3ICsyMjUsOCBAQCBFWENFUFRJT04gV0hFTiBPVEhFUlMgVEhFTgogICAgIFJBSVNFIE5P
VElDRSAnY2F1Z2h0IGV4cGVjdGVkIGVycm9yJzsKIEVORDsKICQkOwotLS0gdGhlIHNlc3Npb24g
aXMgc3RpbGwgaGVhbHRoeQorCistLSB0aGUgc2Vzc2lvbiBpcyBzdGlsbCB1c2FibGUKIFNFTEVD
VCBzbG90X25hbWUgRlJPTSBwZ19yZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UoJ3JlZ3Jlc3Npb25f
c2xvdF90MycsIHBnX2N1cnJlbnRfd2FsX2xzbigpKTsKIAogLS0gcGdfY29weV9sb2dpY2FsX3Jl
cGxpY2F0aW9uX3Nsb3Q6IGVycm9yIGFmdGVyIGNyZWF0aW5nIHRoZSBkZXN0aW5hdGlvbgpAQCAt
MjMzLDkgKzIzOCwxMSBAQCBFWENFUFRJT04gV0hFTiBPVEhFUlMgVEhFTgogICAgIFJBSVNFIE5P
VElDRSAnY2F1Z2h0OiAlJywgU1FMRVJSTTsKIEVORDsKICQkOworCiAtLSB0aGUgZGVzdGluYXRp
b24gc2xvdCBtdXN0IG5vdCBleGlzdCAoaXQgd2FzIGRyb3BwZWQgb24gZXJyb3IpCiBTRUxFQ1Qg
Y291bnQoKikgPSAwIEFTIGRzdF9zbG90X2Ryb3BwZWQgRlJPTSBwZ19yZXBsaWNhdGlvbl9zbG90
cwogICAgIFdIRVJFIHNsb3RfbmFtZSA9ICdyZWdyZXNzaW9uX3Nsb3RfZHN0JzsKKwogLS0gdGhl
IHNlc3Npb24gaXMgc3RpbGwgdXNhYmxlCiBTRUxFQ1QgY291bnQoKikgPj0gMCBBUyBjaGFuZ2Vz
X29rIEZST00gcGdfbG9naWNhbF9zbG90X2dldF9jaGFuZ2VzKCdyZWdyZXNzaW9uX3Nsb3RfdDMn
LCBOVUxMLCBOVUxMKTsKIApkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xv
dGZ1bmNzLmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYwppbmRleCA5YmE5
YmQ4OTJlNi4uZDMyZTkxODdiOTUgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9u
L3Nsb3RmdW5jcy5jCisrKyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3RmdW5jcy5jCkBA
IC0xOTQsNiArMTk0LDcgQEAgY3JlYXRlX2xvZ2ljYWxfcmVwbGljYXRpb25fc2xvdChjaGFyICpu
YW1lLCBjaGFyICpwbHVnaW4sCiAJCSAqLwogCQlpZiAoTXlSZXBsaWNhdGlvblNsb3QgIT0gTlVM
TCkKIAkJCVJlcGxpY2F0aW9uU2xvdERyb3BBY3F1aXJlZCgpOworCiAJCVBHX1JFX1RIUk9XKCk7
CiAJfQogCVBHX0VORF9UUlkoKTsKQEAgLTYzMyw2ICs2MzQsNyBAQCBwZ19yZXBsaWNhdGlvbl9z
bG90X2FkdmFuY2UoUEdfRlVOQ1RJT05fQVJHUykKIAl7CiAJCWlmIChNeVJlcGxpY2F0aW9uU2xv
dCAhPSBOVUxMKQogCQkJUmVwbGljYXRpb25TbG90UmVsZWFzZSgpOworCiAJCVBHX1JFX1RIUk9X
KCk7CiAJfQogCVBHX0VORF9UUlkoKTsKQEAgLTg5Niw2ICs4OTgsNyBAQCBjb3B5X3JlcGxpY2F0
aW9uX3Nsb3QoRnVuY3Rpb25DYWxsSW5mbyBmY2luZm8sIGJvb2wgbG9naWNhbF9zbG90KQogCQkg
Ki8KIAkJaWYgKE15UmVwbGljYXRpb25TbG90ICE9IE5VTEwpCiAJCQlSZXBsaWNhdGlvblNsb3RE
cm9wQWNxdWlyZWQoKTsKKwogCQlQR19SRV9USFJPVygpOwogCX0KIAlQR19FTkRfVFJZKCk7Ci0t
IAoyLjM0LjEKCg==
--0000000000003920060652c49032--