Re: Allow logical replication to copy tables in binary format

Hi,

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

Takamichi Osumi (Fujitsu) <osumi(dot)takamichi(at)fujitsu(dot)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