On Thu, Jul 1, 2021 at 5:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@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.
