Horton

RE: Allow logical replication to copy tables in binary format

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

View thread
From: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
To: Melih Mutlu <m.melihmutlu@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 'Jelte Fennema' <postgres@jeltef.nl>
Cc: "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>, Euler Taveira <euler@eulerto.com>, Amit Kapila <amit.kapila16@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: 2023-02-23T09:29:44Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow logical replication to copy tables in binary format.

  2. Common function for percent placeholder replacement

  3. Doc: add XML ID attributes to <sectN> and <varlistentry> tags.

  4. Re-order disable_on_error in tab-complete.

  5. Make subscription tests pass with log_error_verbosity=verbose

  6. Weaken type-OID-matching checks in array_recv and record_recv.

  7. Allow logical replication to transfer data in binary format.

On Thu, Feb 23, 2023 12:40 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> 
> > > I'm not sure the combination of "copy_format = binary" and "copy_data =
> false"
> > > should be accepted or not. How do you think?
> >
> > It seems quite useless indeed to specify the format of a copy that won't
> happen.
> 
> I understood that the conbination of "copy_format = binary" and "copy_data =
> false"
> should be rejected in parse_subscription_options() and AlterSubscription(). Is it
> right?
> I'm expecting that is done in next version.
> 

The copy_data option only takes effect once in CREATE SUBSCIPTION or ALTER
SUBSCIPTION REFRESH PUBLICATION command, but the copy_format option can take
affect multiple times if the subscription is refreshed multiple times. Even if
the subscription is created with copy_date=false, copy_format can take affect
when executing ALTER SUBSCIPTION REFRESH PUBLICATION. So, I am not sure we want
to reject this usage.

Besides, here are my comments on the v9 patch.
1.
src/bin/pg_dump/pg_dump.c
	if (fout->remoteVersion >= 160000)
-		appendPQExpBufferStr(query, " s.suborigin\n");
+	{
+		appendPQExpBufferStr(query, " s.suborigin,\n");
+		appendPQExpBufferStr(query, " s.subcopyformat\n");
+	}
 	else
-		appendPQExpBuffer(query, " '%s' AS suborigin\n", LOGICALREP_ORIGIN_ANY);
+	{
+		appendPQExpBuffer(query, " '%s' AS suborigin,\n", LOGICALREP_ORIGIN_ANY);
+		appendPQExpBuffer(query, " '%c' AS subcopyformat\n", LOGICALREP_COPY_AS_TEXT);
+	}

src/bin/psql/describe.c
		if (pset.sversion >= 160000)
+		{
 			appendPQExpBuffer(&buf,
 							  ", suborigin AS \"%s\"\n",
 							  gettext_noop("Origin"));
+			/* Copy format is only supported in v16 and higher */
+			appendPQExpBuffer(&buf,
+							  ", subcopyformat AS \"%s\"\n",
+							  gettext_noop("Copy Format"));
+		}

I think we can call only once appendPQExpBuffer() for the two options which are supported in v16.
For example,
		if (pset.sversion >= 160000)
		{
			appendPQExpBuffer(&buf,
							  ", suborigin AS \"%s\"\n"
							  ", subcopyformat AS \"%s\"\n",
							  gettext_noop("Origin"),
							  gettext_noop("Copy Format"));
		}

2.
src/bin/psql/tab-complete.c
@@ -1926,7 +1926,7 @@ psql_completion(const char *text, int start, int end)
 	/* ALTER SUBSCRIPTION <name> SET ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "("))
 		COMPLETE_WITH("binary", "disable_on_error", "origin", "slot_name",
-					  "streaming", "synchronous_commit");
+					  "streaming", "synchronous_commit", "copy_format");
 	/* ALTER SUBSCRIPTION <name> SKIP ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SKIP", "("))
 		COMPLETE_WITH("lsn");
@@ -3269,7 +3269,8 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
 		COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
 					  "disable_on_error", "enabled", "origin", "slot_name",
-					  "streaming", "synchronous_commit", "two_phase");
+					  "streaming", "synchronous_commit", "two_phase",
+					  "copy_format");


The options should be listed in alphabetical order. See commit d547f7cf5ef.

Regards,
Shi Yu