Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wS5w2-002wuG-0g for pgsql-hackers@arkaria.postgresql.org; Wed, 27 May 2026 04:32:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wS5vz-0074a5-2d for pgsql-hackers@arkaria.postgresql.org; Wed, 27 May 2026 04:32:00 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wS5vz-0074Zv-0p for pgsql-hackers@lists.postgresql.org; Wed, 27 May 2026 04:32:00 +0000 Received: from mail-vs1-xe36.google.com ([2607:f8b0:4864:20::e36]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wS5vx-00000000xjh-2CHx for pgsql-hackers@lists.postgresql.org; Wed, 27 May 2026 04:31:58 +0000 Received: by mail-vs1-xe36.google.com with SMTP id ada2fe7eead31-67bfd0ec7f0so4579563137.2 for ; Tue, 26 May 2026 21:31:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1779856317; cv=none; d=google.com; s=arc-20240605; b=LBFcRFDVUyx7uWBZ9h8wtiKz7LiM3Q1SmEVLCxXFGi5N7cHUOzszMwRTF8N1aHZjgG veJ0Q+HK/sa+jW/15oc8Xut84klSkLmljEUdAg24qDkv5tWluHqex3bJQrmd4mNE9yTd lI2cHuO4FBtFcKyU+AINg977lsOK6HK1+48KEPi3WWXKdsmB4L4W6EysuscQJVqhPgG/ GufOw5xC29QRrQ9u8s8JIlzjtBP+oUkzd+WJX1So4n63v0nzPOU+zF2v8IAzwwZX5zTS /E4FhC7jP/q0Bz7lM2+HPf7ZWlRgqG+4EAaM1ouYOTnyZwbswLgHrcQMfPs9l4FbcjH5 P1dw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=o4Qs/cF79dh5dYwMD0e3IrjCSIRR7x/ywszR9/froKM=; fh=5fb08t6mxyY2ByK56gbCRsbEZEnLIoUu74ZjnnFNTDY=; b=E8esGswEvfw0FyWCcM47xlghecob7Kp0wfEkgoypmouu9KLFfZDYwh7RTYbE5LWbSk rtZfiFC36ENikXT2CTRNbAyN4eE6aAklwxzHMvea+IP7nYIymSbNTK/3Ur19ZmMZpkh1 OS6UeipAfuwOQqXR+Lh3BlqbtEr+nsPZAKifG77N2aazAR+NktgrV94jcTk5kxkaWVEa OfclUlh3GK1QoqkWOGBMc8RpLohssTPVCqSE3TsGZce5YV6FsHB6NNgw3j0TtzgZFJIw C9wcm63WUPfGQRc2XogtnW7r4HBXd5QXpUiuDCevyT3eFfcnLXtjvfjmic4MYr/2vLs9 P9Hg==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779856317; x=1780461117; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=o4Qs/cF79dh5dYwMD0e3IrjCSIRR7x/ywszR9/froKM=; b=Vqgf5QaV0VstnfEsNuc4O7QtvxK/pF8zXT7t8Cez9NXm6oim7c/86YgWdEfAFqFYh5 Amk9+T6DJPNOyx5tnM+nxxGBdPvtndai3Vw+x2Nbi1sc7WMFsynB6BD+XdWEh77SNbJA 8JnqEhyW8oHMTiBV9Io2iWPHxMzydDQVXwFvPHqdWHfo0SXhvoCpypmTqzuBVJCd1WrG fNYBZJEtsqqDJ+fNzBKXR5DiMXDTaPaNIJG3H+wTuDxlXaXdnS1OmHRBDX+XF6Oo3miU pszWzb/QUT76AWGDadymPMHh6eB/CWGI9szH16JZKeFH4pj3aV9T4Bpv11w0ES0kAwdt KKrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779856317; x=1780461117; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=o4Qs/cF79dh5dYwMD0e3IrjCSIRR7x/ywszR9/froKM=; b=bF7zu62wIOesCuNuztrj5xAZ4yOb1v0F/mNd6UghCIi27ChUtXYIuBuwfd2XEvk4Ug sOi1NDAhiPhtd29W8CFNcxripb7W9SUEUExh4C2j7mYCooV3AacyIKijg78Kq3qz+wvJ rFspcdYviQQoWri8VN/VFLpdyeKXcpsi7MmqDm13l7il2swJdJQf8qRjAoCRk6/tMpTS HzjgCkNAO2TncJ9nm1UoiHMzPtBG2eZQ++E330O9svIpIDxJ6r+OXroFxFw5w0YpPQRW 5ch1k1Vl6FPCJXVWexQsrX5j65Yj8I3adHiahZ9Xfctc4jS1/A3StxRW8gIw39byLvc1 26eA== X-Forwarded-Encrypted: i=1; AFNElJ+xuRN42t3B39pfi3FrQqUBRn08adfarCdIC535/qAsBKpj1P2pP+AhtoJ1p5jnVz7FQdxsGHGGW0YZr4jG@lists.postgresql.org X-Gm-Message-State: AOJu0YwdUuREXc7S088LJMqPYRo3iayvWOEj5fLUcy22aqR2JLPX9YqL HGpfRCiDMHKJv2sqJJIk3a4Od4skLP1RTr+cmrke58Y1M7JRRjHJOe2ukm+wfTV1E4HlaOXhjy3 H3hZ3TDeLMPmYMPy3XdL6DO/X898KA3c= X-Gm-Gg: Acq92OEnx1rY0FQfzZSymGFD9Yg/tM/g3FNLPiejBkem+42CMrYss+Wph2vvbkhM9BS 3qcMeXi66IZpkIUl+kWgG+ZgQKntBN2v4dzFvt75WU9HZoKmTvVjTYN9oZAyUH5HNJOc+khxQYv dLTvMbyNg7adJLHZLdeIUp1wQuFvKD2Q4Z90rT63hDCwct/p33b6zyo6tr/po1JKj3R+imojs/x V8yT6CxWy3V0LuF0C+cdyNL57YWnJTxUYXXq8MWfvRa6HjsAyLZMQfXnKrJ+NY2kO+FDmv8sdej Vct94xCd0tjYyz7Mp7zPNJAEo4rxxPni069V3WNB X-Received: by 2002:a05:6102:3e22:b0:631:ab8b:c348 with SMTP id ada2fe7eead31-67c803c8b93mr10974074137.8.1779856316658; Tue, 26 May 2026 21:31:56 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: SATYANARAYANA NARLAPURAM Date: Tue, 26 May 2026 21:31:45 -0700 X-Gm-Features: AVHnY4K12KXrH7AevB1T8ybWTbs_14zH1gHJPW2vwiDn96393zIldWq41eoPv7M Message-ID: Subject: Re: [PATCH] Release replication slot on error in SQL-callable slot functions To: shveta malik Cc: vignesh C , Fujii Masao , PostgreSQL Hackers , shveta malik Content-Type: multipart/alternative; boundary="0000000000008177570652c517d8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000008177570652c517d8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Shveta, On Tue, May 26, 2026 at 8:54=E2=80=AFPM shveta malik wrote: > On Tue, May 26, 2026 at 1:41=E2=80=AFPM SATYANARAYANA NARLAPURAM > wrote: > > > > Hi, > > > > On Mon, May 25, 2026 at 10:50=E2=80=AFPM shveta malik > wrote: > >> > >> On Tue, May 26, 2026 at 10:06=E2=80=AFAM shveta malik > wrote: > >> > > >> > On Tue, May 26, 2026 at 12:31=E2=80=AFAM SATYANARAYANA NARLAPURAM > >> > wrote: > >> > > > >> > > Hi, > >> > > > >> > > On Mon, May 25, 2026 at 2:58=E2=80=AFAM shveta malik < > shveta.malik@gmail.com> wrote: > >> > >> > >> > >> On Mon, May 25, 2026 at 12:42=E2=80=AFPM SATYANARAYANA NARLAPURAM > >> > >> wrote: > >> > >> > > >> > >> > Hi > >> > >> > > >> > >> > On Fri, May 22, 2026 at 2:16=E2=80=AFAM shveta malik < > shveta.malik@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 > cases as well. > >> > >> > > >> > >> > >> > >> Thank You for addressuing these cases. A few comments: > >> > >> > >> > >> 1) > >> > >> > >> > >> +-- Test 2: session remains usable after the error > (MyReplicationSlot cleared) > >> > >> > >> > >> It shoudl be part of 'Test 1' itself and thus should not be named > as 'Test 2' > >> > >> > >> > >> 2) > >> > >> -------- > >> > >> +-- Test 4: copy_replication_slot with max_replication_slots > exceeded. > >> > >> +-- We reduce max_replication_slots artificially by filling all > remaining slots. > >> > >> +-- Instead, trigger an error by copying to an already-existing > name. > >> > >> +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 > could > >> > >> not). Since creation of slot itself will fail as the slot with > >> > >> same-name already exists, MyReplicationSlot will never be set and > thus > >> > >> Assert will not be hit. A better testcase will be below which > fails > >> > >> during LoadOutputPlugin() after slot-creation and > MyReplicationSlot is > >> > >> set already. > >> > >> > >> > >> SELECT pg_create_logical_replication_slot('src_slot', > 'test_decoding'); > >> > >> > >> > >> 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 > case > >> > >> 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 > advanced */ > >> > + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn)) > >> > > >> > > >> > We can have a blank line after ReplicationSlotAcquire for better > readability. > >> > > >> > 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 > above. > >> > > >> > >> 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.patch > >> Patch format detection failed. > > > > Thanks , Shveta! Please find the attached v4 patch that addressed your > comments. > > > > 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. No specific reason to use temp slot, ok to create a permanent slot too. > > 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. Thank you for the changes and review. Thanks, Satya --0000000000008177570652c517d8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Shveta,

On Tue, May 26, 2026= at 8:54=E2=80=AFPM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, May 26, 2026 at 1:41=E2=80=AFPM SATYANARA= YANA NARLAPURAM
<satyanar= lapuram@gmail.com> wrote:
>
> Hi,
>
> On Mon, May 25, 2026 at 10:50=E2=80=AFPM shveta malik <shveta.malik@gmail.com&= gt; wrote:
>>
>> On Tue, May 26, 2026 at 10:06=E2=80=AFAM shveta malik <shveta.malik@gmail.com= > wrote:
>> >
>> > On Tue, May 26, 2026 at 12:31=E2=80=AFAM SATYANARAYANA NARLAP= URAM
>> > <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 SATYANARAYA= NA NARLAPURAM
>> > >> <satyanarlapuram@gmail.com> wrote:
>> > >> >
>> > >> > Hi
>> > >> >
>> > >> > On Fri, May 22, 2026 at 2:16=E2=80=AFAM shveta = malik <shvet= a.malik@gmail.com> wrote:
>> > >> >>
>> > >> >> Thanks for reporting the issue. I could rep= roduce 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 addresse= d these three cases as well.
>> > >> >
>> > >>
>> > >> Thank You for addressuing these cases. A few comment= s:
>> > >>
>> > >> 1)
>> > >>
>> > >> +-- Test 2: session remains usable after the error (= MyReplicationSlot cleared)
>> > >>
>> > >> It shoudl be part of 'Test 1' itself and thu= s should not be named as 'Test 2'
>> > >>
>> > >> 2)
>> > >> --------
>> > >> +-- Test 4: copy_replication_slot with max_replicati= on_slots exceeded.
>> > >> +-- We reduce max_replication_slots artificially by = filling all remaining slots.
>> > >> +-- Instead, trigger an error by copying to an alrea= dy-existing name.
>> > >> +DO $$
>> > >> +BEGIN
>> > >> +=C2=A0 =C2=A0 PERFORM pg_copy_logical_replication_s= lot('regression_slot_t3',
>> > >> 'regression_slot_t3');
>> > >> +EXCEPTION WHEN OTHERS THEN
>> > >> +=C2=A0 =C2=A0 RAISE NOTICE 'caught: %', SQL= ERRM;
>> > >> +END;
>> > >> +$$;
>> > >> +-- The original slot must still exist and be usable=
>> > >> +SELECT count(*) =3D 1 AS orig_slot_ok FROM pg_repli= cation_slots
>> > >> +=C2=A0 =C2=A0 WHERE slot_name =3D 'regression_s= lot_t3';
>> > >> -----------
>> > >>
>> > >> I don't think we can hit the Assert with above t= est (at-least I could
>> > >> not). Since creation of slot itself will fail as the= slot with
>> > >> same-name already exists, MyReplicationSlot will nev= er be set and thus
>> > >> Assert will not be hit.=C2=A0 A better testcase will= be below which fails
>> > >> during LoadOutputPlugin() after slot-creation and My= ReplicationSlot is
>> > >> set already.
>> > >>
>> > >> SELECT pg_create_logical_replication_slot('src_s= lot', 'test_decoding');
>> > >>
>> > >> DO $$
>> > >> BEGIN
>> > >> PERFORM pg_copy_logical_replication_slot('src_sl= ot', 'dst_slot',
>> > >> false, 'nonexistent_plugin');
>> > >> EXCEPTION WHEN others THEN
>> > >> RAISE NOTICE 'caught: %', SQLERRM;
>> > >> END $$;
>> > >>
>> > >> SELECT count(*) FROM pg_logical_slot_get_changes(= 9;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 w= e have added
>> > >> testcases. Last 4 are addressed by fixing common fun= ction
>> > >> pg_logical_slot_get_changes_guts(). I think we shoul= d add a test case
>> > >> for at-least any one of these APIs to cover
>> > >> pg_logical_slot_get_changes_guts().
>> > >
>> > >
>> > > Thanks for reviewing. Please review the attached v3 patc= h.
>> > >
>> >
>> > 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 advanced */
>> > + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_l= sn))
>> >
>> >
>> > We can have a blank line after ReplicationSlotAcquire for bet= ter readability.
>> >
>> > 2)
>> >
>> > +SELECT 'init' FROM
>> > pg_create_logical_replication_slot('regression_slot_t3= 9;,
>> > 'test_decoding', true);
>> > +SELECT count(*) =3D 1 AS slot_exists FROM pg_replication_slo= ts
>> > +=C2=A0 =C2=A0 WHERE slot_name =3D 'regression_slot_t3= 9;;
>> >
>> > The intent is not clear why are we checking existence of
>> > regression_slot_t3? I think we can skip it (or else add a com= ment 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 &= #39;-- cleanup'
>> >
>> > ~~
>> >
>> > I have no further comments other than the trivial things ment= ioned above.
>> >
>>
>> 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, &= #39;patch -p1'
>> works.
>>
>> git am v3-0001-Release-replication-slot-on-error-in-slot-SQL-funct= ions.patch
>> Patch format detection failed.
>
> Thanks , Shveta! Please find the attached v4 patch that addressed your= comments.
>

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.

No specific reason to use temp slot, ok to create a permanent slot too= .



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

Thank you for the changes and review.
=

Thanks,
Satya
--0000000000008177570652c517d8--