Re: Allow logical replication to copy tables in binary format

On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
>

Thanks for providing an updated patch.

>> On Thu, Jan 12, 2023 at 1:53 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
>> 1. The performance numbers posted upthread [1] look impressive for the
>> use-case tried, that's a 2.25X improvement or 55.6% reduction in
>> execution times. However, it'll be great to run a few more varied
>> tests to confirm the benefit.
>
> Sure, do you have any specific test case or suggestion in mind?

Have a huge amount of publisher's table (with mix of columns like int,
text, double, bytea and so on) prior data so that the subscriber's
table sync workers have to do a "good" amount of work to copy, then
measure the copy_table time with and without patch.

>> 2. It'll be great to capture the perf report to see if the time spent
>> in copy_table() is reduced with the patch.
>
> Will provide that in another email soon.

Thanks.

>> 4. Why to add tests to existing 002_types.pl? Can we add a new file
>> with all the data types covered?
>
> Since 002_types.pl is where the replication of data types are covered. I thought it would be okay to test replication with the binary option in that file.
> Sure, we can add a new file with different data types for testing subscriptions with binary option. But then I feel like it would have too many duplicated lines with 002_types.pl.
> If you think that 002_types.pl lacks some data types needs to be tested, then we should add those into 002_types.pl too whether we test subscription with binary option in that file or some other place, right?
>
> Previously, it wasn't actually testing the initial table sync since all tables were empty when subscription was created.
> I just simply split the data initially inserted to test initial table sync.
>
> With this patch, it inserts the first two rows for all data types before subscriptions get created.
> You can see these lines:

It'd be better and clearer to have a separate TAP test file IMO since
the focus of the feature here isn't to just test for data types. With
separate tests, you can verify "ERROR: incorrect binary data format
in logical replication column 1" cases.

>> 8. Note that the COPY binary format isn't portable across platforms
>> (Windows to Linux for instance) or major versions
>> https://www.postgresql.org/docs/devel/sql-copy.html, whereas, logical
>> replication is https://www.postgresql.org/docs/devel/logical-replication.html.
>> I don't see any handling of such cases in copy_table but only a check
>> for the publisher version. I think we need to account for all the
>> cases - allow binary COPY only when publisher and subscriber are of
>> same versions, architectures, platforms. The trick here is how we
>> identify if the subscriber is of the same type and taste
>> (architectures and platforms) as the publisher. Looking for
>> PG_VERSION_STR of publisher and subscriber might be naive, but I'm not
>> sure if there's a better way to do it.
>
> I think having the "copy_format" option with text as default, like I replied to your 3rd review above, will keep things as they are now.
> The patch now will only allow users to choose binary copy as well, if they want it and acknowledge the limitations that come with binary copy.
> COPY command's portability issues shouldn't be an issue right now, since the patch still supports text format. Right?

With the above said, do you think checking for publisher versions is
needed? The user can decide to enable/disable binary COPY based on the
publisher's version no?
+ /* If the publisher is v16 or later, specify the format to copy data. */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000)
+ {

Few more comments on v7:
1.
+ Specifies the format in which pre-existing data on the publisher will
+ copied to the subscriber. Supported formats are
+ <literal>text</literal> and <literal>binary</literal>. The default is
+ <literal>text</literal>.
It'll be good to call out the cases in the documentation as to where
copy_format can be enabled and needs to be disabled.

2.
+ errmsg("%s value should be either \"text\" or \"binary\"",
How about "value must be either ...."?

3.
+ if (!opts->binary &&
+ opts->copy_format == LOGICALREP_COPY_AS_BINARY)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s and %s are mutually exclusive options",
+ "binary = false", "copy_format = binary")));

+ "CREATE SUBSCRIPTION tap_sub_binary CONNECTION
'$publisher_connstr' PUBLICATION tap_pub WITH (slot_name =
tap_sub_binary_slot, binary = true, copy_format = 'binary')"
Why should the subscription's binary option and copy_format option be
tied at all? Tying these two options hurts usability. Is there a
fundamental reason? I think they both are/must be independent. One
deals with data types and another deals with how initial table data is
copied.

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