Horton

RE: Handle infinite recursion in logical replication setup

Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com>

View thread View on postgr.es
From: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
To: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-07-04T08:16:45Z
Lists: pgsql-hackers 
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);

Regards,
Shi yu

Commits

  1. Improved CREATE SUBSCRIPTION message for clarity

  2. Fix the test case introduced by commit 8756930190.

  3. Raise a warning if there is a possibility of data from multiple origins.

  4. Add wait_for_subscription_sync for TAP tests.

  5. Bump catversion for commit d8cd0c6c95c0120168df93aae095df4e0682a08a.

  6. Allow users to skip logical replication of data having origin.

  7. Improve two comments related to a boolean DefElem's value

  8. doc: Fix man page whitespace issues