On Mon, 16 Mar 2026 at 12:06, Peter Smith <[email protected]> wrote: > > I was away for a couple of weeks leading up to when the EXCEPT TABLE > feature got pushed (commit fd36606), so I missed my chance to review > it. > > Here are some late review comments of code already on master: > > ====== > doc/src/sgml/ref/create_publication.sgml > > 1. > + <para> > + There can be a case where a subscription includes multiple > publications. > + In such a case, a table or partition that is included in one > publication > + and listed in the <literal>EXCEPT TABLE</literal> clause of another is > + considered included for replication. > + </para> > > 1a > IIUC you cannot directly specify a partition in the EXCEPT TABLE list > but a partition still might implicitly be excluded if its root > partition table is excluded. So I felt the wording "and listed in" is > not quite correct since you cannot "list" a partition -- it could be > more like below. > > SUGGESTION: > There can be a case where a subscription includes multiple > publications. In such a case, a table or partition that is included in > one publication but excluded (explicitly or implicitly) by the > <literal>EXCEPT TABLE</literal> clause of another is considered > included for replication.
Modified
> ~
>
> 1b.
> Anyway, this note is talking about *subscriptions* to multiple
> publications, so I felt that it belongs in the CREATE SUBSCRIPTION
> "Notes" section. Or, maybe on both pages.
I felt this talks about publication, so this place is ok. We can do it
like the way you are telling if others also feel that way.
> ======
> src/backend/catalog/pg_publication.c
>
> check_publication_add_relation:
>
> 2.
> + if (pri->except)
> + errormsg = gettext_noop("cannot use publication EXCEPT clause for
> relation \"%s\"");
> + else
> + errormsg = gettext_noop("cannot add relation \"%s\" to publication");
>
> I felt that the first message wording could be improved. e.g. "using
> EXCEPT clause for a relation" sounds backwards. In passing, make it
> more similar to the other (else) message.
>
> SUGGESTION
> cannot specify relation \"%s\" in the publication EXCEPT clause
Modified
> ======
> src/backend/commands/tablecmds.c
>
> ATExecAttachPartition:
>
> 3.
> + bool first = true;
> + StringInfoData pubnames;
> +
> + initStringInfo(&pubnames);
> +
> + foreach_oid(pubid, exceptpuboids)
> + {
> + char *pubname = get_publication_name(pubid, false);
> +
> + if (!first)
> + {
> + /*
> + * translator: This is a separator in a list of publication
> + * names.
> + */
> + appendStringInfoString(&pubnames, _(", "));
> + }
> +
> + first = false;
> +
> + appendStringInfo(&pubnames, _("\"%s\""), pubname);
> + }
>
> 3a.
> Why is appendStringInfo(&pubnames, _("\"%s\""), pubname) using the
> "_(" macro?. AFAIK it does not make sense to call gettext() for a
> pubname.
I had seen that it is implemented similarly in logicalrep_get_attrs_str too:
appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
Amit also has mentioned this at [1].
> ~
>
> 3b.
> Postgres already has a function to make a CSV list of pubnames. Can't
> we build a list of pubnames here, then just call the common
> 'GetPublicationsStr' to get that as a CSV string. PSA a patch
> demonstrating what I mean.
The current implementation loops the publication only once. In the
approach you suggested first you will loop through pubids and get the
pubname then pass it to GetPublicationsStr and loop again. I felt the
existing is better.
> ======
> src/test/subscription/t/037_except.pl
>
> 4.
> +# Exclude tab1 (non-inheritance case), and also exclude parent and ONLY
> parent1
> +# to verify exclusion behavior for inherited tables, including the effect of
> +# ONLY in the EXCEPT TABLE clause.
> +$node_publisher->safe_psql('postgres',
> + "CREATE PUBLICATION tab_pub FOR ALL TABLES EXCEPT TABLE (tab1,
> parent, only parent1)"
> +);
> +
> +# Create a logical replication slot to help with later tests.
> +$node_publisher->safe_psql('postgres',
> + "SELECT pg_create_logical_replication_slot('test_slot', 'pgoutput')");
> +
> +$node_subscriber->safe_psql('postgres',
> + "CREATE SUBSCRIPTION tab_sub CONNECTION '$publisher_connstr'
> PUBLICATION tab_pub"
> +);
> +
>
> Why are those pub/sub prefixed "tab_"? They are not tables.
>
> Note that elsewhere in this same patch tap test there are other
> pub/subs called "tap_pub_part" and "tap_sub_part". So why are some
> "tap_" and some "tab_"?
>
> I guess those ones called "tab_pub" and "tab_sub" look like they've
> been accidentally changed by a global "tab" replacement, or were
> assumed to be typos when they were not.
Modified, additionally updated an incorrect test comment to reflect
that only tables are created, not schemas.
The attached patch has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAA4eK1LLZayJBtrZEMt8PTXdbv_XB14u4PTS4pVBUgEUHPikKQ%40mail.gmail.com
Regards,
Vignesh
0001-Fix-few-issues-in-commit-fd366065e0.patch
Description: Binary data
