Re: Handle infinite recursion in logical replication setup

Here are my review comments for patch v15-0001.

======

1. Commit message

Should this also say new test cases were added for the test_decoding plugin?

~~~

2. contrib/test_decoding/sql/replorigin.sql

@@ -119,3 +119,18 @@ SELECT data FROM
pg_logical_slot_get_changes('regression_slot_no_lsn', NULL, NUL
SELECT pg_replication_origin_session_reset();
SELECT pg_drop_replication_slot('regression_slot_no_lsn');
SELECT pg_replication_origin_drop('regress_test_decoding:
regression_slot_no_lsn');
+
+-- Verify that remote origin data is not returned with only-local option
+SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot_only_local',
'test_decoding');
+SELECT pg_replication_origin_create('regress_test_decoding:
regression_slot_only_local');
+SELECT pg_replication_origin_session_setup('regress_test_decoding:
regression_slot_only_local');
+INSERT INTO origin_tbl(data) VALUES ('only_local, commit1');
+-- remote origin data returned when only-local option is not set
+SELECT data FROM
pg_logical_slot_get_changes('regression_slot_only_local', NULL, NULL,
'skip-empty-xacts', '1', 'include-xids', '0', 'only-local', '0');
+INSERT INTO origin_tbl(data) VALUES ('only_local, commit2');
+-- remote origin data not returned when only-local option is set
+SELECT data FROM
pg_logical_slot_get_changes('regression_slot_only_local', NULL, NULL,
'skip-empty-xacts', '1', 'include-xids', '0', 'only-local', '1');
+-- Clean up
+SELECT pg_replication_origin_session_reset();
+SELECT pg_drop_replication_slot('regression_slot_only_local');
+SELECT pg_replication_origin_drop('regress_test_decoding:
regression_slot_only_local');

All the new comments should consistently start with upper case.

~~~

3. doc/src/sgml/catalogs.sgml

+ <para>
+ If true, the subscription will request that the publisher send locally
+ originated changes. False indicates that the publisher sends any changes
+ regardless of their origin.
+ </para></entry>

SUGGESTION
If true, the subscription will request the publisher to only send
changes that originated locally.

~~~

4. doc/src/sgml/ref/create_subscription.sgml

+ <para>
+ Specifies whether the subscription will request the publisher to send
+ locally originated changes at the publisher node, or send any
+ publisher node changes regardless of their origin. The default is
+ <literal>false</literal>.
+ </para>

This wording should be more similar to the same information in catalogs.sgml

SUGGESTION
Specifies whether the subscription will request the publisher to only
send changes that originated locally, or to send any changes
regardless of origin.

~~~

5. src/include/replication/walreceiver.h

@@ -183,6 +183,7 @@ typedef struct
bool streaming; /* Streaming of large transactions */
bool twophase; /* Streaming of two-phase transactions at
* prepare time */
+ bool only_local; /* publish only locally originated data */
} logical;
} proto;
} WalRcvStreamOptions;

SUGGESTION
/* Publish only local origin data */

~~~

6. src/test/subscription/t/032_onlylocal.pl - cosmetic changes

6a.
+# Setup a bidirectional logical replication between Node_A & Node_B

SUGGESTION
"... node_A and node_B"

6b.
+is(1, 1, "Circular replication setup is complete");

SUGGESTION (or maybe saying "circular" is also OK - I wasn't sure)
"Bidirectional replication setup is complete"

6c.
+# check that bidirectional logical replication setup...

Start comment sentence with upper case.

6d.
+###############################################################################
+# check that remote data that is originated from node_C to node_B is not
+# published to node_A
+###############################################################################

SUGGESTION
Check that remote data of node_B (that originated from node_C) is not
published to node_A

6e.
+is($result, qq(11
+12
+13), 'Node_C data replicated to Node_B'
+);

SUGGESTION for message
'node_C data replicated to node_B'

6f.
+is($result, qq(11
+12), 'Remote data originated from other node is not replicated when
only_local option is ON'
+);

SUGGESTION for message
'Remote data originating from another node (not the publisher) is not
replicated when only_local option is ON'

6g.
"Circular replication setup is complete"
'Inserted successfully without leading to infinite recursion in
bidirectional replication setup'
'Inserted successfully without leading to infinite recursion in
bidirectional replication setup'
'Node_C data replicated to Node_B'
'Remote data originated from other node is not replicated when
only_local option is ON'

Why do some of the "is" messages have single quotes and others have
double quotes? Should be consistent.

~~~

7. src/test/subscription/t/032_onlylocal.pl

+my $appname_B2 = 'tap_sub_B2';
+$node_B->safe_psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tap_sub_B2
+ CONNECTION '$node_C_connstr application_name=$appname_B2'
+ PUBLICATION tap_pub_C
+ WITH (only_local = on)");
+

AFAIK the "WITH (only_local = on)" is unnecessary here. We don't care
where the node_C data came from for this test case.

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