RE: Allow logical replication to copy tables 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