Re: Support EXCEPT for TABLES IN SCHEMA publications

On Wed, 5 Aug 2026 at 14:30, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> A few comments on v25-001:
>
> 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.
> */

Modified

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

I agree with you to remove this as it seemed obvious from the error message.

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

Added an assert for this in the beginning of "else if
(!stmt->for_all_sequences)" block to validate this.

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

I felt these comments should be there now also and added it back.

Apart from these, the rest of the comments are also fixed in the v26
version attached.
Additionally Peter's comments from [1] and Shveta's comments from [2]
to include an additional test are also addressed in this version.

[1] - https://www.postgresql.org/message-id/CAHut%2BPuMvpk-R1uv9dTiiLvDGfKuYBm1KaZP8%2B5xFNzxY9h8Pg%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAJpy0uCdp%3D5q2kW2KMYJ%2B2C3SQnPSpr5gn%2Be3J6jr_V-sjuTBg%40mail.gmail.com

Regards,
Vignesh