Allow logical replication to copy tables in binary format

On Wed, Aug 10, 2022, at 12:03 PM, Melih Mutlu wrote:

I see that logical replication subscriptions have an option to enable binary [1].
When it's enabled, subscription requests publisher to send data in binary format.
But this is only the case for apply phase. In tablesync, tables are still copied as text.

This option could have been included in the commit 9de77b54531; it wasn't.
Maybe it wasn't considered because the initial table synchronization can be a
separate step in your logical replication setup idk. I agree that the binary
option should be available for the initial table synchronization.

To copy tables, COPY command is used and that command supports copying in binary. So it seemed to me possible to copy in binary for tablesync too.
I'm not sure if there is a reason to always copy tables in text format. But I couldn't see why not to do it in binary if it's enabled.

The reason to use text format is that it is error prone. There are restrictions
while using the binary format. For example, if your schema has different data
types for a certain column, the copy will fail. Even with such restrictions, I
think it is worth adding it.

You can find the small patch that only enables binary copy attached.

I have a few points about your implementation.

* Are we considering to support prior Postgres versions too? These releases
support binary mode but it could be an unexpected behavior (initial sync in
binary mode) for a publisher using 14 or 15 and a subscriber using 16. IMO
you should only allow it for publisher on 16 or later.
* Docs should say that the binary option also applies to initial table
synchronization and possibly emphasize some of the restrictions.
* Tests. Are the current tests enough? 014_binary.pl.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira <euler@eulerto.com> wrote:

On Wed, Aug 10, 2022, at 12:03 PM, Melih Mutlu wrote:

I see that logical replication subscriptions have an option to enable binary [1].
When it's enabled, subscription requests publisher to send data in binary format.
But this is only the case for apply phase. In tablesync, tables are still copied as text.

This option could have been included in the commit 9de77b54531; it wasn't.
Maybe it wasn't considered because the initial table synchronization can be a
separate step in your logical replication setup idk. I agree that the binary
option should be available for the initial table synchronization.

To copy tables, COPY command is used and that command supports copying in binary. So it seemed to me possible to copy in binary for tablesync too.
I'm not sure if there is a reason to always copy tables in text format. But I couldn't see why not to do it in binary if it's enabled.

The reason to use text format is that it is error prone. There are restrictions
while using the binary format. For example, if your schema has different data
types for a certain column, the copy will fail.

Won't such restrictions hold true even during replication?

--
With Regards,
Amit Kapila.

On Thu, Aug 11, 2022, at 8:04 AM, Amit Kapila wrote:

On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira <euler@eulerto.com> wrote:

The reason to use text format is that it is error prone. There are restrictions
while using the binary format. For example, if your schema has different data
types for a certain column, the copy will fail.

Won't such restrictions hold true even during replication?

I expect that the COPY code matches the proto.c code. The point is that table
sync is decoupled from the logical replication. Hence, we should emphasize in
the documentation that the restrictions *also* apply to the initial table
synchronization.

--
Euler Taveira
EDB https://www.enterprisedb.com/

Euler Taveira <euler@eulerto.com>, 11 Ağu 2022 Per, 16:27 tarihinde şunu
yazdı:

On Thu, Aug 11, 2022, at 8:04 AM, Amit Kapila wrote:

On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira <euler@eulerto.com> wrote:

The reason to use text format is that it is error prone. There are

restrictions

while using the binary format. For example, if your schema has different

data

types for a certain column, the copy will fail.

Won't such restrictions hold true even during replication?

I expect that the COPY code matches the proto.c code. The point is that
table
sync is decoupled from the logical replication. Hence, we should emphasize
in
the documentation that the restrictions *also* apply to the initial table
synchronization.

If such restrictions are already the case for replication phase after
initial table sync, then it shouldn't prevent us from enabling binary
option for table sync. Right?
But yes, it needs to be stated somewhere.

Euler Taveira <euler@eulerto.com>, 11 Ağu 2022 Per, 05:03 tarihinde şunu
yazdı

I have a few points about your implementation.

* Are we considering to support prior Postgres versions too? These releases
support binary mode but it could be an unexpected behavior (initial sync
in
binary mode) for a publisher using 14 or 15 and a subscriber using 16.
IMO
you should only allow it for publisher on 16 or later.

