Re: Handle infinite recursion in logical replication setup

On Fri, Jul 8, 2022 at 4:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 5, 2022 at 9:33 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Since the existing test is already handling the verification of this
> > scenario, I felt no need to add the test. Updated v29 patch removes
> > the 0001 patch which had the test case.
> >
>
> I am not able to apply 0001.
> patching file src/bin/psql/tab-complete.c
> Hunk #1 FAILED at 1873.
> Hunk #2 FAILED at 3152.

I have rebased the patch on top of HEAD.

> Few comments on 0002
> =====================
> 1.
> + <xref linkend="sql-createsubscription-notes" /> for interaction
>
> Is there a need for space before / in above? If not, please remove it
> with similar extra space from other similar usages.

Modified

> 2.
> + <para>
> + There is some interaction between the <literal>origin</literal>
> + parameter and the <literal>copy_data</literal> parameter. Refer to
> + the <command>CREATE SUBSCRIPTION</command>
> + <xref linkend="sql-createsubscription-notes" /> for interaction
> + details and usage of <literal>force</literal> for
> + <literal>copy_data</literal> parameter.
> </para>
>
> I think this is bit long. We can try to reduce this by something like:
> Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
> <literal>force</literal> option and its interaction with the
> <literal>origin</literal> parameter.
>
> Also, adopt the same other places if you agree with the above change.

Modified with slight rewording.

> 4.
> @@ -601,16 +549,28 @@ GetSubscriptionNotReadyRelations(Oid subid)
> SysScanDesc scan;
>
> rel = table_open(SubscriptionRelRelationId, AccessShareLock);
> -
> ScanKeyInit(&skey[nkeys++],
> Anum_pg_subscription_rel_srsubid,
>
> Spurious line removal.

I have removed this

> 5.
> +static void
> +check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications,
> + CopyData copydata, char *origin, Oid subid)
> {
> ...
> ...
> + /*
> + * Get the ready relations for the subscription. The subid will be valid
> + * only for ALTER SUBSCRIPTION ... REFRESH because there will be no
> + * relations in ready state while the subscription is created.
> + */
> + if (OidIsValid(subid))
> + subreadyrels = GetSubscriptionRelations(subid, READY_STATE);
>
> Why do we want to consider only READY_STATE relations here? If you see
> its caller AlterSubscription_refresh(), we don't consider copying the
> relation if it exists on subscribers in any state. If my observation
> is correct then you probably don't need to introduce SubRelStateType.

I have changed it similar to the caller function and removed SubRelStateType

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

Regards,
Vignesh