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). 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." 3) There's an whitespace introduced making the SUBOPT_SLOT_NAME, SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with the SUBOPT_CONNECT + /* 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); The other changes look good to me. > I am not sure whether the second patch is an improvement over what we have > currently but if you and others feel that is a good idea then you can > submit the same after the main patch gets committed. Peter Smith was also not happy with that patch. Anyways, I will post that patch in this thread after 0001 gets in and see if it interests other hackers. Regards, Bharath Rupireddy.
