On Tue, Apr 19, 2022 at 8:29 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> I checked the latest v9-0001 patch. Below are my review comments.
>
> Other than these few trivial comments this 0001 patch looks good to me.
>
> ~~~
>
> 1. src/backend/replication/pgoutput/pgoutput.c - whitespace
>
> @@ -1696,6 +1714,10 @@ static bool
> pgoutput_origin_filter(LogicalDecodingContext *ctx,
> RepOriginId origin_id)
> {
> + PGOutputData *data = (PGOutputData *) ctx->output_plugin_private;
> +
> + if (data->local_only && origin_id != InvalidRepOriginId)
> + return true;
> return false;
> }
>
> Suggest to add a blank line after the return true;
Modified
> ~~~
>
> 2. src/bin/psql/tab-complete.c - not alphabetical
>
> @@ -1874,7 +1874,7 @@ psql_completion(const char *text, int start, int end)
> COMPLETE_WITH("(", "PUBLICATION");
> /* ALTER SUBSCRIPTION <name> SET ( */
> else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
> TailMatches("SET", "("))
> - COMPLETE_WITH("binary", "slot_name", "streaming",
> "synchronous_commit", "disable_on_error");
> + COMPLETE_WITH("binary", "slot_name", "streaming", "local_only",
> "synchronous_commit", "disable_on_error");
>
> 2a. AFAIK the code intended that these options be listed in
> alphabetical order (I think the recent addition of disable_on_error is
> also wrong here). So "local_only" should be moved.
Modified
> @@ -3156,7 +3156,7 @@ psql_completion(const char *text, int start, int end)
> /* Complete "CREATE SUBSCRIPTION <name> ... WITH ( <opt>" */
> else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
> COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
> - "enabled", "slot_name", "streaming",
> + "enabled", "slot_name", "streaming", "local_only",
> "synchronous_commit", "two_phase", "disable_on_error");
>
> 2b. ditto
Modified
> ~~~
>
> 3. src/test/subscription/t/032_localonly.pl - wrong message
>
> +$node_C->wait_for_catchup($appname_B2);
> +$node_B->wait_for_catchup($appname_A);
> +
> +$result = $node_B->safe_psql('postgres', "SELECT * FROM tab_full;");
> +is( $result, qq(11
> +12
> +13),
> + 'Inserted successfully without leading to infinite recursion in
> circular replication setup'
> +);
> +
> +# check that the data published from node_C to node_B is not sent to node_A
> +$result = $node_A->safe_psql('postgres', "SELECT * FROM tab_full;");
> +is( $result, qq(11
> +12),
> + 'Inserted successfully without leading to infinite recursion in
> circular replication setup'
> +);
> +
>
> The new test looked good, but the cut/paste text message ('Inserted
> successfully without leading to infinite recursion in circular
> replication setup') maybe needs changing because there is nothing
> really "circular" about this test case.
Modified
Attached v10 patch has the changes for the same.
Regards,
Vignesh