How is any issue that might occur due to version mismatch being handled
right now in repliaction after table sync?
What I understand from the documentation is if replication can fail due to
using different pg versions, it just fails. So binary option cannot be
used. [1]https://www.postgresql.org/docs/15/sql-createsubscription.html <https://www.postgresql.org/docs/15/sql-createsubscription.html&gt;
Do you think that this is more serious for table sync and we need to
restrict binary option with different publisher and subscriber versions?
But not for replication?

* Docs should say that the binary option also applies to initial table

synchronization and possibly emphasize some of the restrictions.
* Tests. Are the current tests enough? 014_binary.pl.

You're right on both points. I just wanted to know your opinions on this
first. Then the patch will need some tests and proper documentation.

[1]: https://www.postgresql.org/docs/15/sql-createsubscription.html <https://www.postgresql.org/docs/15/sql-createsubscription.html&gt;
<https://www.postgresql.org/docs/15/sql-createsubscription.html&gt;

Best,
Melih

On Thu, Aug 11, 2022, at 10:46 AM, Melih Mutlu wrote:

If such restrictions are already the case for replication phase after initial table sync, then it shouldn't prevent us from enabling binary option for table sync. Right?

I didn't carefully examine the COPY code but I won't expect significant
differences (related to text vs binary mode) from the logical replication
protocol. After inspecting the git history, I took my argument back after
checking the commit 670c0a1d474. The initial commit 9de77b54531 imposes some
restrictions (user-defined arrays and composite types) as mentioned in the
commit message but it was removed in 670c0a1d474. My main concern is to break a
scenario that was previously working (14 -> 15) but after a subscriber upgrade
it won't (14 -> 16). I would say that you should test some scenarios:
014_binary.pl and also custom data types, same column with different data
types, etc.

How is any issue that might occur due to version mismatch being handled right now in repliaction after table sync?
What I understand from the documentation is if replication can fail due to using different pg versions, it just fails. So binary option cannot be used. [1]
Do you think that this is more serious for table sync and we need to restrict binary option with different publisher and subscriber versions? But not for replication?

