RE: Handle infinite recursion in logical replication setup

On Sun, Jul 3, 2022 11:00 PM vignesh C  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