Subject: RE: Allow logical replication to copy tables in binary format

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 20 Feb 2023 07:12:03 +0000

Dear Melih,

Thank you for updating the patch. Before reviewing, I found that
cfbot have not accepted v8 patch [1].

IIUC src/psql/describe.c has been modified in v8, but src/test/regress/expected/subscription.out
has not been changed accordingly.

[1]: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/42/3840

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Melih Mutlu <[email-masked]>
Date: Mon, 20 Feb 2023 14:46:57 +0300

Hi,

Hayato Kuroda (Fujitsu) <[email-masked]>, 20 Şub 2023 Pzt, 10:12

> cfbot have not accepted v8 patch [1].

Thanks for letting me know.
Attached the fixed version of the patch.

Best,
-- 
Melih Mutlu
Microsoft

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 21 Feb 2023 19:18:00 +0530

> Attached the fixed version of the patch.

Thanks. I have few comments on v9 patch:

1.
+                    /* Do not allow binary = false with copy_format = binary */
+                    if (!opts.binary &&
+                        sub->copyformat == LOGICALREP_COPY_AS_BINARY &&
+                        !IsSet(opts.specified_opts, SUBOPT_COPY_FORMAT))
+                        ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                 errmsg("cannot set %s for a
subscription with %s",
+                                        "binary = false",
"copy_format = binary")));

I don't understand why we'd need to tie an option (binary) that deals
with data types at column-level with another option (copy_format) that
requests the entire table data to be in binary. This'd essentially
make one to set binary = true to use copy_format = binary, no? IMHO,
this inter-dependency is not good for better usability.

2. Why can't the tests that this patch adds be simple? Why would it
need to change the existing tests at all? I'm thinking to create a new
00X_binary_copy_format.pl or such and setting up logical replication
with copy_format = binary and letting table sync worker request
publisher in binary format - you can verify this via publisher server
logs - look for COPY with BINARY option. If required, have the table
with different data types. This greatly reduces the patch's footprint.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 22 Feb 2023 07:01:28 +0000

Dear Melih,

Thank you for updating the patch! Followings are my comments.

01. catalogs.sgml

```
        If true, the subscription will request that the publisher send data
-       in binary format
```

I'm not clear here. subbinary does not directly mean that whether the worker
requests to send data or not. How about:

If true, the subscription will request that the publisher send data in binary
format, except initial data synchronization

02. create_subscription.sgml

```
+          the binary format is very data type specific, it will not allow copying
+          between different column types as opposed to text format. Note that
```

The name of formats are not specified as <literal>, whereas in previous sentence
they are. We can use format either of them.

03. parse_subscription_options()

I'm not sure the combination of "copy_format = binary" and "copy_data = false"
should be accepted or not. How do you think?

04. parse_subscription_options()

```
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                       errmsg("%s and %s are mutually exclusive options",
+                                               "binary = false", "copy_format = binary")));
+       }
```

A comment for translator seemed to be missed.

05. CreateSubscription()

```
        values[Anum_pg_subscription_suborigin - 1] =
                CStringGetTextDatum(opts.origin);
+       values[Anum_pg_subscription_subcopyformat - 1] = CharGetDatum(opts.copy_format);
```

I think this should be done the same ordering with FormData_pg_subscription.
Maybe after the line?

```
	values[Anum_pg_subscription_subdisableonerr - 1] = BoolGetDatum(opts.disableonerr);
```

06. AlterSubscription()

If we decided not to accept "copy_format = binary" and "copy_data = false", here
should be also fixed.

07. AlterSubscription()

```
+                                               ereport(ERROR,
+                                                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                                                errmsg("cannot set %s for a subscription with %s",
+                                                                               "binary = false", "copy_format = binary")));
...
+                                               ereport(ERROR,
+                                                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                                                errmsg("cannot set %s for a subscription with %s",
+                                                                               "copy_format = binary", "binary = false")));
```

