On Fri, Jul 24, 2026 at 11:54 AM Peter Smith <[email protected]> wrote:
>
> Some review comments for the v23* patches
>
Thanks for the review. I've incorporated most of the suggestions in
v24. Please see my responses to a few below.
> //////////
> Patch 0001
> //////////
>
> src/backend/catalog/pg_publication.c
>
> is_table_publication:
>
> 2.
> /*
> - * Returns true if the publication has explicitly included relation (i.e.,
> + * Returns true if the publication has an explicitly-included relation (i.e.,
> * not marked as EXCEPT).
> */
>
> In hindsight, the function comment seems a bit odd. It doesn't need to
> mention EXCEPT -- those are more like logic details about *how* it
> works, nothing to do with the purpose of the function.
>
> SUGGESTION
> Returns true if the publication has a FOR TABLE clause.
>
Since this function is being modified, updating the comment makes sense. Done.
> ~
>
> Similarly, the `is_schema_publication` function comment could simply say:
> Returns true if the publication has a FOR TABLES IN SCHEMA clause.
>
This patch doesn't touch this function, so I'm not sure unrelated
comments should be updated here.
> ~~~
>
> GetAllSchemaPublicationRelations:
>
> 3.
> + if (except_relids != NIL)
> + {
> + /* filter out any tables that appear in the EXCEPT clause */
>
> But `except_relids` really is a List so I think the comment saying
> "EXCEPT list" was probably fine here.
>
Fixed.
> ~~~
>
> is_table_publishable_in_publication:
>
> 5.
> /*
> * Check whether the table is explicitly published via pg_publication_rel
> * or pg_publication_namespace.
> + *
> + * 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.
> */
> - return (SearchSysCacheExists2(PUBLICATIONRELMAP,
> - ObjectIdGetDatum(relid),
> - ObjectIdGetDatum(pub->oid)) ||
> - SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP,
> - ObjectIdGetDatum(get_rel_namespace(relid)),
> - ObjectIdGetDatum(pub->oid)));
> + if (CheckPublicationRelEntry(pub->oid, relid, &is_except))
> + return !is_except;
> +
> + return SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP,
> + ObjectIdGetDatum(get_rel_namespace(relid)),
> + ObjectIdGetDatum(pub->oid));
>
> I think that the last `if` would be better written as if/else. That's
> because your big comment applies to *both* of those returns, but it is
> not immediately obvious due to the blank line.
>
Done.
> ======
> src/backend/commands/publicationcmds.c
>
> AlterPublication:
>
> 6.
> Code was moved out of patch 0001.
>
> TBH, I thought this could've stayed in patch 0001 -- Although code was
> in the AlterPublication function, AFAICT it had nothing really to do
> with "alter" patch -- Isn't it more related just to the signature
> change of `ObjectsInPublicationToOids` which is elsewhere in here
> patch 0001?
I think it's fine to move the ALTER-related change into the respective
patch. It should make the review easier.
> //////////
> Patch 0003
> //////////
>
> src/backend/commands/publicationcmds.c
>
> 1.
> - List *exceptrelations = NIL;
> + List *except_pubtables = NIL;
> List *schemaidlist = NIL;
> Oid pubid = pubform->oid;
>
> ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
> - &exceptrelations, &schemaidlist);
> + &except_pubtables, &schemaidlist);
>
>
> (same as patch 0001 review comment #6)
>
> Wondering if this change belonged in patch 0001, which is where
> ObjectsInPublicationToOids signature was modified.
>
As above, I feel keeping it here makes it easier for reviewers to
follow the changes.
>
> //////////
> Patch 0005
> //////////
>
> There's a couple of threads [1][2] which might get pushed soon. Those
> are going to mean a rebase will be needed, so watch out for them.
>
> doc/src/sgml/logical-replication.sgml
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> + <varlistentry>
> + <term><literal>EXCEPT</literal></term>
> + <listitem>
> + <para>
> + When used with <literal>ADD TABLES IN SCHEMA</literal>
> + or <literal>SET TABLES IN SCHEMA</literal>, specifies
> + tables to be excluded from the publication. Each named
> + table must belong to the schema specified in the same
> + <literal>TABLES IN SCHEMA</literal> clause. Table names may be
> + schema-qualified or unqualified; unqualified names are implicitly
> + qualified with the schema named in the same clause. See
> + <xref linkend="sql-createpublication"/> for further details on the
> + semantics of <literal>EXCEPT</literal>.
> + </para>
> + <para>
> + Dropping a table always removes it from the <literal>EXCEPT</literal>
> + clause.
> + </para>
> + </listitem>
> + </varlistentry>
>
> Curiously, this does not mention FOR ALL TABLES EXCEPT?
>
> I felt there was a missing earlier paragraph "When used with ALL TABLES ..."?
>
The FOR ALL TABLES EXCEPT semantics are already documented in the
Description, as discussed in [1]. I'm not sure we need to repeat them
here.
If we decide a separate paragraph is needed for FOR ALL TABLES, we can
settle that in [1]. I'd prefer to keep this patch limited to FOR
TABLES IN SCHEMA.
[1]
https://www.postgresql.org/message-id/CABdArM4L5JhUa_PB3TNb0zbor8PpAVX1H3qD%2B6XC3t9TGeJi0w%40mail.gmail.com
--
Thanks,
Nisha
