From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Amul Sul <sulamul(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options |
Date: | 2021-06-02 06:13:06 |
Message-ID: | CAHut+PtqHiNM29GTkeOkCXcugAc8o-LwcozJD_Z_qyX1DrDj3Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> > + /* If connect option is supported, the others also need to be. */
> > + Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
> > + ((supported_opts & SUBOPT_ENABLED) != 0 &&
> > + (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
> > + (supported_opts & SUBOPT_COPY_DATA) != 0));
> > +
> > + /* Set default values for the supported options. */
> > + if ((supported_opts & SUBOPT_CONNECT) != 0)
> > + vals->connect = true;
> > +
> > + if ((supported_opts & SUBOPT_ENABLED) != 0)
> > + vals->enabled = true;
> > +
> > + if ((supported_opts & SUBOPT_CREATE_SLOT) != 0)
> > + vals->create_slot = true;
> > +
> > + if ((supported_opts & SUBOPT_SLOT_NAME) != 0)
> > + vals->slot_name = NULL;
> > +
> > + if ((supported_opts & SUBOPT_COPY_DATA) != 0)
> > + vals->copy_data = true;
> >
> > 3. Are all those "!= 0" really necessary when checking the
> > supported_opts against the bit masks? Maybe it is just a style thing,
> > but since there are so many of them I felt it contributed to clutter
> > and made the code less readable. This pattern was in many places, not
> > just the example above.
>
> Yeah these are necessary to know whether a particular option's bit is
> set in the bitmask.
Hmmm. Maybe I did not ask the question properly. See below.
> How about having a macro like below:
> #define IsSet(val, option) ((val & option) != 0)
> The if statements can become like below:
> if (IsSet(supported_opts, SUBOPT_CONNECT))
> if (IsSet(supported_opts, SUBOPT_ENABLED))
> if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
> if (IsSet(supported_opts, SUBOPT_COPY_DATA))
>
> The above looks better to me. Thoughts?
Yes, it looks better, but (since the masks are all 1 bit) I was only
asking why not do like:
if (supported_opts & SUBOPT_CONNECT)
if (supported_opts & SUBOPT_ENABLED)
if (supported_opts & SUBOPT_SLOT_NAME)
if (supported_opts & SUBOPT_COPY_DATA)
>
> Can we implement a similar idea for the parse_publication_options
> although there are only two options right now. Option parsing code
> will be consistent for logical replication DDLs and is extensible.
> Thoughts?
I have no strong opinion about it. It seems a trade off between having
a goal of "code consistency", versus "if it aint broke don't fix it".
------
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2021-06-02 06:20:30 | Re: Two patches to speed up pg_rewind. |
Previous Message | Dilip Kumar | 2021-06-02 06:07:56 | Re: Decoding speculative insert with toast leaks memory |