Re: Handle infinite recursion in logical replication setup

Below are my review comments for the v9-0002 patch (except I did not
yet look at the TAP tests).

~~~

1. General comment - describe.c

I wondered why the copy_data enum value is not displayed by the psql
\drs+ command. Should it be?

~~~

2. General comment - SUBOPT_LOCAL_ONLY

@@ -1134,7 +1204,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,

case ALTER_SUBSCRIPTION_SET_PUBLICATION:
{
- supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
+ supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH | SUBOPT_LOCAL_ONLY;
parse_subscription_options(pstate, stmt->options,
supported_opts, &opts);

@@ -1236,7 +1308,8 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
subscriptions")));

parse_subscription_options(pstate, stmt->options,
- SUBOPT_COPY_DATA, &opts);
+ SUBOPT_COPY_DATA | SUBOPT_LOCAL_ONLY,
+ &opts);

I noticed there is some new code that appears to be setting the
SUBOT_LOCAL_ONLY as a supported option. Shouldn't those changes belong
in the patch 0001 instead of in this patch 0002?

~~~

3. Commit message - wording

CURRENT
a) Node1 - Publication publishing employee table.
b) Node2 - Subscription subscribing from publication pub1 with
local_only.
c) Node2 - Publication publishing employee table.
d) Node1 - Subscription subscribing from publication pub2 with
local_only.

SUGGESTION (I think below has the same meaning but is simpler)
a) Node1 - PUBLICATION pub1 for the employee table
b) Node2 - SUBSCRIPTION from pub1 with local_only=true
c) Node2 - PUBLICATION pub2 for the employee table
d) Node1 - SUBSCRIPTION from pub2 with local_only=true

~~~

4. Commit message - meaning?

CURRENT
Now when user is trying to add another node Node3 to the
above Multi master logical replication setup:
a) user will have to create one subscription subscribing from Node1 to
Node3
b) user wil have to create another subscription subscribing from
Node2 to Node3 using local_only option and copy_data as true.

While the subscription is created, server will identify that Node2 is
subscribing from Node1 and Node1 is subscribing from Node2 and throw an
error so that user can handle the initial copy data.

~

The wording above confused me. Can you clarify it?
e.g.
a) What exactly was the user hoping to achieve here?
b) Are the user steps doing something deliberately wrong just so you
can describe later that an error gets thrown?

~~~

5. doc/src/sgml/logical-replication.sgml - how to get here?

I didn’t see any easy way to get to this page. (No cross refs from anywhere?)

~~~

6. doc/src/sgml/logical-replication.sgml - section levels

I think the section levels may be a bit messed up. e.g. The HTML
rendering of sections looks a bit confused. Maybe this is same as my
review comment #13.

~~

7. doc/src/sgml/logical-replication.sgml - headings

<title>Setting Bidirection logical replication between two nodes:</title>

7a. Maybe better just to have a simpler main heading like
"Bidirectional logical replication".

7b. Don't put ":" in the title.

~~~

8. doc/src/sgml/logical-replication.sgml - missing intro

IMO this page needs some sort of introduction/blurb instead of leaping
straight into examples without any preamble text to give context.

~~~

9. doc/src/sgml/logical-replication.sgml - bullets

Suggest removing all the bullets from the example steps. (I had
something similar a while ago for the "Row Filter" page but
eventually, they all had to be removed).

~~~

10. doc/src/sgml/logical-replication.sgml - SQL too long

Many of the example commands are much too long, and for me, they are
giving scroll bars when rendered. It would be better if they can be
wrapped in appropriate places so easier to read (and no resulting
scroll bars).

~~~

11. doc/src/sgml/logical-replication.sgml - add the psql prompt

IMO it would also be easier to understand the examples if you show the
psql prompt. Then you can easily know the node context without having
to describe it in the text so often.

e.g.

+ <para>
+ Create the subscription in node2 to subscribe the changes from node1:
+<programlisting>
+CREATE SUBSCRIPTION sub_node1_node2 ...

SUGGGESTION
+ <para>
+ Create the subscription in node2 to subscribe the changes from node1
+<programlisting>
+node_2=# CREATE SUBSCRIPTION sub_node1_node2 ...

~~~

12. doc/src/sgml/logical-replication.sgml - typo

+ <para>
+ Now the BiDirectional logical replication setup is complete between node1

typo "BiDirectional"

~~~

13. doc/src/sgml/logical-replication.sgml - deep levels

The section levels are very deep, but > 3 will not appear in the table
of contents when rendered. Maybe you can rearrange to raise them all
up one level, then IMO the TOC will work better and the whole page
will be easier to read.

~~~

14. doc/src/sgml/logical-replication.sgml - unnecessarily complex?

