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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0ecf835ffaa..b1ed780c55a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -50,6 +50,7 @@
#include "catalog/pg_publication_rel.h"
#include "catalog/pg_rewrite.h"
#include "catalog/pg_statistic_ext.h"
+#include "catalog/pg_subscription.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_trigger.h"
#include "catalog/pg_type.h"
@@ -20699,28 +20700,15 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd,
exceptpuboids =
GetRelationExcludedPublications(RelationGetRelid(attachrel));
if (exceptpuboids != NIL)
{
- bool first = true;
+ List *pubs = NIL;
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, _(", "));
- }
+ pubs = lappend(pubs,
makeString(get_publication_name(pubid, false)));
- first = false;
+ initStringInfo(&pubnames);
- appendStringInfo(&pubnames, _("\"%s\""), pubname);
- }
+ GetPublicationsStr(pubs, &pubnames, false);
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
