RE: Handle infinite recursion in logical replication setup

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

Regards,
Shi yu