Re: Allow logical replication to copy tables in binary format

On Fri, Mar 17, 2023 at 11:25 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
>
> Hi,
>
> Sharing v17.
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 17 Mar 2023 Cum, 03:02 tarihinde şunu yazdı:
>>
>> I think to reduce the risk of breakage, let's change the check to
>> >=v16. Also, accordingly, update the doc and commit message.
>
>
> Done.
>

Here are my review comments for v17-0001

======
Commit message

1.
Binary copy is supported for v16 or later.

~

As written that's very general and not quite correct. E.g. COPY ...
WITH (FORMAT binary) has been available for a long time. IMO that
commit message sentence ought to be more specific.

SUGGESTION
Binary copy for logical replication table synchronization is supported
only when both publisher and subscriber are v16 or later.

======
src/backend/replication/logical/tablesync.c

2.
@@ -1168,6 +1170,15 @@ copy_table(Relation rel)

appendStringInfoString(&cmd, ") TO STDOUT");
}
+
+ /* The binary option for replication is supported since v16 */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000 &&
+ MySubscription->binary)
+ {
+ appendStringInfoString(&cmd, " WITH (FORMAT binary)");
+ options = lappend(options, makeDefElem("format", (Node *)
makeString("binary"), -1));
+ }

Logical replication binary mode was introduced in v14, so the old
comment ("The binary option for replication is supported since v14")
was correct. Unfortunately, after changing the code check to 16000, I
think the new comment ("The binary option for replication is supported
since v16") became incorrect, and so it needs some rewording. Maybe it
should say something like below:

SUGGESTION
If the publisher is v16 or later, then any initial table
synchronization will use the same format as specified by the
subscription binary mode. If the publisher is before v16, then any
initial table synchronization will use text format regardless of the
subscription binary mode.

------
Kind Regards,
Peter Smith.
Fujitsu Australia