Re: Handle infinite recursion in logical replication setup

On Mon, Jul 4, 2022 at 1:46 PM shiy(dot)fnst(at)fujitsu(dot)com
<shiy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Sun, Jul 3, 2022 11:00 PM vignesh C <vignesh21(at)gmail(dot)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