Horton

Re: Allow logical replication to copy tables in binary format

Melih Mutlu <m.melihmutlu@gmail.com>

View thread
From: Melih Mutlu <m.melihmutlu@gmail.com>
To: "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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-16T09:33:37Z
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 →

  1. Allow logical replication to copy tables in binary format.

  2. Common function for percent placeholder replacement

  3. Doc: add XML ID attributes to <sectN> and <varlistentry> tags.

  4. Re-order disable_on_error in tab-complete.

  5. Make subscription tests pass with log_error_verbosity=verbose

  6. Weaken type-OID-matching checks in array_recv and record_recv.

  7. Allow logical replication to transfer data in binary format.

Hi,

Thanks for reviewing. Please see the v8 here [1]

Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com>, 1 Şub 2023 Çar,
06:05 tarihinde şunu yazdı:

> (1) general comment
>
> I wondered if the addition of the new option/parameter can introduce some
> confusion to the users.
>
> case 1. When binary = true and copy_format = text, the table sync is
> conducted by text.
> case 2. When binary = false and copy_format = binary, the table sync is
> conducted by binary.
> (Note that the case 2 can be accepted after addressing the 3rd comment of
> Bharath-san in [1].
> I agree with the 3rd comment by itself.)
>

I replied to Bharath's comment [1], can you please check to see if that
makes sense?


> The name of the new subscription parameter looks confusing.
> How about "init_sync_format" or something ?
>

The option to enable initial sync is named "copy_data", so I named the new
parameter as "copy_format" to refer to that copy meant by "copy_data".
Maybe "copy_data_format" would be better. I can change it if you think it's
better.


> (2) The commit message doesn't get updated.
>

Done


> (3) whitespace errors.
>

Done


> (4) pg_dump.c
>

Done


> (5) describe.c
>

Done


> (6)
>
> + * Extract the copy format value from a DefElem.
> + */
> +char
> +defGetCopyFormat(DefElem *def)
>
> Shouldn't this function be static and remove the change of
> subscriptioncmds.h ?
>

I wanted to make "defGetCopyFormat" be consistent with
"defGetStreamingMode" since they're basically doing the same work for
different parameters. And that function isn't static, so I didn't make
"defGetCopyFormat" static too.


> (7) catalogs.sgml
>

Done

(8) create_subscription.sgml
>

Done

Also;

The latest patch doesn't get updated for more than two weeks
> after some review comments. If you don't have time,
> I would like to help updating the patch for you and other reviewers.
>
> Kindly let me know your status.
>

 Sorry for the delay. This patch is currently one of my priorities.
Hopefully I will share quicker updates from now on.

[1]
https://www.postgresql.org/message-id/CAGPVpCQYi9AYQSS%3DRmGgVNjz5ZEnLB8mACwd9aioVhLmbgiAMA%40mail.gmail.com


Thanks,
-- 
Melih Mutlu
Microsoft