Re: Skipping schema changes in publication

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.

~

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.

======
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

======
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.

~

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.

======
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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia