RE: Allow logical replication to copy tables in binary format

On Monday, January 30, 2023 7:50 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
> Thanks for reviewing.
...
> Please see [1] and you'll get the following error in your case:
> "ERROR: incorrect binary data format in logical replication column 1"
Hi, thanks for sharing v7.

(1) general comment

I wondered if the addition of the new option/parameter can introduce some confusion to the users.

case 1. When binary = true and copy_format = text, the table sync is conducted by text.
case 2. When binary = false and copy_format = binary, the table sync is conducted by binary.
(Note that the case 2 can be accepted after addressing the 3rd comment of Bharath-san in [1].
I agree with the 3rd comment by itself.)

The name of the new subscription parameter looks confusing.
How about "init_sync_format" or something ?

(2) The commit message doesn't get updated.

The commit message needs to mention the new subscription option.

(3) whitespace errors.

$ git am v7-0001-Allow-logical-replication-to-copy-table-in-binary.patch
Applying: Allow logical replication to copy table in binary
.git/rebase-apply/patch:95: trailing whitespace.
copied to the subscriber. Supported formats are
.git/rebase-apply/patch:101: trailing whitespace.
that data will not be copied if <literal>copy_data = false</literal>.
warning: 2 lines add whitespace errors.

(4) pg_dump.c

if (fout->remoteVersion >= 160000)
- appendPQExpBufferStr(query, " s.suborigin\n");
+ appendPQExpBufferStr(query, " s.suborigin,\n");
else
- appendPQExpBuffer(query, " '%s' AS suborigin\n", LOGICALREP_ORIGIN_ANY);
+ appendPQExpBuffer(query, " '%s' AS suborigin,\n", LOGICALREP_ORIGIN_ANY);
+
+ if (fout->remoteVersion >= 160000)
+ appendPQExpBufferStr(query, " s.subcopyformat\n");
+ else
+ appendPQExpBuffer(query, " '%c' AS subcopyformat\n", LOGICALREP_COPY_AS_TEXT);

This new branch for v16 can be made together with the previous same condition.

(5) describe.c

+
+ /* Copy format is only supported in v16 and higher */
+ if (pset.sversion >= 160000)
+ appendPQExpBuffer(&buf,
+ ", subcopyformat AS \"%s\"\n",
+ gettext_noop("Copy Format"));

Similarly to (4), this creates a new branch for v16. Please see the above codes of this part.

(6)

+ * Extract the copy format value from a DefElem.
+ */
+char
+defGetCopyFormat(DefElem *def)

Shouldn't this function be static and remove the change of subscriptioncmds.h ?

(7) catalogs.sgml

The subcopyformat should be mentioned here and the current description for subbinary
should be removed.

(8) create_subscription.sgml

+ <literal>text</literal>.
+
+ <literal>binary</literal> format can be selected only if

Unnecessary blank line.

[1] - https://www.postgresql.org/message-id/CALj2ACW5Oa7_v25iZb326UXvtM_tjQfw0Tc3hPJ8zN4FZqc9cw%40mail.gmail.com

Best Regards,
Takamichi Osumi