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