Re: Support EXCEPT for TABLES IN SCHEMA publications

On Mon, Jul 20, 2026 at 3:47 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Jul 20, 2026 at 1:53 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> >
> > Updated as suggested in v22.
> > I think patch 002 is now in good shape to be merged into patch 001. Thoughts?
> >
>
> I will check it soon. A few comments on 001:
>

Okay, thanks!

>
> 3)
> +/*
> + * Check whether a pg_publication_rel row exists for (pubid, relid); if so,
> + * set *is_except to its prexcept flag.
> + */
> +bool
> +GetPublicationRelEntry(Oid pubid, Oid relid, bool *is_except)
>
> It is only checking (returning bool) and not getting the pg_pub_rel
> row as such. Shall we name it as CheckPublicationRelEntry or
> CheckPubRelEntry?
>

CheckPublicationRelEntry looks good to me. Renamed accordingly.

> 4)
> Are the changes in GetAllPublicationRelations() intentional? We are
> not changing any logic there though. So we can avoid touching this
> function.
>

The changes were done as part of previous comment to make the
list/vars names used for EXCEPT specific objects consistent across
code. But yes I agree GetAllPublicationRelations is untouched in the
patch, so should leave those as it is. Fixed.

>
> 7)
> Currently is_table_publishable_in_publication() has:
> ----------------------------------
> /*
> * If an ancestor is published, the partition's status depends on
> * publish_via_partition_root value.
> *
> * If it's true, the ancestor's relation OID is the effective published
> * OID, so the partition itself should be excluded (return false).
> *
> * If it's false, the partition is covered by its ancestor's presence in
> * the publication, it should be included (return true).
> */
> if (relispartition)
> {
> if (OidIsValid(GetTopMostAncestorInPublication(pub->oid, ancestors, NULL)))
> return !pub->pubviaroot;
>
> /*
> * GetTopMostAncestorInPublication() returns InvalidOid both when no
> * ancestor matched at all, and when the root is excluded via EXCEPT.
> * Disambiguate here: if the root is excluded, the partition is never
> * published through this publication, regardless of whether the
> * partition's schema is separately included. ancestors can be NIL
> * for a partition whose DETACH ... CONCURRENTLY is pending, in which
> * case there is no ancestor to check.
> */
> if (ancestors != NIL &&
> GetPublicationRelEntry(pub->oid, llast_oid(ancestors), &is_except) &&
> is_except)
> return false;
> }
> ----------------------------------
>
> Shall we change this to:
>
> /*
> * If the partition root is excluded via the EXCEPT clause, the partition
> * is never considered to be published through this publication. Check this
> * before checking whether an ancestor is published.
> *
> * If an ancestor is published, the partition's status depends on
> *....rest of the comment here...
> */
> if (relispartition)
> {
> /*
> * ancestors can be NIL for a partition whose DETACH ... CONCURRENTLY is
> * pending, in which case there is no ancestor to check.
> */
> if (ancestors != NIL &&
> GetPublicationRelEntry(pub->oid, llast_oid(ancestors), &is_except) &&
> is_except)
> return false;
>
> if (OidIsValid(GetTopMostAncestorInPublication(pub->oid, ancestors, NULL)))
> return !pub->pubviaroot;
> }
>
>
> The benefit of above apporach is the bigger comment saying
> 'Disambiguate here' is not needed. That makes it complex to
> understand. Plus if ROOT is excluded, we need not to execute ROOT
> exlcusion check twice as you were doing earlier, one through
> GetTopMostAncestorInPublication and one outside it. But let me know if
> you see any issue in above changes.
>

The suggestion makes sense to me. I've updated the code accordingly.
~~~

Attached v23 patch set addressing all of the above comments, as well
as the comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CAJpy0uD%2BcODgXzM%3DyQFCVMDx3bPT18wiOUtEMTNvtSkzLUKnGw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPt_G%3DUm7vpY_eJLknOKVzcOrfY6ADzwcmfy09ZsG0sRTA%40mail.gmail.com
--
Thanks,
Nisha