Re: Support EXCEPT for TABLES IN SCHEMA publications

On Thu, Jul 23, 2026 at 5:26 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
>
> Attached v23 patch set addressing all of the above comments, as well
> as the comments in [1] and [2].
>

A few comments on v23-001:

1)
+ if (CheckPublicationRelEntry(pubid, root, &root_except) && root_except)
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("cannot add partition \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),

Is the errorcode "ERRCODE_DUPLICATE_OBJECT" intentional? To me it does
not look duplicate object as ROOT was in EXCEPT while this
we are trying to add to inclusion list. Other similar places have
different error, see:

+ if (exceptrelid == explicitrelid)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s\" cannot be both published and excluded",
+ RelationGetQualifiedRelationName(pri->relation)));

2)
+ * Also reject any relation in explicitrelids whose partition root is in
+ * except_rels. Otherwise the catalog would be inconsistent (root excluded,
+ * partition explicitly included), and the ancestor-cascade rule would
+ * silently override the include at replication time.

Shall we simply say:
/*
* Also reject any relation in explicitrelids whose partition root is in
* except_rels. Excluding a partition root means that the entire partition
* tree is excluded, so a partition cannot be explicitly added to the
* publication if its partition root is excluded.
*/

3)
Shall we change this too
+ errdetail("The table is currently named in the EXCEPT clause of the
publication."),
similar to:
+ errdetail("Partition root \"%s\" is named in the publication's
EXCEPT clause for schema \"%s\".",

4)
+
+ /*
+ * ancestors is NIL for a partition with a pending DETACH
+ * CONCURRENTLY.
+ */
+ if (ancestors != NIL)
+ root = llast_oid(ancestors);

I see non-null ancestors check at some places while other places do it
directly, see:
+ root = llast_oid(get_partition_ancestors(relid));

is it intentional?

5)
+ list_free(ancestors);

This is also done at some places, while missed at other places.

thanks
Shveta