Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

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.