RE: Allow logical replication to copy tables in binary format

On Tuesday, August 16, 2022 2:04 AM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)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].

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] - https://www.postgresql.org/message-id/f1d58324-8df4-4bb5-a546-8c741c2e6fa8%40www.fastmail.com

Best Regards,
Takamichi Osumi