Comments for translator seemed to be missed.

08. defGetCopyFormat()

```
+       /*
+        * If no parameter value given, set it to text format.
+        */
+       if (!def->arg)
+               return LOGICALREP_COPY_AS_TEXT;
```

I think the case no parameter is given should be rejected. It should be accepted
only when the parameter has boolean data type. Note that defGetStreamingMode()
is accepted no-parameter style due to the compatibility. At first streaming is
boolean, and then "parallel" is added.

09. describeSubscriptions

```
+                       /* Copy format is only supported in v16 and higher */
```

I think this comment should be atop of if statement, and it can mention about Origin too.

10. pg_subscription.h

```
+       char            subcopyformat BKI_DEFAULT(LOGICALREP_COPY_AS_TEXT); /* Copy format *
```

I'm not sure whether BKI_DEFAULT() is needed or not. Other options like twophase
does not have the default value as catalog level. The default is set in
parse_subscription_options() and then the value will be set to catalog.

11. typedef struct Subscription

In catalog entry the subcopyformat is aligned just after subdisableonerr, but in
struct Subscription, copyformat is added at the end. Can we place just after disableonerr?

12. Reply

> Since binary copy relies on COPY command, we may have problems across
> different server versions in cases where COPY is not portable.
> What I understand from this [1], COPY works across server versions later
> than 7.4. This shouldn't be a problem for logical replication.
> Currently the patch also does not allow binary copy if the publisher
> version is older than 16.

If in future version the general data type is added, the copy command in binary
format will not work well, right? It is because the inferior version does not have
recv/send functions for added type. It means that there is a possibility that
replication between different versions may be failed if binary format is specified.
Therefore, I think we should accept copy_format = binary only when the major
version of servers are the same.

Note that this comments is not the request to the patch. Maybe the modification
should be done not only for copy_format but also binary, and it may be out of scope
the patch.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Smith <[email-masked]>
Date: Fri, 24 Feb 2023 14:02:19 +1100

Here is my summary of this thread so far, plus some other thoughts.

(I wrote this mostly for my own internal notes/understanding, but
maybe it is a helpful summary for others so I am posting it here too).

~~

1. Initial Goal
------------------

Currently, the CREATE SUBSCRIPTION ... WITH(binary=true) mode does
data replication in binary mode, but tablesync COPY phase is still
only in text mode. IIUC Melih just wanted to unify those phases so
binary=true would mean also do the COPY phase in binary [1].

Actually, this was my very first expectation too.

2. Objections to unification
-----------------------------------

Bharath posted suggesting tying the COPY/replication parts is not a
good idea [2]. But if these are not to be unified then it requires a
new subscription option to be introduced, and currently, the thread
refers to this new option as copy_format=binary.

3. A problem: binary replication is not always binary!
----------------------------------------------------------------------

Shi-san reported [3] that under special circumstances (e.g. if the
datatype has no binary output function) the current HEAD binary=true
mode for replication has the ability to fall back to text replication.
Since the binary COPY doesn't do this, it means binary COPY could fail
in some cases where the binary=true replication will be OK.

AFAIK, this means that even if we *wanted* to unify everything with
just 'binary=true' it can't be done like that.

4. New option is needed
---------------------------------

OK, so let's assume these options must be separated (because of the
problem of #3).

~

4a. New string option called copy_format?

Currently, the patch/thread describes a new 'copy_format' string
option with values 'text' (default) and 'binary'.

Why? If there are only 2 values then IMO it would be better to have a
*boolean* option called something like binary_copy=true/false.

~

4b. Or, we could extend copy_data

Jelte suggested [4] we could extend copy_data = 'text' (aka on/true)
OR 'binary' OR 'none' (aka off/false).

That was interesting, although
- my first impression was to worry about backward compatibility issues
for existing application code. I don't know if those are easily
solved.
- AFAIK such "hybrid" boolean/enum options are kind of frowned upon so
I am not sure if a committer would be in favour of introducing another
one.

