Re: Support EXCEPT for TABLES IN SCHEMA publications

On Tue, May 26, 2026 at 11:27 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Nisha.
>
> Some review comments for patch v6-0003.
>

Thanks for the review. All comments are addressed in v7. Please find
responses below for a few of the comments.

>
> 3.
>     The <literal>EXCEPT</literal> clause can be used with
> -   <literal>ADD TABLES IN SCHEMA</literal> to exclude specific tables from a
> +   <literal>ADD TABLES IN SCHEMA</literal> and
> +   <literal>SET TABLES IN SCHEMA</literal> to exclude specific tables from a
>     schema-level publication. <literal>EXCEPT</literal> is not supported with
>     <literal>DROP TABLES IN SCHEMA</literal>; instead, dropping a schema from
>     the publication automatically removes all of its associated
>
> 3a.
> This whole description section seems arranged in a confusing way IMO.
> Anyway, it is not all the fault of the current patch. But I don't
> think it should be talking about "SET TABLES IN SCHEMA" here because
> that was all mentioned already in the earlier "third variant"
> paragraph.
>

Right. it seems repeating. Removed "SET TABLES IN SCHEMA" related info.

> ~
>
> 3b.
> That last sentence all about EXCEPT with DROP TABLES IN SCHEMA seems
> redundant to me. It is not allowed by the synopsis, so there is no
> possible confusion about it being supported. Isn't it better to just
> say nothing?
>

Okay, that makes sense. Fixed.

> ~~~
>
> 4b.
> This description about EXCEPT is missing talking about FOR ALL TABLES
> EXCEPT, but IIRC I already reported that in a previous review.
>

Yes, we can handle this in a separate patch.

> ~~~
>
> PublicationDropSchemas:
>
> 12.
> + /*
> + * Collect prexcept rows for tables belonging to this schema before
> + * removing the schema entry.  GetExcludedPublicationTables relies on
> + * is_schema_publication(), which scans pg_publication_namespace; if
> + * this is the last schema in the publication, performDeletion() below
> + * would remove that row and make is_schema_publication() return
> + * false, tripping the assertion.
> + */
>
> What assertion?
>

The assertion is Assert(GetPublication(pubid)->alltables ||
is_schema_publication(pubid)) in GetExcludedPublicationTables().
I’ve trimmed the comment a bit, as it felt slightly over-explained.
~~~~

Please find the updated patch-set v7 attached.

--
Thanks,
Nisha