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

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