5. Combining options
------------------------------

Because options are separated, it means combinations become possible...

~~

5a. Combining option: "copy_format = binary" and "copy_data = false"

Kuroda [5] wrote such a combination should not be allowed.

I kind of disagree with that. IMO everything should be flexible as
possible. The patch might end up accidentally stopping something that
has a valid use case. Anyway, such restrictions are easy to add later.

~~

5b. Combining options: binary=true/copy_format=binary, and
binary=false/copy_format=binary become possible.

AFAIK currently the patch disallows some combinations:

+ ereport(ERROR,

I kind of disagree with introducing such rules/restrictions. IMO all
this patch needs to do is clearly document all necessary precautions
etc. But if the user still wants to do something, we should just let
them do it. If they manage to shoot themselves in the foot, well it
was their choice after reading the docs, and it's their foot.

6. pub/sub version checking
----------------------------

There were some discussions about doing some server checking to reject
some PG combinations from being allowed to use the copy_format=binary.

Jelte suggested [5] that it is the "responsibility of the operator to
evaluate the risk".

I agree. Yes, the patch certainly needs to *document* all precautions,
but having too many restrictions might end up accidentally stopping
something useful. Anyway, this seems like #5a. I prefer KISS
Principle. More restrictions can be added later if found necessary.

7. More doubts & a thought bubble
---------------------------------

7a. What is user action for this scenario?

I am unsure about this scenario - imagine that everything is working
properly and the copy_format=binary/copy_data=true is all working
nicely for weeks for a certain pub/sub...

But if the publication was defined as FOR ALL TABLES, or as ALL TABLES
IN SCHEMA, then I think the tablesync can still crash if a user
creates a new table that suffers the same COPY binary trouble Shi-san
described earlier [3].

What is the user supposed to do when that tablesync fails?

They had no way to predict it could happen, And it will be too painful
to have to DROP and re-create the entire SUBSCRIPTION again now that
it has happened.

~~

7a. A thought bubble

I wondered (perhaps this is naive) would it be it possible to enhance
the COPY command to enhance the "binary" mode so it can be made to
fall back to text mode if it needs to in the same way that binary
replication does.

If such an enhanced COPY format mode worked, then most of the patch is redundant
- there is no need for any new option
- tablesync COPY could then *always* use this "binary-with-falback" mode

------
[1] Melih initially wanted a unified binary mode -
https://www.postgresql.org/message-id/CAGPVpCQYi9AYQSS%3DRmGgVNjz5ZEnLB8mACwd9aioVhLmbgiAMA%40mail.gmail.com

[2] Barath doesn't like the binary/copy_format inter-dependency -
https://www.postgresql.org/message-id/CALj2ACVPt-BaLMm3Ezy1-rfUzH9qStxePcyGrHPamPESEZSBFA%40mail.gmail.com

[3] Shi-san found binary mode has the ability to fall back to text
sometimes - https://www.postgresql.org/message-id/OSZPR01MB6310B58F069FF8E148B247FDFDA49%40OSZPR01MB6310.jpnprd01.prod.outlook.com

[4] Jelte idea to enhance the copy_data option -
https://www.postgresql.org/message-id/CAGECzQS393xiME%2BEySLU7ceO4xOB81kPjPqNBjaeW3sLgfLhNw%40mail.gmail.com

[5] Kuroda-san etc expecting copy_data=false/copy_format=binary
combination is not allowed -
https://www.postgresql.org/message-id/TYAPR01MB5866DDF02B3CEE59DA024CC3F5AB9%40TYAPR01MB5866.jpnprd01.prod.outlook.com

[6] Jelte says it is the operator's responsibility to use the correct
options - https://www.postgresql.org/message-id/CAGECzQSStdb%2Bx1BxzCktZd1uSjvd6eYq1wcHV3vPCykrGqxYKQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Takamichi Osumi (Fujitsu)" <[email-masked]>
Date: Sun, 26 Feb 2023 22:58:26 +0000

> Attached the fixed version of the patch.
Hi, Melih

Thanks for updating the patch. Minor comments on v9.

(1) commit message

"The patch introduces a new parameter, copy_format, to CREATE SUBSCRIPTION to allow to choose
the format used in initial table synchronization."

This patch introduces the new parameter not only to CREATE SUBSCRIPTION and ALTER SUBSCRIPTION, then this description should be more general, something like below.

"The patch introduces a new parameter, copy_format, as subscription option to
allow user to choose the format of initial table synchronization."

(2) copy_table

We don't need to check the publisher's version below.

+
+       /* If the publisher is v16 or later, specify the format to copy data. */
+       if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000)
+       {
+               char *format = MySubscription->copyformat == LOGICALREP_COPY_AS_BINARY ? "binary" : "text";
+               appendStringInfo(&cmd, "  WITH (FORMAT %s)", format);
+               options = lappend(options, makeDefElem("format", (Node *) makeString(format), -1));
+       }
+

