Re: Support EXCEPT for TABLES IN SCHEMA publications

A few comments on v25-001:

1)
We can rearrange the comment atop ProcessSchemaExceptTables() to
mention EXCEPT-clause duplicate removal first in accordance with the
code.

2)
+ * list, so that except-table validation below can check for

Just a minor tweak:
so that the except-table validation for pub-included schemas below

3)
+ * Both checks are made here, on the lists the statement supplied, because at
+ * this point the catalog cannot answer either question:
+ *
+ * - CreatePublication() inserts the explicit tables and the EXCEPT tables in
+ * a single command, and a row inserted by the current command is not
+ * visible to a later lookup within that command. So the similar checks in
+ * publication_add_relation() and check_publication_add_relation(), which
+ * read pg_publication_rel, find nothing to complain about.
+ *
+ * - In the partition case the excluded root's row does not exist at all yet,
+ * and those checks only look upward from the relation being added.

Slightly confusing, the problem is that "those checks" in the second
bullet implicitly refers to the checks mentioned only in the first
bullet. Shall we say:

/*
* Both checks are performed here, on the lists supplied by the statement,
* because at this point the catalog cannot answer either question. The
* corresponding checks in publication_add_relation() and
* check_publication_add_relation() rely on pg_publication_rel entries and
* ancestor lookups, neither of which is sufficient in the cases below:
*
* - CreatePublication() inserts the explicit tables and the EXCEPT tables in
* a single command, and rows inserted by the current command are not
* visible to later lookups within the same command.
*
* - In the partition case, the excluded root's row might not yet exist
* because the root may be added to the EXCEPT list later. Detecting such
* contradictions when adding the root to the EXCEPT clause would require
* traversing downward through the partition hierarchy, whereas the
* existing ancestor lookups only traverse upward from the relation being
* added.
*/

But the comment may become too long. I wonder do we even need to
mention these 2 points : 'An excluded relation...' and 'Also reject
any relation..' as the error messages in code are self-explanatory. We
can cut down on those if you agree. Also fell free to reduce above
suggested comment as you see apt.

4)
+ if (except_pubtables != NIL)
+ {
+ List *except_rels;
+
+ except_rels = OpenTableList(except_pubtables);
+
+ /*
+ * Validate that a table is not both explicitly included and
+ * excluded by the schema's EXCEPT clause.
+ */
+ CheckExceptNotInTableList(except_rels, explicitrelids);

We can have an ASSERT to ensure explicitrelids must not be NIL here
i.e. we have not come here post first if-block (if
(stmt->for_all_tables)). This is not a possibility but it is not
vlaidated anywhere.

5)
RelationBuildPublicationDesc()
- /*
- * For a regular table or a root partitioned table, check exclusion on
- * table itself.
- */

Is this intentional to remove this comment? It still makes sense.

6)
+ /*
+ * Only the topmost root of a partition hierarchy can appear in an
+ * EXCEPT clause, so that is where exclusion has to be evaluated.
+ * ancestors is NIL for a partition with a pending DETACH
+ * CONCURRENTLY, in which case there is no root to consult.
+ */
+ if (ancestors != NIL)
+ exceptpuboids = GetRelationExcludedPublications(llast_oid(ancestors));

-
- /*
- * Only the top-most ancestor can appear in the EXCEPT clause.
- * Therefore, for a partition, exclusion must be evaluated at the
- * top-most ancestor.
- */
- exceptpuboids = GetRelationExcludedPublications(last_ancestor_relid);

Also, can we move computing exceptpuboids at its earlier place. This
will avoid showing redundant changes in patch.

thanks
Shveta