Re: Allow logical replication to copy tables in binary format

On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote: 

It'd be better and clearer to have a separate TAP test file IMO since
the focus of the feature here isn't to just test for data types. With
separate tests, you can verify "ERROR:  incorrect binary data format
in logical replication column 1" cases.

With the above said, do you think checking for publisher versions is
needed? The user can decide to enable/disable binary COPY based on the
publisher's version no?
+    /* If the publisher is v16 or later, specify the format to copy data. */
+    if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000)
+    {

Few more comments on v7:
1.
+          Specifies the format in which pre-existing data on the publisher will
+          copied to the subscriber. Supported formats are
+          <literal>text</literal> and <literal>binary</literal>. The default is
+          <literal>text</literal>.
It'll be good to call out the cases in the documentation as to where
copy_format can be enabled and needs to be disabled.

2.
+             errmsg("%s value should be either \"text\" or \"binary\"",
How about "value must be either ...."?

3.
Why should the subscription's binary option and copy_format option be
tied at all? Tying these two options hurts usability. Is there a
fundamental reason? I think they both are/must be independent. One
deals with data types and another deals with how initial table data is
copied.