On Monday, April 25, 2022 4:35 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > On Friday, April 22, 2022 12:12 PM Peter Smith <smithpb2...@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
v5-0001-Perform-streaming-logical-transactions-by-background.patch
Description: v5-0001-Perform-streaming-logical-transactions-by-background.patch