We don't have this kind of check for "binary" option and it seems this is user's responsibility to avoid any errors during replication. If we want to add this kind of check, then we can add checks for both "binary" and "copy_format" option together as an independent patch.

(3) subscription.sql/out

The other existing other subscription options check the invalid input for newly created option (e.g. "foo" for disable_on_error,  streaming mode). So, I think we can add this type of test for this feature.

Best Regards,
	Takamichi Osumi

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 27 Feb 2023 15:46:06 +0530

> Here is my summary of this thread so far, plus some other thoughts.

Thanks.

> 1. Initial Goal

Looking closely at the existing binary=true option and COPY's binary
protocol, essentially they depend on the data type's binary send and
receive functions.

> 3. A problem: binary replication is not always binary!

Right. Apply workers can fallback to text mode transparently, whereas
with binary COPY it's a bit difficult to do so.

> just 'binary=true' it can't be done like that.

Hm, it looks like that.

> 4. New option is needed

How about copy_binary = {true/false}? So, the options for the user are:

copy_binary - defaults to false and when true, the subscriber requests
publisher to send pre-existing table's data in binary format (as
opposed to text) during data synchronization/table copy phase. It is
recommended to enable this option only when 1) the column data types
have appropriate binary send/receive functions, 2) not replicating
between different major versions or different platforms, 3) both
publisher and subscriber tables have the exact same column types (not
when replicating from smallint to int or numeric to int8 and so on), 4)
both publisher and subscriber supports COPY with binary option,
otherwise the table copy can fail.

binary - defaults to false and when true, the subscriber requests
publisher to send table's data in binary format (as opposed to text)
during normal replication phase. <<existing description from the docs
continues>>

Note that with this we made users responsible to choose copy_binary
rather than we being smart.

> 6. pub/sub version checking

IMHO, these restrictions make the feature more complicated to use and
be removed and the options be made simple to use (usability and
simplicity clearly wins the race). If there's any kind of feedback
from the users/developers, we can always come back and improve.

> to have to DROP and re-create the entire SUBSCRIPTION again now that

Can't ALTER SUBSCRIPTION .... SET copy_format = 'text'; and ALTER
SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_data = true); work
here instead of drop and recreate subscription?

> - tablesync COPY could then *always* use this "binary-with-falback" mode

I don't think that's a great idea. COPY is a user-facing SQL command
and any errors (because of data type not having binary send/receive
functions) better be reported to the users. Again, such an option
might complicate both COPY code and usability.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 27 Feb 2023 18:54:00 +0530

> with different data types. This greatly reduces the patch's footprint.

I've done performance testing with the v9 patch.

I can constantly observe 1.34X improvement or 25% improvement in table
sync/copy performance with the patch:
HEAD binary = false
Time: 214398.637 ms (03:34.399)