It is a conservative argument. If we didn't allow a publisher to run COPY in
binary mode while using previous Postgres versions, we know that it works. (At
least there aren't bug reports for logical replication using binary option.)
Since one of the main use cases for logical replication is migration, I'm
concerned that it may not work (even if the binary option defaults to false,
someone can decide to use it for performance reasons).

I did a quick test and the failure while using binary mode is not clear. Since
you are modifying this code, you could probably provide additional patch(es) to
make it clear that there is an error (due to some documented restriction).

--
Euler Taveira
EDB https://www.enterprisedb.com/

Euler Taveira <euler@eulerto.com>, 11 Ağu 2022 Per, 20:16 tarihinde şunu
yazdı:

My main concern is to break a scenario that was previously working (14 ->
15) but after a subscriber upgrade
it won't (14 -> 16).

Fair concern. Some cases that might break the logical replication with
version upgrade would be:
1- Usage of different binary formats between publisher and subscriber. As
stated here [1]https://www.postgresql.org/docs/devel/sql-copy.html, binary format has been changed after v7.4.
But I don't think this would be a concern, since we wouldn't even have
logical replication with 7.4 and earlier versions.
2- Lack (or mismatch) of binary send/receive functions for custom data
types would cause failures. This case can already cause failures with
current logical replication, regardless of binary copy. Stated here [2]https://www.postgresql.org/docs/devel/sql-createsubscription.html.
3- Copying in binary format would work with the same schemas. Currently,
logical replication does not require the exact same schemas in publisher
and subscriber.
This is an additional restriction that comes with the COPY command.

If a logical replication has been set up with different schemas and
subscription is created with the binary option, then yes this would break
things.
This restriction can be clearly stated and wouldn't be unexpected though.

I'm also okay with allowing binary copy only for v16 or later, if you think
it would be safer and no one disagrees with that.
What are your thoughts?

I would say that you should test some scenarios:

014_binary.pl and also custom data types, same column with different data
types, etc.

I added scenarios in two tests to test binary copy:
014_binary.pl: This was already testing subscriptions with binary option
enabled. I added an extra step to insert initial data before creating the
subscription.
So that we can test initial table sync with binary copy.

002_types.pl: This file was already testing more complex data types. I
added an extra subscriber node to create a subscription with binary option
enabled.
This way, it now tests binary copy with different custom types.

Do you think these would be enough in terms of testing?

Attached patch also includes some additions to the doc along with the
tests.

Thanks,
Melih

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

Hi,

On 2022-08-10 18:03:56 +0300, Melih Mutlu wrote:

To copy tables, COPY command is used and that command supports copying in
binary. So it seemed to me possible to copy in binary for tablesync too.
I'm not sure if there is a reason to always copy tables in text format.

It'd be good to collect some performance numbers justifying this. I'd expect
decent gains if there's e.g. a bytea or timestamptz column involved.

Greetings,

Andres Freund

Hello,

Andres Freund <andres@anarazel.de>, 2 Eyl 2022 Cum, 01:25 tarihinde şunu
yazdı:

It'd be good to collect some performance numbers justifying this. I'd
expect
decent gains if there's e.g. a bytea or timestamptz column involved.

Experimented the binary copy with a quick setup.

- Created a "temp" table with bytea and timestamptz columns

postgres=# \d temp
Table "public.temp"
Column | Type | Collation | Nullable | Default
--------+--------------------------+-----------+----------+---------
i | integer | | |
b | bytea | | |
t | timestamp with time zone | | |

- Loaded with ~1GB data

postgres=# SELECT pg_size_pretty( pg_total_relation_size('temp') );
pg_size_pretty
----------------
1137 MB
(1 row)

- Created a publication with only this "temp" table.
- Created a subscription with binary enabled on instances from master
branch and this patch.
- Timed the tablesync process by calling the following procedure:

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; $$;

Hera are averaged results of multiple consecutive runs from both master
branch and the patch:

master (binary enabled but no binary copy): 20007.7948 ms
the patch (allows binary copy): 8874,869 ms

Seems like a good improvement.
What are your thoughts on this patch?

Best,
Melih

Hi hackers,

I just wanted to gently ping to hear what you all think about this patch.

Appreciate any feedback/thougths.

Thanks,
Melih

On Tuesday, August 16, 2022 2:04 AM Melih Mutlu <m.melihmutlu@gmail.com> wrote:

Attached patch also includes some additions to the doc along with the tests.

Hi, thank you for updating the patch. Minor review comments for the v2.

(1) whitespace issues

Please fix below whitespace errors.

$ git apply v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch
v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:39: trailing whitespace.
binary format.(See <xref linkend="sql-copy"/>.)
v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:120: trailing whitespace.

v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:460: trailing whitespace.
);
warning: 3 lines add whitespace errors.

(2) Suggestion to update another general description about the subscription

Kindly have a look at doc/src/sgml/logical-replication.sgml.

"The data types of the columns do not need to match,
as long as the text representation of the data can be converted to the target type.
For example, you can replicate from a column of type integer to a column of type bigint."

With the patch, I think we have an impact about those descriptions
since enabling the binary option for a subscription and executing the
initial synchronization requires the same data types for binary format.

I suggest that we update those descriptions as well.

(3) shouldn't test that we fail expectedly with binary copy for different types ?

How about having a test that we correctly fail with different data types
between the publisher and the subscriber, for instance ?

(4) Error message of the binary format copy

I've gotten below message from data type contradiction (between integer and bigint).
Probably, this is unclear for the users to understand the direct cause
and needs to be adjusted ?
This might be a similar comment Euler mentioned in [1]/messages/by-id/f1d58324-8df4-4bb5-a546-8c741c2e6fa8@www.fastmail.com.

2022-09-16 11:54:54.835 UTC [4570] ERROR: insufficient data left in message
2022-09-16 11:54:54.835 UTC [4570] CONTEXT: COPY tab, line 1, column id

(5) Minor adjustment of the test comment in 002_types.pl.

+is( $result, $sync_result, 'check initial sync on subscriber');
+is( $result_binary, $sync_result, 'check initial sync on subscriber in binary');

# Insert initial test data