+<programlisting>
+# Truncate the table data but do not replicate the truncate.
+ALTER PUBLICATION pub_node3 SET (publish='insert,update,delete');
+TRUNCATE t1;
+
+CREATE SUBSCRIPTION sub_node3_node1 CONNECTION 'dbname=foo host=node1
user=repuser' PUBLICATION pub_node1 WITH (copy_data = force,
local_only = on);
+
+CREATE SUBSCRIPTION sub_node3_node2 CONNECTION 'dbname=foo host=node2
user=repuser' PUBLICATION pub_node2 WITH (copy_data = off, local_only
= on);
+
+# Include truncate operations from now
+ALTER PUBLICATION pub_node3 SET (publish='insert,update,delete,truncate');
+</programlisting>

Is it really necessary for those CREATE SUBSCRIPTION to be placed
where they are? I felt it would be less confusing if you just do the
TRUNCATE between the 2x ALTER PUBLICATION... and then do those CREATE
SUBSCRIPTION separately later.

~~~

14. doc/src/sgml/logical-replication.sgml - force?

There seems no documentation anywhere that says what is the
purpose/meaning of the copy_data enum "force".

~~~

15. doc/src/sgml/ref/alter_subscription.sgml - force?

Meanings of copy_data true/false are self-evident but should something
here be explaining what is the meaning of "force"? Or a reference to
some place that explains it?

~~~

16. doc/src/sgml/ref/create_subscription.sgml - local_only missing notes?

@@ -161,6 +161,11 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
publisher node changes regardless of their origin. The default is
<literal>false</literal>.
</para>
+ <para>
+ There is some interation between the "local_only" option and
+ "copy_data" option. Refer to the
+ <xref linkend="sql-createsubscription-notes" /> for details.
+ </para>
</listitem>
</varlistentry>

16a. Typo "interation"

16b. But where in the referenced notes does it actually say anything about this?

~~~

17. doc/src/sgml/ref/create_subscription.sgml - force?

The notes about the copy_data do not say anything about what the
values mean. Specifically, there seems nothing that describes what is
the meaning of "force".

~~~

18. doc/src/sgml/ref/create_subscription.sgml - copy_data missing notes?

+
+ <para>
+ There is some interation between the "local_only" option and
+ "copy_data" option. Refer to the
+ <xref linkend="sql-createsubscription-notes" /> for details.
+ </para>

18a. Typo "interation"

18b. But where in the referenced notes does it actually say anything about this?

~~~

19. doc/src/sgml/ref/create_subscription.sgml - xref to the new page

Shouldn't there be an xref on this page somewhere to your other new
"Bidirectional" docs page?

~~~

20. src/backend/commands/subscriptioncmds.c - IS_COPY_DATA_ON_OR_FORCE

@@ -69,6 +69,18 @@
/* check if the 'val' has 'bits' set */
#define IsSet(val, bits) (((val) & (bits)) == (bits))

+#define IS_COPY_DATA_ON_OR_FORCE(copy_data) (copy_data != COPY_DATA_OFF)

Maybe this would be better as a static inline function?

~~~

21. src/backend/commands/subscriptioncmds.c - comment

+/*
+ * Represents whether copy_data option is specified with on, off or force.
+ */
+typedef enum CopyData
+{
+ COPY_DATA_OFF,
+ COPY_DATA_ON,
+ COPY_DATA_FORCE
+} CopyData;

I felt it might be better if the comment described these enums in the
same order they are defined.

E.g. "Represents whether copy_data option is specified as off/on, or force."

~~~

22. src/backend/commands/subscriptioncmds.c - CreateSubscription bug?

@@ -720,7 +788,8 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
* PENDING, to allow ALTER SUBSCRIPTION ... REFRESH
* PUBLICATION to work.
*/
- if (opts.twophase && !opts.copy_data && tables != NIL)
+ if (opts.twophase && IS_COPY_DATA_ON_OR_FORCE(opts.copy_data)
+ && tables != NIL)
twophase_enabled = true;
Is that a bug? It does not seem to match the previous code. Should
that IS_COPY_DATA_ON_OR_FORCE be "not" ?

~~~

23. src/backend/commands/subscriptioncmds.c - long errmsg

+ if (copydata == COPY_DATA_ON && local_only && !slot_attisnull(slot, 3))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("CREATE/ALTER SUBSCRIPTION with local_only and copy_data as
true is not allowed when the publisher might have replicated data,
table:%s.%s might have replicated data in the publisher",
+ nspname, relname),
+ errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off or force"));
+

The errmsg seems way too long for the source code. Can you use string
concatenation or continuation chars to wrap the message over multiple
lines?

~~~

24. src/test/regress/sql/subscription.sql - typo

@@ -66,6 +69,15 @@ ALTER SUBSCRIPTION regress_testsub4 SET (local_only = false);
DROP SUBSCRIPTION regress_testsub3;
DROP SUBSCRIPTION regress_testsub4;

+-- ok - valid coy_data options

Typo "coy_data". (it looks like this typo is not caused by this patch,
but I think this patch should fix it anyhow).

~~~

25. src/test/regress/sql/subscription.sql - test order

The new tests are OK but IMO they could be re-ordered so then they
will be more consistent for the positive and negative tests.

CURRENT
"true/force/on/1" and "off/false/0"

SUGGEST
"true/on/1/force" and "false/off/0"

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