PATCHED binary = true, copy_format = binary:
Time: 160509.262 ms (02:40.509)

There's a clear reduction (5.68% to 4.49%) in the CPU cycles spent in
copy_table with the patch:
perf report HEAD:
-    5.68%     0.00%  postgres  postgres  [.] copy_table
   - copy_table
      - 5.67% CopyFrom
         - 4.26% NextCopyFrom
            - 2.16% NextCopyFromRawFields
               - 1.55% CopyReadLine
                  - 1.52% CopyReadLineText
                     - 0.76% CopyLoadInputBuf
                          0.50% CopyConvertBuf
                 0.60% CopyReadAttributesText
            - 1.93% InputFunctionCall
                 0.69% timestamptz_in
                 0.53% byteain
         - 0.73% CopyMultiInsertInfoFlush
            - 0.73% CopyMultiInsertBufferFlush
               - 0.66% table_multi_insert
                    0.65% heap_multi_insert

perf report PATCHED:
-    4.49%     0.00%  postgres  postgres  [.] copy_table
   - copy_table
      - 4.48% CopyFrom
         - 2.36% NextCopyFrom
            - 1.81% CopyReadBinaryAttribute
                 1.47% ReceiveFunctionCall
         - 1.21% CopyMultiInsertInfoFlush
            - 1.21% CopyMultiInsertBufferFlush
               - 1.11% table_multi_insert
                  - 1.09% heap_multi_insert
                     - 0.71% RelationGetBufferForTuple
                        - 0.63% ReadBufferBI
                           - 0.62% ReadBufferExtended
                                0.62% ReadBuffer_common

I've tried to choose the table columns such that the binary send/recv
vs non-binary/plain/text copy has some benefit. The table has 100mn
rows, and is of 11GB size. I've measured the benefit using Melih's
helper function wait_for_rep(). Note that I had to compile source code
with -ggdb3 -O0 for perf report, otherwise with -O3 for performance
numbers:

wal_level = 'logical'
shared_buffers = '8GB'
wal_buffers = '1GB'
max_wal_size = '32GB'

create table foo(i int, n numeric, v varchar, b bytea, t timestamp
with time zone default current_timestamp);
insert into foo select i, i+1, md5(i::text), md5(i::text)::bytea from
generate_series(1, 100000000) i;

CREATE OR REPLACE PROCEDURE wait_for_rep()
LANGUAGE plpgsql
AS $$
BEGIN
    WHILE (SELECT count(*) != 0 FROM pg_subscription_rel WHERE
srsubstate <> 'r') LOOP COMMIT;
    END LOOP;
END;
$$;

---

From: Melih Mutlu <[email-masked]>
Date: Mon, 27 Feb 2023 22:52:29 +0300

Hi,

Thanks for all of your reviews!

So, I made some changes in the v10 according to your comments.

1- copy_format is changed to a boolean parameter copy_binary.
It sounds easier to use a boolean to enable/disable binary copy. Default
value is false, so nothing changes in the current behaviour if copy_binary
is not specified.
It's still persisted into the catalog. This is needed since its value will
be needed by tablesync workers later. And tablesync workers fetch
subscription configurations from the catalog.
In the copy_data case, it is not directly stored anywhere but it affects
the state of the table which is stored in the catalog. So, I guess this is
the convenient way if we decide to go with a new parameter.

2- copy_binary is not tied to another parameter
The patch does not disallow any combination of copy_binary with binary and
copy_data options. I tried to explain what binary copy can and cannot do in
the docs. Rest is up to the user now.

3- Removed version check for copy_binary
HEAD already does not have any check for binary option. Making binary copy
work only if publisher and subscriber are the same major version can be too
restricted.

4- Added separate test file
Although I believe 002_types.pl and 014_binary.pl can be improved more for
binary enabled subscription cases, this patch might not be the best place
to make those changes.
032_binary_copy.pl now has the tests for binary copy option. There are also
some regression tests in subscription.sql.

