Re: Allow logical replication to copy tables in binary format

Here are my review comments for v18-0001

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

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

Because you've changed the linkend to be the binary option, IMO now
the <link> part also needs to be modified. Otherwise, this page has
multiple "CREATE SUBSCRIPTION" links which jump to different places,
which just seems wrong to me.

SUGGESTION (for the "See the" sentence)

See the <link linkend="sql-createsubscription-binary"><literal>binary</literal>
option</link> of <command>CREATE SUBSCRIPTION</command> for details.

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

2.
+ <para>
+ See the <literal>binary</literal> option of
+ <link
linkend="sql-createsubscription-binary"><command>CREATE
SUBSCRIPTION</command></link>
+ for details about copying pre-existing data in binary format.
+ </para>

(Same as review comment #1 above)

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

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

3.
+ /*
+ * Prior to v16, initial table synchronization will use text format even
+ * if the binary option is enabled for a subscription.
+ */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000 &&
+ MySubscription->binary)
+ {
+ appendStringInfoString(&cmd, " WITH (FORMAT binary)");
+ options = lappend(options,
+ makeDefElem("format",
+ (Node *) makeString("binary"), -1));
+ }

I think there can only be 0 or 1 list element in 'options'.

So, why does the code here use lappend(options,...) instead of just
using list_make1(...)?

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

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

# Test syncing tables with mismatching column types
$node_publisher->safe_psql(
'postgres', qq(
CREATE TABLE public.test_mismatching_types (
a bigint PRIMARY KEY
);
INSERT INTO public.test_mismatching_types (a)
VALUES (1), (2);
));

# Check the subscriber log from now on.
$offset = -s $node_subscriber->logfile;

# Ensure the subscription is enabled. disable_on_error is still true,
# so the subscription can be disabled due to missing realtion until
# the test_mismatching_types table is created.
$node_subscriber->safe_psql(
'postgres', qq(
CREATE TABLE public.test_mismatching_types (
a int PRIMARY KEY
);
ALTER SUBSCRIPTION tsub ENABLE;
ALTER SUBSCRIPTION tsub REFRESH PUBLICATION;
));

~~

I found the "Ensure the subscription is enabled..." comment and the
necessity for enabling the subscription to be confusing.

Can't some complications all be eliminated just by creating the table
on the subscribe side first?

For example, I rearranged that test (above fragment) like below and it
still works OK for me:

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

# Create the table on the subscriber side
$node_subscriber->safe_psql(
'postgres', qq(
CREATE TABLE public.test_mismatching_types (
a int PRIMARY KEY
)));

# Check the subscriber log from now on.
$offset = -s $node_subscriber->logfile;

# Test syncing tables with mismatching column types
$node_publisher->safe_psql(
'postgres', qq(
CREATE TABLE public.test_mismatching_types (
a bigint PRIMARY KEY
);
INSERT INTO public.test_mismatching_types (a)
VALUES (1), (2);
));

# Refresh the publication to trigger the tablesync
$node_subscriber->safe_psql(
'postgres', qq(
ALTER SUBSCRIPTION tsub REFRESH PUBLICATION;
));

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