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>
In-Reply-To: 
 <CAJpy0uCCqFLY7pu0RQVcS9fRr0FimFMuHPsMBQ-KzEGX3BEGPA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Tue, 26 May 2026 11:20:03 +0530
Message-ID: 
 <CAJpy0uDHMvpUAdwXA3X7ugmO8S7kry-ZtrKUcugpX3WWp8hykw@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: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uDHMvpUAdwXA3X7ugmO8S7kry-ZtrKUcugpX3WWp8hykw%40mail.gmail.com>
Precedence: bulk

On Tue, May 26, 2026 at 10:06=E2=80=AFAM shveta malik <shveta.malik@gmail.c=
om> 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@gmai=
l.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.malik@g=
mail.com> wrote:
> >> >>
> >> >> Thanks for reporting the issue. I could reproduce the same issue wi=
th
> >> >> 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 a=
s 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 remain=
ing 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, NUL=
L);
> >>
> >> 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 readabil=
ity.
>
> 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.patc=
h
Patch format detection failed.

thanks
Shveta