Refactor "mutually exclusive options" error reporting code in parse_subscription_options

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Refactor "mutually exclusive options" error reporting code in parse_subscription_options

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

amul sul
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

akapila
On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Wed, May 19, 2021 at 2:33 PM Amul Sul <[hidden email]> wrote:
> >
> > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > <[hidden email]> wrote:
> > >
> > > Hi,
> > >
> > > parse_subscription_options function has some similar code when
> > > throwing errors [with the only difference in the option]. I feel we
> > > could just use a variable for the option and use it in the error.

I am not sure how much it helps to just refactor this part of the code
alone unless we need to add/change it more. Having said that, this
function is being modified by one of the proposed patches for logical
decoding of 2PC and I noticed that the proposed patch is adding more
parameters to this function which already takes 14 input parameters,
so I suggested refactoring it. See comment 11 in email[1]. See, if
that makes sense to you then we can refactor this function such that
it can be enhanced easily by future patches.

> > > While this has no benefit at all, it saves some LOC and makes the code
> > > look better with lesser ereport(ERROR statements. PSA patch.
> > >
> > > Thoughts?
> >
> > I don't have a strong opinion on this, but the patch should add
> > __translator__ help comment for the error msg.
>
> Is the "/*- translator:" help comment something visible to the user or
> some other tool?
>

We use similar comments at other places. So, it makes sense to retain
the comment as it might help translation tools.

[1] - https://www.postgresql.org/message-id/CAA4eK1Jz64rwLyB6H7Z_SmEDouJ41KN42%3DVkVFp6JTpafJFG8Q%40mail.gmail.com

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

akapila
On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Wed, May 19, 2021 at 4:10 PM Amit Kapila <[hidden email]> wrote:
> >
> > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > <[hidden email]> wrote:
> > >
> > > On Wed, May 19, 2021 at 2:33 PM Amul Sul <[hidden email]> wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > > <[hidden email]> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > parse_subscription_options function has some similar code when
> > > > > throwing errors [with the only difference in the option]. I feel we
> > > > > could just use a variable for the option and use it in the error.
> >
> > I am not sure how much it helps to just refactor this part of the code
> > alone unless we need to add/change it more. Having said that, this
> > function is being modified by one of the proposed patches for logical
> > decoding of 2PC and I noticed that the proposed patch is adding more
> > parameters to this function which already takes 14 input parameters,
> > so I suggested refactoring it. See comment 11 in email[1]. See, if
> > that makes sense to you then we can refactor this function such that
> > it can be enhanced easily by future patches.
>
> Thanks Amit for the comments. I agree to move the parse options to a
> new structure ParseSubOptions as suggested. Then the function can just
> be parse_subscription_options(ParseSubOptions opts); I wonder if we
> should also have a structure for parse_publication_options as we might
> add new options there in the future?
>

That function has just 5 parameters so not sure if that needs the same
treatment. Let's leave it for now.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Peter Smith
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Peter Smith
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Álvaro Herrera
On 2021-May-24, Bharath Rupireddy wrote:

> On Mon, May 24, 2021 at 7:04 AM Michael Paquier <[hidden email]> wrote:
> > What you are writing here and your comment two paragraphs above are
> > inconsistent as you are using an enum here.  Please see a3dc926 and
> > the surrounding discussion for reasons why we've been using bitmaps
> > for option parsing lately.
>
> Thanks! I'm okay to do something similar to what the commit a3dc926
> did using bits32. But I wonder if we will ever cross the 32 options
> limit (imposed by bits32) for CREATE/ALTER SUBSCRIPTION command.
> Having said that, for now, we can have an error similar to
> last_assigned_kind in add_reloption_kind() if the limit is crossed.

There's no API limitation here, since that stuff is not user-visible, so
it doesn't matter.  If we ever need a 33rd option, we can change the
datatype to bits64.  Any extensions using it will have to be recompiled
across a major version jump anyway.

--
Álvaro Herrera                            39°49'30"S 73°17'W


Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Álvaro Herrera
In reply to this post by Bharath Rupireddy
On 2021-May-25, Bharath Rupireddy wrote:

> On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera <[hidden email]> wrote:

> > There's no API limitation here, since that stuff is not user-visible, so
> > it doesn't matter.  If we ever need a 33rd option, we can change the
> > datatype to bits64.  Any extensions using it will have to be recompiled
> > across a major version jump anyway.
>
> Thanks. I think there's no bits64 data type currently, I'm sure you
> meant we will define (when requirement arises) something like typedef
> uint64 bits64; Am I correct?

Right.

> I see that the commit a3dc926 and discussion at [1] say below respectively:
> "All the options of those commands are changed to use hex values
> rather than enums to reduce the risk of compatibility bugs when
> introducing new options."
> "My reasoning is that if you look at an enum value of this type,
> either say in a switch statement or a debugger, the enum value might
> not be any of the defined symbols. So that way you lose all the type
> checking that an enum might give you."
>
> I'm not able to grasp what are the incompatibilities we can have if
> the enums are used as bit masks. It will be great if anyone throws
> some light on this?

The problem is that enum members have consecutive integers assigned by
the compiler.  Say you have an enum with three values for options.  They
get assigned 0, 1, and 2.  You can test for each option with "opt &
VAL_ONE" and "opt & VAL_TWO" and everything works -- each test returns
true when that specific option is set, and all is well.  Now if somebody
later adds a fourth option, it gets value 3.  When that option is set,
"opt & VAL_ONE" magically returns true, even though you did not set that
bit in your code.  So that becomes a bug.

Using hex values or bitshifting (rather than letting the compiler decide
its value in the enum) is a more robust way to ensure that the options
will not collide in that way.

So why not define the enum as a list, and give each option an exclusive
bit by bitshifting? For example,

enum options {
  OPT_ZERO  = 0,
  OPT_ONE   = 1 << 1,
  OPT_TWO   = 1 << 2,
  OPT_THREE = 1 << 3,
};

This should be okay, right?  Well, almost. The problem here is if you
want to have a variable where you set more than one option, you have to
use bit-and of the enum values ... and the resulting value is no longer
part of the enum.  A compiler would be understandably upset if you try
to pass that value in a variable of the enum datatype.

--
Álvaro Herrera       Valdivia, Chile
"Ed is the standard text editor."
      http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3


Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

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

Bharath Rupireddy
In reply to this post by Michael Paquier-2
CONTENTS DELETED
The author has deleted this message.
12