Re: Handle infinite recursion in logical replication setup

I checked again the v26* patch set.

I had no more comments for v26-0001, v26-0002, or v26-0004, but below
are some comments for v26-0003.

========
v26-0003
========

3.1 src/backend/catalog/pg_subscription.c

3.1.a
+/*
+ * Get all relations for subscription that are in a ready state.
+ *
+ * Returned list is palloc'ed in current memory context.
+ */
+List *
+GetSubscriptionReadyRelations(Oid subid)

"subscription" -> "the subscription"

Also, It might be better to say in the function header what kind of
structures are in the returned List. E.g. Firstly, I'd assumed it was
the same return type as the other function
GetSubscriptionReadyRelations, but it isn’t.

3.1.b
I think there might be a case to be made for *combining* those
SubscriptionRelState and SubscripotionRel structs into a single common
struct. Then all those GetSubscriptionNotReadyRelations and
GetSubscriptionNotReadyRelations (also GetSubscriptionRelations?) can
be merged to be just one common function that takes a parameter to say
do you want to return the relation List of ALL / READY /  NOT_READY?
What do you think?

======

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

+/*
+ * 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.
+ */
+static void
+check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications,
+    CopyData copydata, char *origin, Oid subid)

The function comment probably should say something about why the new
READY logic was added in v26.

~~~

3.3

3.3.a
+ /*
+ * The subid will be valid only for ALTER SUBSCRIPTION ... REFRESH
+ * PUBLICATION. Get the ready relations for the subscription only in case
+ * of ALTER SUBSCRIPTION case as there will be no relations in ready state
+ * while the subscription is created.
+ */
+ if (subid != InvalidOid)
+ subreadyrels = GetSubscriptionReadyRelations(subid);

The word "case" is 2x in the same sentence. I also paraphrased my
understanding of this comment below. Maybe it is simpler?

SUGGESTION
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.

3.3.b
+ if (subid != InvalidOid)
+ subreadyrels = GetSubscriptionReadyRelations(subid);

SUGGESTION
if (OidIsValid(subid))

======

3.4 src/test/subscription/t/032_localonly.pl

Now all the test cases are re-using the same data (e.g. 11,12,13) and
you are deleting the data between the tests. I guess it is OK, but IMO
the tests are easier to read when each test part was using unique data
(e.g. 11,12,13, then 12,22,32, then 13,23,33 etc)

------
Kind Regards,
Peter Smith.
Fujitsu Australia