There are two same comments which say "Insert initial test data" in this file.
We need to update them, one for the initial table sync and
the other for the application of changes.

[1]: /messages/by-id/f1d58324-8df4-4bb5-a546-8c741c2e6fa8@www.fastmail.com

Best Regards,
Takamichi Osumi

Hi

Few more minor comments.

On Tuesday, August 16, 2022 2:04 AM Melih Mutlu <m.melihmutlu@gmail.com> wrote:

My main concern is to break a scenario that was previously working (14
-> 15) but after a subscriber upgrade
it won't (14 -> 16).

Fair concern. Some cases that might break the logical replication with version
upgrade would be:

...

3- Copying in binary format would work with the same schemas. Currently,
logical replication does not require the exact same schemas in publisher and
subscriber.
This is an additional restriction that comes with the COPY command.

If a logical replication has been set up with different schemas and subscription
is created with the binary option, then yes this would break things.
This restriction can be clearly stated and wouldn't be unexpected though.

I'm also okay with allowing binary copy only for v16 or later, if you think it would
be safer and no one disagrees with that.
What are your thoughts?

I agree with the direction to support binary copy for v16 and later.

IIUC, the binary format replication with different data types fails even during apply phase on HEAD.
I thought that means, the upgrade concern only applies to a scenario that the user executes
only initial table synchronizations between the publisher and subscriber
and doesn't replicate any data at apply phase after that. I would say
this isn't a valid scenario and your proposal makes sense.

Best Regards,
Takamichi Osumi

Hi Takamichi,

Thanks for your reviews.

I addressed your reviews, please find the attached patch.

osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>, 16 Eyl 2022 Cum,
16:51 tarihinde şunu yazdı:

(1) whitespace issues

Fixed

(2) Suggestion to update another general description about the subscription

Kindly have a look at doc/src/sgml/logical-replication.sgml.

"The data types of the columns do not need to match,
as long as the text representation of the data can be converted to the
target type.
For example, you can replicate from a column of type integer to a column
of type bigint."

With the patch, I think we have an impact about those descriptions
since enabling the binary option for a subscription and executing the
initial synchronization requires the same data types for binary format.

I suggest that we update those descriptions as well.

You're right, this needs to be stated in the docs. Modified descriptions
accordingly.

(3) shouldn't test that we fail expectedly with binary copy for different
types ?

How about having a test that we correctly fail with different data types
between the publisher and the subscriber, for instance ?

Modified 002_types.pl test such that it now tests the replication between
different data types.
It's expected to fail if the binary is enabled, and succeed if not.

(4) Error message of the binary format copy

I've gotten below message from data type contradiction (between integer
and bigint).
Probably, this is unclear for the users to understand the direct cause
and needs to be adjusted ?
This might be a similar comment Euler mentioned in [1].

2022-09-16 11:54:54.835 UTC [4570] ERROR: insufficient data left in
message
2022-09-16 11:54:54.835 UTC [4570] CONTEXT: COPY tab, line 1, column id

It's already unclear for users to understand what's the issue if they're
copying data between different column types via the COPY command.
This issue comes from COPY, and logical replication just utilizes COPY.
I don't think it would make sense to adjust an error message from a
functionality which logical replication only uses and has no direct impact
on.
It might be better to do this in a separate patch. What do you think?

(5) Minor adjustment of the test comment in 002_types.pl.

+is( $result, $sync_result, 'check initial sync on subscriber');
+is( $result_binary, $sync_result, 'check initial sync on subscriber in
binary');

# Insert initial test data

There are two same comments which say "Insert initial test data" in this
file.
We need to update them, one for the initial table sync and
the other for the application of changes.

Fixed.

I agree with the direction to support binary copy for v16 and later.

IIUC, the binary format replication with different data types fails even
during apply phase on HEAD.
I thought that means, the upgrade concern only applies to a scenario that
the user executes
only initial table synchronizations between the publisher and subscriber
and doesn't replicate any data at apply phase after that. I would say
this isn't a valid scenario and your proposal makes sense.

No, logical replication in binary does not fail on apply phase if data
types are different.
The concern with upgrade (if data types are not the same) would be not
being able to create a new subscription with binary enabled or replicate
new tables added into publication.
Replication of tables from existing subscriptions would not be affected by
this change since they will already be in the apply phase, not tablesync.
Do you think this would still be an issue?

Thanks,
Melih

Hi,

On Monday, October 3, 2022 8:50 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:

(4) Error message of the binary format copy

I've gotten below message from data type contradiction (between
integer and bigint).
Probably, this is unclear for the users to understand the direct cause
and needs to be adjusted ?
This might be a similar comment Euler mentioned in [1].

