On Monday, April 25, 2022 4:35 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> On Friday, April 22, 2022 12:12 PM Peter Smith <smithpb2250@gmail.com>
> wrote:
> >
> > Hello Hou-san. Here are my review comments for v4-0001. Sorry, there
> > are so many of them (it is a big patch); some are trivial, and others
> > you might easily dismiss due to my misunderstanding of the code. But
> > hopefully, there are at least some comments that can be helpful in
> > improving the patch quality.
>
> Thanks for the comments !
> I think most of the comments make sense and here are explanations for some
> of them.
Hi,
I addressed the rest of Peter's comments and here is a new version patch.
The naming of the newly introduced option and worker might
need more thought, so I haven't change all of them. I will think over
and change it later.
One comment I didn't address:
> 3. General comment - bool option change to enum
>
> This option change for "streaming" is similar to the options change
> for "copy_data=force" that Vignesh is doing for his "infinite
> recursion" patch v9-0002 [1]. Yet they seem implemented differently
> (i.e. char versus enum). I think you should discuss the 2 approaches
> with Vignesh and then code these option changes in a consistent way.
>
> [1] https://www.postgresql.org/message-id/CALDaNm2Fe%3Dg4Tx-DhzwD6NU0VRAfaPedXwWO01maNU7_OfS8fw%40mail.gmail.> com
I think the "streaming" option is a bit different from the "copy_data" option.
Because the "streaming" is a column of the system table (pg_subscription) which
should use "char" type to represent different values in this case(For example:
pg_class.relkind/pg_class.relpersistence/pg_class.relreplident ...).
And the "copy_data" option is not a system table column and I think it's fine
to use Enum for it.
Best regards,
Hou zj
