Re: Handle infinite recursion in logical replication setup

On Mon, Jul 11, 2022 at 9:11 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are my review comments for the v30* patches:
>
> ========
> v30-0001
> ========
>
> 1.1 <general>
>
> I was wondering if it is better to implement a new defGetOrigin method
> now instead of just using the defGetString to process the 'origin',
> since you may need to do that in future anyway if the 'origin' name is
> planned to become specifiable by the user. OTOH maybe you prefer to
> change this code later when the time comes. I am not sure what way is
> best.

I preferred to do that change when the feature is getting extended.

> ~~~
>
> 1.2. src/include/replication/walreceiver.h
>
> @@ -183,6 +183,8 @@ typedef struct
> bool streaming; /* Streaming of large transactions */
> bool twophase; /* Streaming of two-phase transactions at
> * prepare time */
> + char *origin; /* Only publish data originating from the
> + * specified origin */
> } logical;
> } proto;
> } WalRcvStreamOptions;
>
> Should the new comments be aligned with the other ones?

I kept it like this as pgindent also is aligning it as the current code.

> ========
> v30-0002
> ========
>
> 2.1 doc/src/sgml/ref/alter_subscription.sgml
>
> + <para>
> + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
> + <literal>force</literal> for <literal>copy_data</literal> parameter
> + and its interaction with the <literal>origin</literal> parameter.
> </para>
>
> IMO it's better to say "Refer to the Notes" or "Refer to CREATE
> SUBSCRIPTION Notes" instead of just "Refer Notes"

Modified to "Refer to the Notes"

> ~~~
>
> 2.2 doc/src/sgml/ref/create_subscription.sgml
>
> 2.2.a
> + <para>
> + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
> + <literal>force</literal> for <literal>copy_data</literal> parameter
> + and its interaction with the <literal>origin</literal> parameter.
> + </para>
>
> IMO it's better to say "Refer to the Notes" (same as other xref on
> this page) instead of "Refer Notes"

Modified to "Refer to the Notes"

> 2.2.b
> @@ -316,6 +324,11 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
> publisher sends changes regardless of their origin. The default is
> <literal>any</literal>.
> </para>
> + <para>
> + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
> + <literal>force</literal> for <literal>copy_data</literal> parameter
> + and its interaction with the <literal>origin</literal> parameter.
> + </para>
>
> Ditto

Modified to "Refer to the Notes"

> ~~~
>
> 2.3 src/backend/commands/subscriptioncmds.c - DefGetCopyData
>
> +/*
> + * Validate the value specified for copy_data parameter.
> + */
> +static CopyData
> +DefGetCopyData(DefElem *def)

Changed it to defGetCopyData to keep the naming similar to others

> ~~~
>
> 2.4
>
> + /*
> + * If no parameter given, assume "true" is meant.
> + */
>
> Please modify the comment to match the recent push [1].

Modified

> ~~~
>
> 2.5 src/test/subscription/t/032_localonly.pl
>
> 2.5.a
> +# Check Alter subscription ... refresh publication when there is a new
> +# table that is subscribing data from a different publication
> +$node_A->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");
> +$node_B->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");
>
> Unfortunately, I think tab_full1 is a terrible table name because in
> my screen font the 'l' and the '1' look exactly the same so it just
> looks like a typo. Maybe change it to "tab_new" or something?

Modified to tab_new

> 2.5b
> What exactly is the purpose of "full" in all these test table names?
> AFAIK "full" is just some name baggage inherited from completely
> different tests which were doing full versus partial table
> replication. I'm not sure it is relevant here.

Modified to tab

Thanks for the comments, the v31 patch attached has the changes for the same.

Regards,
Vignesh