2022-09-16 11:54:54.835 UTC [4570] ERROR: insufficient data left in
message
2022-09-16 11:54:54.835 UTC [4570] CONTEXT: COPY tab, line 1,
column id

It's already unclear for users to understand what's the issue if they're copying
data between different column types via the COPY command.
This issue comes from COPY, and logical replication just utilizes COPY.
I don't think it would make sense to adjust an error message from a functionality
which logical replication only uses and has no direct impact on.
It might be better to do this in a separate patch. What do you think?

Yes, makes sense. It should be a separate patch.

I agree with the direction to support binary copy for v16 and later.

IIUC, the binary format replication with different data types fails even
during apply phase on HEAD.
I thought that means, the upgrade concern only applies to a scenario
that the user executes
only initial table synchronizations between the publisher and subscriber
and doesn't replicate any data at apply phase after that. I would say
this isn't a valid scenario and your proposal makes sense.

No, logical replication in binary does not fail on apply phase if data types are
different.

With HEAD, I observe in some case we fail at apply phase because of different data types like
integer vs. bigint as written scenario in [1]binary format test that we fail for different types on apply phase on HEAD. In short, I think we can slightly
adjust your documentation and make it more general so that the description applies to
both table sync phase and apply phase.

I'll suggest a below change for your sentence of logical-replication.sgml.
FROM:
In binary case, it is not allowed to replicate data between different types due to restrictions inherited from COPY.
TO:
Binary format is type specific and does not allow to replicate data between different types according to its
restrictions.

If my idea above is correct, then I feel we can remove all the fixes for create_subscription.sgml.
I'm not sure if I should pursue this perspective of the document improvement
any further after this email, since this isn't essentially because of this patch.

The concern with upgrade (if data types are not the same) would be not being
able to create a new subscription with binary enabled or replicate new tables
added into publication.
Replication of tables from existing subscriptions would not be affected by this
change since they will already be in the apply phase, not tablesync.
Do you think this would still be an issue?

Okay, thanks for explaining this. I understand that
the upgrade concern applies to the table sync that is executed
between text format (before the patch) and binary format (after the patch).

[1]: binary format test that we fail for different types on apply phase on HEAD

<publisher>
create table tab (id integer);
insert into tab values (100);
create publication mypub for table tab;

<subscriber>
create table tab (id bigint);
create subscription mysub connection '...' publication mypub with (copy_data = false, binary = true, disable_on_error = true);

-- wait for several seconds

<subscriber>
select srsubid, srrelid, srrelid::regclass, srsubstate, srsublsn from pg_subscription_rel; -- check the status as 'r' for the relation
select * from tab; -- confirm we don't copy the initial data on the pub

<publisher>
insert into tab values (1), (2);

-- wait for several seconds

<subscriber>
select subname, subenabled from pg_subscription; -- shows 'f' for the 2nd column because of an error
select * from tab -- no records

This error doesn't happen when we adopt 'integer' on the subscriber aligned with the publisher
and we can see the two records on the subscriber.

Best Regards,
Takamichi Osumi

Hello,

osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>, 12 Eki 2022 Çar,
04:36 tarihinde şunu yazdı:

I agree with the direction to support binary copy for v16 and

later.

IIUC, the binary format replication with different data types

fails even

during apply phase on HEAD.
I thought that means, the upgrade concern only applies to a

scenario

that the user executes
only initial table synchronizations between the publisher and

subscriber

and doesn't replicate any data at apply phase after that. I would

say

this isn't a valid scenario and your proposal makes sense.

No, logical replication in binary does not fail on apply phase if data

types are

different.

With HEAD, I observe in some case we fail at apply phase because of
different data types like
integer vs. bigint as written scenario in [1]. In short, I think we can
slightly
adjust your documentation and make it more general so that the description
applies to
both table sync phase and apply phase.

