Re: Support EXCEPT for TABLES IN SCHEMA publications

On Wed, Jul 15, 2026 at 10:19 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for v20-0001.
>

Thanks for review. All suggestions are incorporated. Please find
responses for few below.

> ~~~
>
> 6.
> + /*
> + * 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 own schema is separately included.
> + */
>
> When you say "the partition's own schema", it seems to be viewed
> through the lens of FOR TABLES IN SCHEMA, but the partition could also
> be explicitly included (e.g. FOR TABLE xxx).
>
> IOW, does "own schema" need to be mentioned, or can that last sentence
> say "regardless of whether the partition is separately included".
>

I think it is better to view this through the lens of FOR TABLES IN
SCHEMA, since a partition cannot be included by any other means if its
root is excluded.
My intent with this comment is to clarify that a partition should not
be assumed to be included simply because its schema is included --
hence the additional check.

I reworded it as: "regardless of whether the partition's schema is
separately included".

> ~~~
>
> 7.
> + *
> + * A pg_publication_rel row with prexcept=true means the table is
> + * explicitly excluded via EXCEPT and must not be reported as published,
> + * even if its schema is otherwise included. A row with prexcept=false
> + * means it is explicitly included. If no pg_publication_rel row exists,
> + * the table is published iff its schema appears in
> + * pg_publication_namespace.
>
> Again, this comment seems viewed the the lens of FOR TABLES IN SCHEMA.
>
> That last part, ("If no pg_publication_rel row exists, the table is
> published iff its schema appears in pg_publication_namespace."), seems
> not quite correct. For example, a table can be published by a FOR ALL
> TABLES publication, which has nothing in pg_publication_namespace at
> all.
>

IIUC, we reach this code only for non-FOR ALL TABLES publications,
since the pub->alltables case is handled before reaching here.
Therefore, the pointed "last part" of comment holds true. Please
correct me if I'm missing another scenario where this would not be the
case.

>
> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> get_rel_sync_entry:
>
> 10.
> + /*
> + * GetTopMostAncestorInPublication() itself skips
> + * schema-based ancestor matches when the partition root
> + * is excluded via this publication's EXCEPT clause.
> + */
> ancestor = GetTopMostAncestorInPublication(pub->oid,
> ancestors,
> &level);
>
> I'm not sure if that comment is helpful here or not. Why is the caller
> describing the inner logic of the function it is calling?

I missed removing the comment during the recent code optimization.
It's removed now.

>
> ~~~
> ~~~
>
> 12.
> + /*
> + * schemaPubids is relid's own schema, independent of the
> + * ancestor walk above -- a partition can live in a
> + * different schema than its root. Still need to check
> + * for exclusion here, using the top-most ancestor since
> + * only a root (non-partition) table can appear in an
> + * EXCEPT clause.
> + */
>
> I don't know what "schemaPubids is relid's own schema" means. AFAIK
> the `schemaPubids` was a list of publications this schemas is a member
> of (??).
>

Correct. Reworded the comment as: "schemaPubids is the list of
publications that include relid's schema..."

> ======
> src/test/regress/sql/publication.sql
>
> 13.
> +-- fail: partition still rejected from EXCEPT even when its own root is also
> +-- named in the same EXCEPT list
> +CREATE PUBLICATION testpub_except_partition_with_root
> + FOR TABLES IN SCHEMA pub_test EXCEPT (TABLE
> pub_test.testpub_part_s, TABLE pub_test.testpub_parted_s);
> +
>
> Is this test needed? Isn't the previous test ("only root tables are
> allowed") covering this?
>

Okay, makes sense. I've removed it.

> Alternatively, consider to put the partition in a different schema
> (e.g. see the review comment #15 just below)

Added a test case covering the cross-schema failure scenario.

>
> ~~~
>
> 15.
> +-- test for EXCEPT clause with schema publication, where the excluded root's
> +-- partition lives in a different schema that is separately, fully included
> +-- (no EXCEPT) in the same publication. The root's exclusion must cascade to
> +-- the partition regardless of the partition's own schema membership.
> +CREATE SCHEMA gpt_cross_sch1;
> +CREATE SCHEMA gpt_cross_sch2;
> +CREATE TABLE gpt_cross_sch1.croot (id int) PARTITION BY RANGE (id);
> +CREATE TABLE gpt_cross_sch2.cpart1 PARTITION OF gpt_cross_sch1.croot
> FOR VALUES FROM (1) TO (10);
> +
> +SET client_min_messages = 'ERROR';
> +CREATE PUBLICATION pub_cross_schema_except FOR TABLES IN SCHEMA
> gpt_cross_sch1 EXCEPT (TABLE gpt_cross_sch1.croot), TABLES IN SCHEMA
> gpt_cross_sch2;
>
> IIUC, a different way to do this test would be to name the partition explicitly:
> CREATE PUBLICATION pub_cross_schema_except FOR TABLES IN SCHEMA
> gpt_cross_sch1 EXCEPT (TABLE gpt_cross_sch1.croot), TABLE
> gpt_cross_sch2.cpart1;
>
> Is it worth adding this way as well?
>

This test is intended to verify pg_get_publication_tables() for the
case where a partition's schema is included, but the partition itself
should not appear in the publication.

The suggested command would not cover that code path. That said, I
agree that explicitly adding such a partition should also fail, so
I've added a failure test under the "EXCEPT tests for TABLES IN
SCHEMA" section.

--
Thanks,
Nisha