Finally, some other small fixes are done according to the reviews.

 Also, thanks Bharath for performance testing [1]. It can be seen that the
patch has some benefits.

I appreciate further thoughts/reviews.

[1]
https://www.postgresql.org/message-id/CALj2ACUfE08ZNjKK-nK9JiwGhwUMRLM%2BqRhNKTVM9HipFk7Fow%40mail.gmail.com

Thanks,
-- 
Melih Mutlu
Microsoft

---

From: vignesh C <[email-masked]>
Date: Tue, 28 Feb 2023 16:29:22 +0530

On Tue, 28 Feb 2023 at 01:22, Melih Mutlu <[email-masked]> wrote:
> It sounds easier to use a boolean to enable/disable binary copy. Default value is false, so nothing changes in the current behaviour if copy_binary is not specified.
> It's still persisted into the catalog. This is needed since its value will be needed by tablesync workers later. And tablesync workers fetch subscription configurations from the catalog.
> In the copy_data case, it is not directly stored anywhere but it affects the state of the table which is stored in the catalog. So, I guess this is the convenient way if we decide to go with a new parameter.
>
> The patch does not disallow any combination of copy_binary with binary and copy_data options. I tried to explain what binary copy can and cannot do in the docs. Rest is up to the user now.
>
> HEAD already does not have any check for binary option. Making binary copy work only if publisher and subscriber are the same major version can be too restricted.
>
> Although I believe 002_types.pl and 014_binary.pl can be improved more for binary enabled subscription cases, this patch might not be the best place to make those changes.
> 032_binary_copy.pl now has the tests for binary copy option. There are also some regression tests in subscription.sql.
>
>  Also, thanks Bharath for performance testing [1]. It can be seen that the patch has some benefits.
>

Thanks for the patch, Few comments:
1) Are primary key required for the tables, if not required we could
remove the primary key which will speed up the test by not creating
the indexes and inserting in the indexes. Even if required just create
for one of the tables:
+# Create tables on both sides of the replication
+my $ddl = qq(
+       CREATE TABLE public.test_numerical (
+               a INTEGER PRIMARY KEY,
+               b NUMERIC,
+               c FLOAT,
+               d BIGINT
+       );
+       CREATE TABLE public.test_arrays (
+               a INTEGER[] PRIMARY KEY,
+               b NUMERIC[],
+               c TEXT[]
+       );
+    CREATE TABLE public.test_range_array (
+               a INTEGER PRIMARY KEY,
+               b TSTZRANGE,
+               c int8range[]
+       );
+    CREATE TYPE public.test_comp_basic_t AS (a FLOAT, b TEXT, c INTEGER);
+    CREATE TABLE public.test_one_comp (
+               a INTEGER PRIMARY KEY,
+               b public.test_comp_basic_t
+       ););
+