Yes, you're right. I somehow had the impression that HEAD supports
replication between different types in binary.
But as can be shown in the scenario you mentioned, it does not work.

I'll suggest a below change for your sentence of logical-replication.sgml.

FROM:
In binary case, it is not allowed to replicate data between different
types due to restrictions inherited from COPY.
TO:
Binary format is type specific and does not allow to replicate data
between different types according to its
restrictions.

In this case, this change makes sense since this patch does actually not
introduce this issue. It already exists in HEAD too.

If my idea above is correct, then I feel we can remove all the fixes for
create_subscription.sgml.
I'm not sure if I should pursue this perspective of the document
improvement
any further after this email, since this isn't essentially because of this
patch.

I'm only keeping the following change in create_subscription.sgml to
indicate binary option copies in binary format now.

-          Specifies whether the subscription will request the publisher to
-          send the data in binary format (as opposed to text).
+          Specifies whether the subscription will copy the initial data to
+          synchronize relations in binary format and also request the
publisher
+          to send the data in binary format too (as opposed to text).

The concern with upgrade (if data types are not the same) would be not

being

able to create a new subscription with binary enabled or replicate new

tables

added into publication.
Replication of tables from existing subscriptions would not be affected

by this

change since they will already be in the apply phase, not tablesync.
Do you think this would still be an issue?

Okay, thanks for explaining this. I understand that
the upgrade concern applies to the table sync that is executed
between text format (before the patch) and binary format (after the patch).

I was thinking apply would work with different types in binary format.
Since apply also would not work, then the scenario that I tried to explain
earlier is not a concern anymore.

Attached patch with updated version of this patch.

Thanks,
Melih

On Mon, Nov 14, 2022 8:08 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:

Attached patch with updated version of this patch.

Thanks for your patch.

I tried to do a performance test for this patch, the result looks good to me.
(The steps are similar to what Melih shared [1]/messages/by-id/CAGPVpCQEKDVKQPf6OFQ-9WiRYB1YRejm--YJTuwgzuvj1LEJ2A@mail.gmail.com.)

The time to synchronize about 1GB data in binary (the average of the middle
eight was taken):
HEAD: 16.854 s
Patched: 6.805 s

Besides, here are some comments.

1.
+# Binary enabled subscription should fail
+$node_subscriber_binary->wait_for_log("ERROR:  insufficient data left in message");

Should it be changed to "ERROR: ( [A-Z0-9]+:)? ", like other subscription tests.

2.
+# Binary disabled subscription should succeed
+$node_publisher->wait_for_catchup('tap_sub');

If we want to wait for table synchronization to finish, should we call
wait_for_subscription_sync()?

3.
I also think it might be better to support copy binary only for publishers of
v16 or later. Do you plan to implement it in the patch?

[1]: /messages/by-id/CAGPVpCQEKDVKQPf6OFQ-9WiRYB1YRejm--YJTuwgzuvj1LEJ2A@mail.gmail.com

Regards,
Shi yu

Hi,

Thanks for your review.

shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>, 11 Oca 2023 Çar, 11:56
tarihinde şunu yazdı:

