Re: Handle infinite recursion in logical replication setup

On Thu, May 26, 2022 at 7:06 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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?

Separated the test as a 0001 patch.

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

Modified

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

Modified

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

Modified

> ~~~
>
> 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 */

Modified

> ~~~
>
> 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"

Modified

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

Modified

> 6c.
> +# check that bidirectional logical replication setup...
>
> Start comment sentence with upper case.

Modified

> 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

Modified

> 6e.
> +is($result, qq(11
> +12
> +13), 'Node_C data replicated to Node_B'
> +);
>
> SUGGESTION for message
> 'node_C data replicated to node_B'

Modified

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

Modified

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

Modified

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

This test was added to handle the comment as in [1]

Thanks for the comments, the attached v17 patch has the fixes for the same.
I have also separated the bidirectional logical replication steps,
since the steps are slightly complex and keeping it separate will help
in easier review and modifying.
[1] - https://www.postgresql.org/message-id/CAHut%2BPvwqwzuoQio-hJniarDUOTyxFrwrS9hucd47gEW-9wu-g%40mail.gmail.com

Regards,
Vignesh