On Wed, Jul 15, 2026 at 10:16 PM Nisha Moond <[email protected]> wrote:
>
> On Mon, Jul 13, 2026 at 3:31 PM shveta malik <[email protected]> wrote:
> >
> > On Fri, Jul 10, 2026 at 5:07 PM Nisha Moond <[email protected]>
> > wrote:
> > >
> > >
> > > Yes, that works for me as we have FOR TABLES example to follow. I've
> > > made the changes in v20.
> > >
> >
> > Thanks! I had a quick look, few initial comments:
> >
> >
> > 1)
> > GetTopMostAncestorInPublication():
> >
> > + if (ancestors == NIL)
> > + return InvalidOid;
> >
> > Should this be Assert or can there be a case where ancestors list can
> > come as NIL?
> >
>
> I found one case where ancestors can be NIL.
> Test case:
> CREATE TABLE t1 (a int) PARTITION BY RANGE (a);
> CREATE TABLE t1_part PARTITION OF t1 FOR VALUES FROM (1) TO (100);
> CREATE PUBLICATION mypub FOR TABLE t1;
>
> -- Get the partition into the pending-detach state:
> -- Session A:
> BEGIN;
> SELECT * FROM t1;
> -- leave open
>
> -- Session B (blocks, waiting on Session A's lock on t1):
> ALTER TABLE t1 DETACH PARTITION t1_part CONCURRENTLY;
>
> -- Session C, while B is blocked:
> SELECT * FROM pg_get_publication_tables('{mypub}'::text[],
> 't1_part'::regclass);
>
> In this scenario, Session C reaches GetTopMostAncestorInPublication()
> with ancestors == NIL. So I think we should keep return InvalidOid;
> rather than replacing it with an Assert.
>
> While investigating this race, I also hit assertions at places that
> call "llast_oid(ancestors)" without checking for NIL, for example in
> check_publication_add_relation() and RelationBuildPublicationDesc(). I
> fixed those by guarding with if (ancestors != NIL).
Okay, I will review.
> But due to this same race, other existing callers of llast_oid() on
> HEAD that assume ancestors is never NIL can also hit the following
> assertion:
> "TRAP: failed Assert("list != NIL"), File:
> "../../../../src/include/nodes/pg_list.h" ..."
>
> One such case is pgoutput.c:get_rel_sync_entry(). To reproduce on HEAD:
> -- create a slot
> SELECT pg_create_logical_replication_slot('myslot', 'pgoutput');
>
> -- get the partition into the pending-detach state (using same A & B
> sessions), and in session C run -
> INSERT INTO t1_part VALUES (5);
> SELECT * FROM pg_logical_slot_get_binary_changes('myslot', NULL, NULL,
> 'proto_version', '1', 'publication_names', 'mypub');
>
> I think this should be handled separately after analyzing whether
> there's a better fix than simply adding if (ancestors != NIL) guards
> at each call site.
> Note: Other callers of llast_oid(ancestors) may need similar review.
>
> Thoughts?
>
Nisha, I am not able to reproduce this on HEAD. Can you please verify
if you missed providing any steps? This I what I have tried:
Session1:
postgres=# SELECT pg_create_logical_replication_slot('myslot', 'pgoutput');
pg_create_logical_replication_slot
------------------------------------
(myslot,0/0178F5B8)
(1 row)
postgres=# CREATE TABLE t1 (a int) PARTITION BY RANGE (a);
CREATE TABLE t1_part PARTITION OF t1 FOR VALUES FROM (1) TO (100);
CREATE PUBLICATION mypub FOR TABLE t1;
CREATE TABLE
CREATE TABLE
CREATE PUBLICATION
Session2:
postgres=# BEGIN;
SELECT * FROM t1;
BEGIN
a
---
(0 rows)
Session3:
postgres=# ALTER TABLE t1 DETACH PARTITION t1_part CONCURRENTLY;
<hanging>
Session4:
postgres=# INSERT INTO t1_part VALUES (5);
SELECT * FROM pg_logical_slot_get_binary_changes('myslot', NULL, NULL,
'proto_version', '1', 'publication_names', 'mypub');
INSERT 0 1
lsn | xid | data
-----+-----+------
(0 rows)
thanks
Shveta
