Re: Support EXCEPT for TABLES IN SCHEMA publications

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:

1)
+ /*
+ * A partition cannot be included when its partition root is excluded
+ * by the same publication. Doing so would leave the catalog
+ * inconsistent (root excluded, partition included) and the
+ * ancestor-cascade rule would silently override the include at
+ * replication time.
+ *
+ * For partition hierarchies, only the partition root can be in an
+ * EXCEPT clause (a partition itself is rejected just above), so we
+ * only need to look up the catalog for the root.
+ */

Instead of mentioning the reason, can we simply mention below.

/*
* A partition cannot be added to a publication if its partition root is
* excluded by the publication. We follow the rule that excluding a
* partition root means that the entire partition tree is excluded. Thus,
* check whether the root is in the EXCEPT clause and reject adding the
* individual partition to the publication.
*/

2)

+ /*
+ * A publication can hold both explicitly-included (prexcept=false) and
+ * EXCEPT (prexcept=true) rows for the same pubid. Scan until we find an
+ * explicitly-included row, rather than relying on the first row's
+ * prexcept value.
+ */

Good to add one example to give clarity on the case here. Suggestion.

/*
* A publication can have both explicitly included (prexcept = false) and
* EXCEPT (prexcept = true) rows for the same pubid. For example:
*
* CREATE PUBLICATION pub1 FOR TABLE s1.t1,
* TABLES IN SCHEMA s2 EXCEPT (TABLE s2.t1);
*
* Scan until we find an explicitly included row.
*/

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?

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

5)
GetAllSchemaPublicationRelations:
+ if (ancestors != NIL)
+ excluded = list_member_oid(except_relids,
+ llast_oid(ancestors));
+ list_free(ancestors);

Since we are already checking 'if (ancestors != NIL)', we can move
'list_free(ancestors)' also inside that if-block.

6)
+ }
+ list_free(schemaRels);

We an leave a blank line before list_free for better readibility.

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.

thanks
Shveta