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
