On Thu, Jul 1, 2021 at 5:37 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > > > > PFA v9 patch set for further review. > > > > > > > The first patch looks mostly good to me. I have made some minor > > modifications to the 0001 patch: (a) added/edited few comments, (b) > > there is no need to initialize supported_opts variable in > > CreateSubscription, (c) used extra bracket in macro, (d) ran pgindent. > > Thanks a lot Amit. > > > Kindly check and let me know what you think of the attachment? > 1) Isn't good to mention in the commit message a note about the > limitation of the maximum number of SUBOPT_*? Currently it is 32 > because of bits32 data type. If required, then we might have to > introduce bits64 (typedef to uint64). >
I am not sure if it is required to mention it as this is not an exposed struct and I think we can't reach that number in near future. > 2) How about just saying "Refactor function > parse_subscription_options." instead of "Refactor function > parse_subscription_options()." in the commit message? This is similar > to the commit 531737d "Refactor function parse_output_parameters." > It hardly matters. We can write either way. I normally use () after function name. > 3) There's an whitespace introduced making the SUBOPT_SLOT_NAME, > SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with > the SUBOPT_CONNECT > okay, will fix it. > + /* Options that can be specified by CREATE SUBSCRIPTION command. */ > + supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT | > + SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA | > + SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY | > + SUBOPT_STREAMING); > Shouldn't it be something like below? > + supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT | > + SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA | > + SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY | > + SUBOPT_STREAMING); > Both appear the same to me. Can you please highlight the difference in some way? -- With Regards, Amit Kapila.
