Re: Handle infinite recursion in logical replication setup

Firstly, I have some (case-sensitivity) questions about the previous
patch which was already pushed [1].

Q1. create_subscription docs

I did not understand why the docs refer to slot_name = NONE, yet the
newly added option says origin = none/any. I think that saying origin
= NONE/ANY would be more consistent with the existing usage of NONE in
this documentation.

~~~

Q2. parse_subscription_options

Similarly, in the code (parse_subscription_options), I did not
understand why the checks for special name values are implemented
differently:

The new 'origin' code is using pg_strcmpcase to check special values
(none/any), and the old 'slot_name' code uses case-sensitive strcmp to
check the special value (none).

FWIW, here I thought the new origin code is the correct one.

======

Now, here are some review comments for the patch v38-0001:

1. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed

@@ -1781,6 +1858,121 @@ AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId)
  table_close(rel, RowExclusiveLock);
 }

+/*
+ * Check and throw an error if the publisher has subscribed to the same table
+ * from some other publisher. This check is required only if copydata is ON and
+ * the origin is local for CREATE SUBSCRIPTION and
+ * ALTER SUBSCRIPTION ... REFRESH statements to avoid replicating remote data
+ * from the publisher.
+ *
+ * This check need not be performed on the tables that are already added as
+ * incremental sync for such tables will happen through WAL and the origin of
+ * the data can be identified from the WAL records.
+ *
+ * subrel_local_oids contains the list of relation oids that are already
+ * present on the subscriber.
+ */
+static void
+check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications,
+    CopyData copydata, char *origin,
+    Oid *subrel_local_oids, int subrel_count)

1a.
"copydata is ON" --> "copy_data = on" (because the comment is talking
about the CREATE/ALTER statements, so it seemed a bit confusing to
refer to the copydata function param instead of the copy_data
subscription parameter)

1b.
"the origin is local" ?? But, "local" was the old special name value.
Now it is "none", so I think this part needs minor rewording.

~~~

2.

+ if (copydata != COPY_DATA_ON || !origin ||
+ (pg_strcasecmp(origin, "none") != 0))
+ return;

Should this be using the constant LOGICALREP_ORIGIN_NONE?

~~~

3.

+ /*
+ * Throw an error if the publisher has subscribed to the same table
+ * from some other publisher. We cannot differentiate between the
+ * origin and non-origin data that is present in the HEAP during the
+ * initial sync. Identification of non-origin data can be done only
+ * from the WAL by using the origin id.
+ *
+ * XXX: For simplicity, we don't check whether the table has any data
+ * or not. If the table doesn't have any data then we don't need to
+ * distinguish between local and non-local data so we can avoid
+ * throwing an error in that case.
+ */

3a.
When the special origin value changed from "local" to "none" this
comment's first part seems to have got a bit lost in translation.

SUGGESTION:
Throw an error if the publisher has subscribed to the same table from
some other publisher. We cannot know the origin of data during the
initial sync. Data origins can be found only from the WAL by looking
at the origin id.

3b.
I think referring to "local and non-local" data in the XXX part of
this comment also needs some minor rewording now that "local" is not a
special origin name anymore.

------
[1] https://github.com/postgres/postgres/commit/366283961ac0ed6d89014444c6090f3fd02fce0a

Kind Regards,
Peter Smith.
Fujitsu Australia