Re: Allow logical replication to copy tables in binary format

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

To: Melih Mutlu <m.melihmutlu@gmail.com>

Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, Euler Taveira <euler@eulerto.com>, Amit Kapila <amit.kapila16@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>

Date: 2023-02-21T13:48:00Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Allow logical replication to copy tables in binary format.
- ecb696527c01 16.0 landed
Common function for percent placeholder replacement
- c96de2ce1782 16.0 cited
Doc: add XML ID attributes to <sectN> and <varlistentry> tags.
- 78ee60ed84bb 16.0 cited
Re-order disable_on_error in tab-complete.
- d547f7cf5efc 16.0 cited
Make subscription tests pass with log_error_verbosity=verbose
- 19408aae7fa2 15.0 cited
Weaken type-OID-matching checks in array_recv and record_recv.
- 670c0a1d474b 14.0 cited
Allow logical replication to transfer data in binary format.
- 9de77b545313 14.0 cited

On Mon, Feb 20, 2023 at 5:17 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
>
> Thanks for letting me know.
> Attached the fixed version of the patch.

Thanks. I have few comments on v9 patch:

1.
+                    /* Do not allow binary = false with copy_format = binary */
+                    if (!opts.binary &&
+                        sub->copyformat == LOGICALREP_COPY_AS_BINARY &&
+                        !IsSet(opts.specified_opts, SUBOPT_COPY_FORMAT))
+                        ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                 errmsg("cannot set %s for a
subscription with %s",
+                                        "binary = false",
"copy_format = binary")));

I don't understand why we'd need to tie an option (binary) that deals
with data types at column-level with another option (copy_format) that
requests the entire table data to be in binary. This'd essentially
make one to set binary = true to use copy_format = binary, no? IMHO,
this inter-dependency is not good for better usability.

2. Why can't the tests that this patch adds be simple? Why would it
need to change the existing tests at all? I'm thinking to create a new
00X_binary_copy_format.pl or such and setting up logical replication
with copy_format = binary and letting table sync worker request
publisher in binary format - you can verify this via publisher server
logs - look for COPY with BINARY option. If required, have the table
with different data types. This greatly reduces the patch's footprint.

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