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>, Peter Smith <smithpb2250@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Dilip Kumar <dilipbalaut@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2022-09-06T04:00:47Z
Lists: pgsql-hackers 
On Tue, Sep 6, 2022 11:14 AM vignesh C <vignesh21@gmail.com> wrote:
> 
> Thanks for the comments, the attached patch has the changes for the same.
> 

Thanks for updating the patch. Here are some comments.

1.
+	if (subrel_count)
+	{
+		/*
+		 * In case of ALTER SUBSCRIPTION ... REFRESH, subrel_local_oids
+		 * contains the list of relation oids that are already present on the
+		 * subscriber. This check should be skipped for these tables.
+		 */
+		appendStringInfo(&cmd, "AND C.oid NOT IN (\n"
+						 "SELECT C.oid \n"
+						 "FROM pg_class C\n"
+						 "     JOIN pg_namespace N ON N.oid = C.relnamespace\n"
+						 "WHERE ");
+		get_skip_tables_str(subrel_local_oids, subrel_count, &cmd);
+		appendStringInfo(&cmd, ")");
+	}

I think we can skip the tables without using a subquery. See the SQL below.
Would it be better?

SELECT DISTINCT P.pubname AS pubname
FROM pg_publication P,
         LATERAL pg_get_publication_tables(P.pubname) GPT
         LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
         pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND P.pubname IN ('p1', 'p3')
AND NOT (N.nspname = 'public' AND C.relname = 'tbl')
AND NOT (N.nspname = 'public' AND C.relname = 't1');


2.
+   When using a subscription parameter combination of
+   <literal>copy_data=true</literal> and <literal>origin=NONE</literal>, the

Could we use "copy_data = true" and "origin = NONE" (add two spaces around the
equals sign)? I think it looks clearer that way. And it is consistent with other
places in this file.

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