RE: Allow logical replication to copy tables in binary format

On Thu, Feb 23, 2023 12:40 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)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