Re: Handle infinite recursion in logical replication setup

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.

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.

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.

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.

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.

--
With Regards,
Amit Kapila.