Re: Support EXCEPT for TABLES IN SCHEMA publications

On Mon, Jul 13, 2026 at 3:31 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Jul 10, 2026 at 5:07 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> 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).

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?

> 2)
> GetAllSchemaPublicationRelations
>
> + if (!excluded && get_rel_relispartition(relid))
> + {
> + List *ancestors = get_partition_ancestors(relid);
> + ListCell *alc;
> +
> + foreach(alc, ancestors)
> + {
> + if (list_member_oid(except_relids, lfirst_oid(alc)))
> + {
> + excluded = true;
> + break;
> + }
> + }
> + list_free(ancestors);
> + }
>
> Should we check root only in except_relids instead of complete ancestor chain?
>

yes, corrected.

> 3)
> postgres=# ALTER TABLE s2.tab_root_new ATTACH PARTITION s2.tab_root
> FOR VALUES FROM (1) TO (2000);
> ERROR: 55000: cannot attach table "tab_root" as partition because it
> is referenced in publication "pub1" EXCEPT clause
> DETAIL: The publication EXCEPT clause cannot contain tables that are
> partitions.
> HINT: Change the publication's EXCEPT clause using ALTER PUBLICATION
> ... SET ALL TABLES.
> LOCATION: ATExecAttachPartition, tablecmds.c:21107
>
> pub1 is a schmea publication table here, I think HINT needs some adjustment.
>

Fixed.

> 4)
> + root_tup = SearchSysCache2(PUBLICATIONRELMAP,
> + ObjectIdGetDatum(llast_oid(ancestors)),
> + ObjectIdGetDatum(pub->oid));
> + if (HeapTupleIsValid(root_tup))
> + {
> + bool root_except = ((Form_pg_publication_rel) GETSTRUCT(root_tup))->prexcept;
> +
> + ReleaseSysCache(root_tup);
> + if (root_except)
> + return false;
> + }
>
> Above code snipet appears a lot many times either for relid or
> llast_oid(ancestors), can we replace this with a common function like:
>
> bool lookup_pub_relmap(Oid pubid, Oid relid, bool *is_except); (or
> get_publication_rel_entry or anything which you find better)
>
> The return value will indicate when the relid is present. 'is_except'
> will indicate whetehr it is marked with except flag or not. Then above
> can be replaced with:
>
> if (lookup_pub_relmap(pub->oid, llast_oid(ancestors), &root_except)
> && root_except)
> return false;
>
> Do this only if you feel it will simplify the code and apply it where it helps.
>

You're right. The same check appears in five places. I replaced it
with a helper function, get_publication_rel_entry(), which
significantly reduces code duplication.
~~~

Attached is the v21 patch set addressing the above changes, along with
all other comments for v20 at [1], [2], and [3].

[1] https://www.postgresql.org/message-id/CAJpy0uCFCdL97L3Pt8f586br5irqkzCa_i3tXMjH6JFCPxxr4Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPuJE947Y3WPo%2BcaEpEojQdRXryS2KjBLBr12rb9RQ95Zw%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAHut%2BPsGg7Pi2CM5eBjb8s6FCPb5XmyWaBUxPrDSzt_2wK4Gfg%40mail.gmail.com

--
Thanks,
Nisha