Re: Handle infinite recursion in logical replication setup

On Tue, Jul 5, 2022 at 6:14 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.

This function has been removed and GetSubscriptionRelations is used
with slight changes.

> 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?

I have used a common function that can get all relations, ready
relations and not ready relations instead of the 3 functions.

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

Added a comment for the same.

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

Modified

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

Modified

> ======
>
> 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)

Modified

Thanks for the comments, the v29 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm1T5utq60qVx%3DRN60rHcg7wt2psM7PpCQ2fDiB-R8oLGg%40mail.gmail.com

Regards,
Vignesh