RE: Handle infinite recursion in logical replication setup

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
publishermight 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
publishermight have replicated data."),
 
                    errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off/force."));

        ExecClearTuple(slot);

Regards,
Shi yu