public inbox for [email protected]  

help / color / mirror / Atom feed

From: [email protected] <[email protected]>
To: 'Melih Mutlu' <[email protected]>
To: Euler Taveira <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: RE: Allow logical replication to copy tables in binary format
Date: Fri, 16 Sep 2022 13:51:07 +0000
Message-ID: <TYCPR01MB837354C136F914D2AE174AE1ED489@TYCPR01MB8373.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAGPVpCRjdFHa71Qqd4Xj22UTa5=-NBUPgtT==X0w7TfWUZrpUg@mail.gmail.com>
References: <CAGPVpCQvAziCLknEnygY0v1-KBtg+Om-9JHJYZOnNPKFJPompw@mail.gmail.com>
	<[email protected]>
	<CAA4eK1KxYj=3rYHbPMQvNSFytHdyS5=q0D3A1T=uerC7HMFCtg@mail.gmail.com>
	<[email protected]>
	<CAGPVpCR15YfE=4hthY6S2OC5G0nj0QM4oE2bV769+FANM_UvwQ@mail.gmail.com>
	<[email protected]>
	<CAGPVpCRjdFHa71Qqd4Xj22UTa5=-NBUPgtT==X0w7TfWUZrpUg@mail.gmail.com>

On Tuesday, August 16, 2022 2:04 AM Melih Mutlu <[email protected]> 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

view thread (18+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Allow logical replication to copy tables in binary format
  In-Reply-To: <TYCPR01MB837354C136F914D2AE174AE1ED489@TYCPR01MB8373.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox