Re: Handle infinite recursion in logical replication setup

On Mon, Jul 4, 2022 at 1:46 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Sun, Jul 3, 2022 11:00 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v27 patch has the changes for the
> > same.
> >
>
> Thanks for updating the patch.
>
> A comment on 0003 patch:
> +               /*
> +                * No need to throw an error for the tables that are in ready state,
> +                * as the walsender will send the changes from WAL in case of tables
> +                * in ready state.
> +                */
> +               if (isreadytable)
> +                       continue;
> +
> ...
> +               ereport(ERROR,
> +                               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                               errmsg("table: \"%s.%s\" might have replicated data in the publisher",
> +                                          nspname, relname),
> +                               errdetail("CREATE/ALTER SUBSCRIPTION with origin = local and copy_data = on is not allowed when the publisher might have replicated data."),
> +                               errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off/force."));
> +
> +               ExecClearTuple(slot);
>
> I think we should call ExecClearTuple() before getting next tuple, so it should
> be called if the table is in ready state. How about modifying it to:
>                 if (!isreadytable)
>                         ereport(ERROR,
>                                         errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>                                         errmsg("table: \"%s.%s\" might have replicated data in the publisher",
>                                                    nspname, relname),
>                                         errdetail("CREATE/ALTER SUBSCRIPTION with origin = local and copy_data = on is not allowed when the publisher might have replicated data."),
>                                         errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off/force."));
>
>                 ExecClearTuple(slot);

Thanks for the comment, I have modified this in the v28 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm3MNK2hMYroTiHGS9HkSxiA-az1QC1mpa0YwDZ8nGmmZg%40mail.gmail.com

Regards,
Vignesh