Re: Allow logical replication to copy tables in binary format

Here are some review comments for v15-0001

======
doc/src/sgml/logical-replication.sgml

1.
+ target table. However, logical replication in binary format is
more restrictive,
+ see <literal>binary</literal> option of
+ <link linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link>
+ for more details.

IMO (and Chat-GPT agrees) the new text should be 2 sentences.

Also, I changed "more details" --> "details" because none are provided here,.

SUGGESTION
However, logical replication in <literal>binary</literal> format is
more restrictive. See the binary option of <link
linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link> for details.

======
doc/src/sgml/ref/alter_subscription.sgml

2.
+ <para>
+ See <literal>binary</literal> option of <xref
linkend="sql-createsubscription"/>
+ for details of copying pre-existing data in binary format.
+ </para>

Should the link should be defined more like you did above using the
<command> markup to get the better font?

SUGGESTION (also minor rewording)
See the <literal>binary</literal> option of <link
linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link> for details about copying pre-existing
data in binary format.

======
doc/src/sgml/ref/create_subscription.sgml

3.
<para>
- Specifies whether the subscription will request the publisher to
- send the data in binary format (as opposed to text).
- The default is <literal>false</literal>.
- Even when this option is enabled, only data types having
- binary send and receive functions will be transferred in binary.
+ Specifies whether the subscription will request the publisher to send
+ the data in binary format (as opposed to text). The default is
+ <literal>false</literal>. Any initial table synchronization copy
+ (see <literal>copy_data</literal>) also uses the same format. Binary
+ format can be faster than the text format, but it is less portable
+ across machine architectures and PostgreSQL versions. Binary format
+ is very data type specific; for example, it will not allow copying
+ from a smallint column to an integer column, even though that would
+ work fine in text format. Even when this option is enabled, only data
+ types having binary send and receive functions will be transferred in
+ binary. Note that the initial synchronization requires all data types
+ to have binary send and receive functions, otherwise the
synchronization
+ will fail (see <xref linkend="sql-createtype"/> for more about
+ send/receive functions).
</para>

IMO that part saying "from a smallint column to an integer column"
should have <type></type> markups for "smallint" and "integer".

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

4.
+ /*
+ * The binary option for replication is supported since v14
+ */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 140000 &&
+ MySubscription->binary)
+ {
+ appendStringInfo(&cmd, " WITH (FORMAT binary)");
+ options = lappend(options, makeDefElem("format", (Node *)
makeString("binary"), -1));
+ }

Should this now be a single-line comment instead of spanning 3 lines?

======
src/test/subscription/t/014_binary.pl

5.
Everything looked OK to me, but the TAP file has only small comments
for each test step, which forces you to read everything from
top-to-bottom to understand what is going on. I felt it might be
easier to understand the tests if you add a few "bigger" comments just
to break the tests into the categories being tested. For example,
something like:

# ------------------------------------------------------
# Ensure binary mode also executes COPY in binary format
# ------------------------------------------------------

~

# --------------------------------------
# Ensure normal binary replication works
# --------------------------------------

~

# ------------------------------------------------------------------------------
# Use ALTER SUBSCRIPTION to change to text format and then back to binary format
# ------------------------------------------------------------------------------

~

# ---------------------------------------------------------------
# Test binary replication without and with send/receive functions
# ---------------------------------------------------------------

~

# ----------------------------------------------
# Test different column orders on pub/sub tables
# ----------------------------------------------

~

# -----------------------------------------------------
# Test mismatched column types with/without binary mode
# -----------------------------------------------------

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