2) We can have the Insert/Select of tables in the same order so that
it is easy to verify. test_range_array/test_one_comp
insertion/selection order was different.
+# Insert some content and before creating a subscription
+$node_publisher->safe_psql(
+       'postgres', qq(
+    INSERT INTO public.test_numerical (a, b, c, d) VALUES
+               (1, 1.2, 1.3, 10),
+        (2, 2.2, 2.3, 20);
+       INSERT INTO public.test_arrays (a, b, c) VALUES
+               ('{1,2,3}', '{1.1, 1.2, 1.3}', '{"one", "two", "three"}'),
+        ('{3,1,2}', '{1.3, 1.1, 1.2}', '{"three", "one", "two"}');
+    INSERT INTO test_range_array (a, b, c) VALUES
+               (1, tstzrange('Mon Aug 04 00:00:00 2014
CEST'::timestamptz, 'infinity'), '{"[1,2]", "[10,20]"}'),
+               (2, tstzrange('Sat Aug 02 00:00:00 2014
CEST'::timestamptz, 'Mon Aug 04 00:00:00 2014 CEST'::timestamptz),
'{"[2,3]", "[20,30]"}');
+       INSERT INTO test_one_comp (a, b) VALUES
+               (1, ROW(1.0, 'a', 1)),
+               (2, ROW(2.0, 'b', 2));
+       ));
+
+# Create the subscription with copy_binary = true
+my $publisher_connstring = $node_publisher->connstr . ' dbname=postgres';
+$node_subscriber->safe_psql('postgres',
+           "CREATE SUBSCRIPTION tsub CONNECTION '$publisher_connstring' "
+         . "PUBLICATION tpub WITH (slot_name = tpub_slot, copy_binary
= true)");
+
+# Ensure nodes are in sync with each other
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tsub');
+
+my $sync_check =  qq(
+       SET timezone = '+2';
+       SELECT a, b, c, d FROM test_numerical ORDER BY a;
+       SELECT a, b, c FROM test_arrays ORDER BY a;
+       SELECT a, b FROM test_one_comp ORDER BY a;
+       SELECT a, b, c FROM test_range_array ORDER BY a;
+);

3) Should we check the results for test_myvarchar table only, since
there is no change in other tables, we need not check other tables
again:
+# Now tablesync should succeed
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tsub');
+
+$sync_check =  qq(
+       SET timezone = '+2';
+       SELECT a, b, c, d FROM test_numerical ORDER BY a;
+       SELECT a, b, c FROM test_arrays ORDER BY a;
+       SELECT a, b FROM test_one_comp ORDER BY a;
+       SELECT a, b, c FROM test_range_array ORDER BY a;
+    SELECT a FROM test_myvarchar;
+);
+
+# Check the synced data on subscriber
+$result = $node_subscriber->safe_psql('postgres', $sync_check);

4) Similarly check only for test_mismatching_types in this case:
+# Cannot sync due to type mismatch
+$node_subscriber->wait_for_log(qr/ERROR: ( [A-Z0-9]+:)? incorrect
binary data format/);
+
+# Setting copy_binary to false should allow syncing
+$node_subscriber->safe_psql(
+    'postgres', qq(
+    ALTER SUBSCRIPTION tsub SET (copy_binary = false);));
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tsub');
+
+$sync_check =  qq(
+       SET timezone = '+2';
+       SELECT a, b, c, d FROM test_numerical ORDER BY a;
+       SELECT a, b, c FROM test_arrays ORDER BY a;
+       SELECT a, b FROM test_one_comp ORDER BY a;
+       SELECT a, b, c FROM test_range_array ORDER BY a;
+    SELECT a FROM test_myvarchar;
+    SELECT a FROM test_mismatching_types ORDER BY a;
+);
+
+# Check the synced data on subscribers
+$result = $node_subscriber->safe_psql('postgres', $sync_check);
+
+is( $result, '1|1.2|1.3|10
+2|2.2|2.3|20
+{1,2,3}|{1.1,1.2,1.3}|{one,two,three}
+{3,1,2}|{1.3,1.1,1.2}|{three,one,two}
+1|(1,a,1)
+2|(2,b,2)
+1|["2014-08-04 00:00:00+02",infinity)|{"[1,3)","[10,21)"}
+2|["2014-08-02 00:00:00+02","2014-08-04 00:00:00+02")|{"[2,4)","[20,31)"}
+a
+1
+2', 'check synced data on subscriber with copy_binary = false');

5) Should we change "Basic logical replication test" to "Test logical
replication of copy table in binary"
diff --git a/src/test/subscription/t/032_binary_copy.pl
b/src/test/subscription/t/032_binary_copy.pl
new file mode 100644
index 0000000000..bcad66e5ea
--- /dev/null
+++ b/src/test/subscription/t/032_binary_copy.pl
@@ -0,0 +1,223 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Basic logical replication test
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+

6) We can change "Initialize publisher node" to "Create publisher
node" to maintain consistency.
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;

