Re: Handle infinite recursion in logical replication setup

On Tue, Jul 5, 2022 at 9:33 PM vignesh C <vignesh21@gmail.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.