On Tuesday, August 16, 2022 2:04 AM Melih Mutlu <m.melihmu...@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].
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
