Re: Handle infinite recursion in logical replication setup

On Mon, Jul 11, 2022 at 5:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Jul 9, 2022 at 8:11 PM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> Thanks, a few more comments on v30_0002*
> 1.
> +/*
> + * Represents whether copy_data parameter is specified with off, on or force.
>
> A comma is required after on.

Modified

> 2.
>   qsort(subrel_local_oids, list_length(subrel_states),
>     sizeof(Oid), oid_cmp);
>
> + check_pub_table_subscribed(wrconn, sub->publications, copy_data,
> +    sub->origin, subrel_local_oids,
> +    list_length(subrel_states));
>
> We can avoid using list_length by using an additional variable in this case.

Modified

> 3.
> errmsg("table: \"%s.%s\" might have replicated data in the publisher",
> +    nspname, relname),
>
> Why ':' is used after the table in the above message? I don't see such
> a convention at other places in the code. Also, having might in the
> error messages makes it less clear, so, can we slightly change the
> message as in the attached patch?

Modified as suggested

> 4. I have made some additional changes in the comments, kindly check
> the attached and merge those if you are okay.

I have merged the changes

> 5.
> +$node_C->safe_psql(
> +     'postgres', "
> +        DELETE FROM tab_full");
> +$node_B->safe_psql(
> +     'postgres', "
> +        DELETE FROM tab_full where a = 13");
>
> Don't we need to wait for these operations to replicate?

Modified to include wait

Thanks for the comments, the v31 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm2-D860yULtcmZAzDbdiof-Dg6Y_YaY4owbO6Rj%3DXEHMw%40mail.gmail.com

Regards,
Vignesh