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:
>
>
> 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.
>

A few more comments:

8)
CreatePublication:
+ /*
+ * Collect explicit table OIDs now, before we close the relation
+ * list, so that except-table validation below can check for
+ * contradictions without relying on a catalog scan that might not
+ * yet see the just-inserted rows.
+ */
+ if (except_pubtables != NIL)
+ {
+ foreach_ptr(PublicationRelInfo, pri, rels)
+ explicitrelids = lappend_oid(explicitrelids,
+ RelationGetRelid(pri->relation));
+ }
+
PublicationAddTables(puboid, rels, true, NULL);

Shall we make the explicitrelids collection after
'PublicationAddTables', as PublicationAddTables may error out too
while doing 'check_publication_add_relation'.

9)
In the check above, IIUC, except_pubtables can be there even for ALL
TABLES, but we want this handling only for schema-except list, so
shall we have the check as:

if (schemaidlist != NIL && except_pubtables != NIL)

10)
+ /*
+ * Validate that each excluded table is not also in the
+ * explicit table list (which would be contradictory), and
+ * that no explicit partition's root is in the except list.
+ * Use the in-memory explicitrelids collected above rather
+ * than re-reading the catalog, which may not yet see the
+ * just-inserted rows.
+ */

Can we please simplify this comment. The function
CheckExceptNotInTableList() already explains it in detail inside. Also
catalog-rows not visible is already explained by a comment where we
collect the relids.

/*
* Validate that a table is not both explicitly included and excluded by
* the schema's EXCEPT clause.
*/

11)
The changes in AlterPublication (variable name change) if really
needed can be shifter to alter-pub patch.

thanks
Shveta