On Mon, Nov 14, 2022 8:08 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
1.
+# Binary enabled subscription should fail
+$node_subscriber_binary->wait_for_log("ERROR:  insufficient data left in
message");

Should it be changed to "ERROR: ( [A-Z0-9]+:)? ", like other subscription
tests.

Done.

2.
+# Binary disabled subscription should succeed
+$node_publisher->wait_for_catchup('tap_sub');

If we want to wait for table synchronization to finish, should we call
wait_for_subscription_sync()?

Done.

3.
I also think it might be better to support copy binary only for publishers
of
v16 or later. Do you plan to implement it in the patch?

Done.

Thanks,
--
Melih Mutlu
Microsoft

On Wed, 11 Jan 2023 at 16:14, Melih Mutlu <m.melihmutlu@gmail.com> wrote:

Hi,

Thanks for your review.

shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>, 11 Oca 2023 Çar, 11:56 tarihinde şunu yazdı:

On Mon, Nov 14, 2022 8:08 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
1.
+# Binary enabled subscription should fail
+$node_subscriber_binary->wait_for_log("ERROR:  insufficient data left in message");

Should it be changed to "ERROR: ( [A-Z0-9]+:)? ", like other subscription tests.

Done.

2.
+# Binary disabled subscription should succeed
+$node_publisher->wait_for_catchup('tap_sub');

If we want to wait for table synchronization to finish, should we call
wait_for_subscription_sync()?

Done.

3.
I also think it might be better to support copy binary only for publishers of
v16 or later. Do you plan to implement it in the patch?

Done.

For some reason CFBot is not able to apply the patch as in [1]http://cfbot.cputube.org/patch_41_3840.log, Could
you have a look and post an updated patch if required:
=== Applying patches on top of PostgreSQL commit ID
c96de2ce1782116bd0489b1cd69ba88189a495e8 ===
=== applying patch
./v5-0001-Allow-logical-replication-to-copy-table-in-binary.patch
gpatch: **** Only garbage was found in the patch input.

[1]: http://cfbot.cputube.org/patch_41_3840.log

Regards,
Vignesh

On Wednesday, January 11, 2023 7:45 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:

Thanks for your review.
Done.

Hi, minor comments on v5.

(1) publisher's version check

+       /* If the publisher is v16 or later, copy in binary format.*/
+       server_version = walrcv_server_version(LogRepWorkerWalRcvConn);
+       if (server_version >=160000 && MySubscription->binary)
+       {
+               appendStringInfoString(&cmd, "  WITH (FORMAT binary)");
+               options = lappend(options, makeDefElem("format", (Node *) makeString("binary"), -1));
+       }
+
+       elog(LOG, "version: %i, %s", server_version, cmd.data);

(1-1) There is no need to log the version and the query by elog here.
(1-2) Also, I suggest we can remove the server_version variable itself,
because we have only one actual reference for it.
There is a style that we call walrcv_server_version in the
'if condition' directly like existing codes in fetch_remote_table_info().
(1-3) Suggestion to improve comments.
FROM:
/* If the publisher is v16 or later, copy in binary format.*/
TO:
/* If the publisher is v16 or later, copy data in the required data format. */

(2) Minor suggestion for some test code alignment.

 $result =
   $node_subscriber->safe_psql('postgres',
        "SELECT sum(a) FROM tst_dom_constr");
-is($result, '21', 'sql-function constraint on domain');
+is($result, '33', 'sql-function constraint on domain');
+
+$result_binary =
+  $node_subscriber->safe_psql('postgres',
+       "SELECT sum(a) FROM tst_dom_constr");
+is($result_binary, '33', 'sql-function constraint on domain');

I think if we change the order of this part of check like below, then
it would look more aligned with other existing test codes introduced by this patch.

---
my $domain_check = 'SELECT sum(a) FROM tst_dom_constr';
$result = $node_subscriber->safe_psql('postgres', $domain_check);
$result_binary = $node_subscriber->safe_psql('postgres', $domain_check);
is($result, '33', 'sql-function constraint on domain');
is($result_binary, '33', 'sql-function constraint on domain in binary');
---

Best Regards,
Takamichi Osumi

Hi,

Thanks for your reviews.

Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com>, 12 Oca 2023 Per,
06:07 tarihinde şunu yazdı:

On Wednesday, January 11, 2023 7:45 PM Melih Mutlu <m.melihmutlu@gmail.com>
wrote:
(1-1) There is no need to log the version and the query by elog here.
(1-2) Also, I suggest we can remove the server_version variable itself,
because we have only one actual reference for it.
There is a style that we call walrcv_server_version in the
'if condition' directly like existing codes in
fetch_remote_table_info().
(1-3) Suggestion to improve comments.
FROM:
/* If the publisher is v16 or later, copy in binary format.*/
TO:
/* If the publisher is v16 or later, copy data in the required data
format. */

Forgot to remove that LOG line. Removed it now and applied other
suggestions too.

I think if we change the order of this part of check like below, then
it would look more aligned with other existing test codes introduced by
this patch.

Right. Changed it to make it more aligned with the rest.

Thanks,
--
Melih Mutlu
Microsoft