7) Should "Insert some content and before creating a subscription." be
changed to:
"Insert some content before creating a subscription."

+# Publish all tables
+$node_publisher->safe_psql('postgres',
+       "CREATE PUBLICATION tpub FOR ALL TABLES");
+
+# Insert some content and before creating a subscription
+$node_publisher->safe_psql(
+       'postgres', qq(
+    INSERT INTO public.test_numerical (a, b, c, d) VALUES
+               (1, 1.2, 1.3, 10),

Regards,
Vignesh

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 28 Feb 2023 17:57:37 +0530

> So, I made some changes in the v10 according to your comments.

Thanks. Some quick comments on v10:

1.
+      <para>
+       If true, initial data synchronization will be performed in binary format
+      </para></entry>
It's not just the initial table sync right? The table sync can happen
at any other point of time when ALTER SUBSCRIPTION ... REFRESH
PUBLICATION WITH (copy = true) is run.
How about - "If true, the subscriber requests publication for
pre-existing data in binary format"?

2.
+          Specifies whether pre-existing data on the publisher will be copied
+          to the subscriber in binary format. The default is
<literal>false</literal>.
+          Binary format is very data type specific, it will not allow copying
+          between different column types as opposed to text format. Note that
+          if this option is enabled, all data types which will be copied during
+          the initial synchronization should have binary send and
receive functions.
+          If this option is disabled, data format for the initial
synchronization
+          will be text.
Perhaps, this should cover the recommended cases for enabling this new
option - something like below (may not need to have exact wording, but
the recommended cases?):

"It is recommended to enable this option only when 1) the column data
types have appropriate binary send/receive functions, 2) not
replicating between different major versions or different platforms,
3) both publisher and subscriber tables have the exact same column
types (not when replicating from smallint to int or numeric to int8
and so on), 4) both publisher and subscriber supports COPY with binary
option, otherwise the table copy can fail."

3. I think the newly added tests must verify if the binary COPY is
picked up when enabled. Perhaps, looking at the publisher's server log
for 'COPY ... WITH BINARY format'? Maybe it's an overkill, otherwise,
we have no way of testing that the option took effect.

---

From: Jelte Fennema <[email-masked]>
Date: Tue, 28 Feb 2023 14:25:11 +0100

> we have no way of testing that the option took effect.

Another way to test that BINARY is enabled could be to trigger one
of the failure cases.

---

From: Melih Mutlu <[email-masked]>
Date: Tue, 28 Feb 2023 17:20:47 +0300

Hi,

Attached v11.

vignesh C <[email-masked]>, 28 Şub 2023 Sal, 13:59 tarihinde şunu

> the indexes and inserting in the indexes. Even if required just create

I think that having a primary key in tables for logical replication tests
is good for checking if log. rep. duplicates any row. Other tests also have
primary keys in almost all tables.

Bharath Rupireddy <[email-masked]>, 28 Şub 2023

> pre-existing data in binary format"?

I changed it as you suggested.
I sometimes feel like the phrase "initial sync" is used for initial sync of
a table, not a subscription. Or table syncs triggered by ALTER SUBSCRIPTION
are ignored in some places where "initial sync" is used.

2.
> option, otherwise the table copy can fail."

I added a line stating that binary format is less portable across machine
architectures and versions as stated in COPY [1].
I don't think we should add line saying "recommended", but state the
restrictions clearly instead. It's also similar in COPY docs as well.
I think the explanation now covers all your points, right?

Jelte Fennema <[email-masked]>, 28 Şub 2023 Sal, 16:25 tarihinde şunu

> Another way to test that BINARY is enabled could be to trigger one

Yes, there is already a failure case for binary copy which resolves with
swithcing binary_copy to false.
But I also added checks for publisher logs now too.

[1] https://www.postgresql.org/docs/devel/sql-copy.html

Thanks,
-- 
Melih Mutlu
Microsoft