Thread: parse_subscription_options - suggested improvements
This is a re-posting of my original mail from here [1] Created this new discussion thread for it as suggested here [2] [1] https://www.postgresql.org/message-id/CAHut%2BPtT0--Tf%3DK_YOmoyB3RtakUOYKeCs76aaOqO2Y%2BJt38kQ%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAA4eK1L0GT5-RJrya4q9ve%3DVi8hQM_SeHhJekmWMnOqsCh3KbQ%40mail.gmail.com =========================================== On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote: > > On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote: > > > > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote: > > > > > > > The latest patch sent by Bharath looks good to me. Would you like to > > > > commit it or shall I take care of it? > > > > > > Please, go ahead. > > > > > > > Okay, I'll push it early next week (by Tuesday) unless there are more > > comments or suggestions. Thanks! > > > > Pushed! Yesterday, I needed to refactor a lot of code due to this push [1]. The refactoring exercise caused me to study these v11 changes much more deeply. IMO there are a few improvements that should be made: ////// 1. Zap 'opts' up-front + * + * Caller is expected to have cleared 'opts'. This comment is putting the onus on the caller to "do the right thing". I think that hopeful expectations about input should be removed - the function should just be responsible itself just to zap the SubOpts up-front. It makes the code more readable, and it removes any potential risk of garbage unintentionally passed in that struct. /* Start out with cleared opts. */ memset(opts, 0, sizeof(SubOpts)); Alternatively, at least there should be an assertion for some sanity check. Assert(opt->specified_opts == 0); ---- 2. Remove redundant conditions /* Check for incompatible options from the user. */ - if (enabled && *enabled_given && *enabled) + if (opts->enabled && + IsSet(supported_opts, SUBOPT_ENABLED) && + IsSet(opts->specified_opts, SUBOPT_ENABLED)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "connect = false", "enabled = true"))); - if (create_slot && create_slot_given && *create_slot) + if (opts->create_slot && + IsSet(supported_opts, SUBOPT_CREATE_SLOT) && + IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually exclusive options", "connect = false", "create_slot = true"))); - if (copy_data && copy_data_given && *copy_data) + if (opts->copy_data && + IsSet(supported_opts, SUBOPT_COPY_DATA) && + IsSet(opts->specified_opts, SUBOPT_COPY_DATA)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually exclusive options", "connect = false", "copy_data = true"))); By definition, this function only allows any option to be "specified_opts" if that option is also "supported_opts". So, there is really no need in the above code to re-check again that it is supported. It can be simplified like this: /* Check for incompatible options from the user. */ - if (enabled && *enabled_given && *enabled) + if (opts->enabled && + IsSet(opts->specified_opts, SUBOPT_ENABLED)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "connect = false", "enabled = true"))); - if (create_slot && create_slot_given && *create_slot) + if (opts->create_slot && + IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually exclusive options", "connect = false", "create_slot = true"))); - if (copy_data && copy_data_given && *copy_data) + if (opts->copy_data && + IsSet(opts->specified_opts, SUBOPT_COPY_DATA)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually exclusive options", "connect = false", "copy_data = true"))); ----- 3. Remove redundant conditions Same as 2. Here are more examples of conditions where the redundant checking of "supported_opts" can be removed. /* * Do additional checking for disallowed combination when slot_name = NONE * was used. */ - if (slot_name && *slot_name_given && !*slot_name) + if (!opts->slot_name && + IsSet(supported_opts, SUBOPT_SLOT_NAME) && + IsSet(opts->specified_opts, SUBOPT_SLOT_NAME)) { - if (enabled && *enabled_given && *enabled) + if (opts->enabled && + IsSet(supported_opts, SUBOPT_ENABLED) && + IsSet(opts->specified_opts, SUBOPT_ENABLED)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "slot_name = NONE", "enabled = true"))); - if (create_slot && create_slot_given && *create_slot) + if (opts->create_slot && + IsSet(supported_opts, SUBOPT_CREATE_SLOT) && + IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), + /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "slot_name = NONE", "create_slot = true"))); It can be simplified like this: /* * Do additional checking for disallowed combination when slot_name = NONE * was used. */ - if (slot_name && *slot_name_given && !*slot_name) + if (!opts->slot_name && + IsSet(opts->specified_opts, SUBOPT_SLOT_NAME)) { - if (enabled && *enabled_given && *enabled) + if (opts->enabled && + IsSet(opts->specified_opts, SUBOPT_ENABLED)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "slot_name = NONE", "enabled = true"))); - if (create_slot && create_slot_given && *create_slot) + if (opts->create_slot && + IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), + /*- translator: both %s are strings of the form "option = value" */ errmsg("%s and %s are mutually exclusive options", "slot_name = NONE", "create_slot = true"))); ------ 4. Remove redundant conditions - if (enabled && !*enabled_given && *enabled) + if (opts->enabled && + IsSet(supported_opts, SUBOPT_ENABLED) && + !IsSet(opts->specified_opts, SUBOPT_ENABLED)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("subscription with %s must also set %s", "slot_name = NONE", "enabled = false"))); - if (create_slot && !create_slot_given && *create_slot) + if (opts->create_slot && + IsSet(supported_opts, SUBOPT_CREATE_SLOT) && + !IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), + /*- translator: both %s are strings of the form "option = value" */ errmsg("subscription with %s must also set %s", "slot_name = NONE", "create_slot = false"))); This code can be simplified even more than the others mentioned, because here the "specified_opts" checks were already done in the code that precedes this. It can be simplified like this: - if (enabled && !*enabled_given && *enabled) + if (opts->enabled) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), /*- translator: both %s are strings of the form "option = value" */ errmsg("subscription with %s must also set %s", "slot_name = NONE", "enabled = false"))); - if (create_slot && !create_slot_given && *create_slot) + if (opts->create_slot) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), + /*- translator: both %s are strings of the form "option = value" */ errmsg("subscription with %s must also set %s", "slot_name = NONE", "create_slot = false"))); ////// PSA my patch which includes all the fixes mentioned above. (Make check, and TAP subscription tests are tested and pass OK) ------ [1] https://github.com/postgres/postgres/commit/8aafb02616753f5c6c90bbc567636b73c0cbb9d4 Kind Regards, Peter Smith. Fujitsu Australia
Attachment
v11 -> v12 * A rebase was needed due to some recent pgindent changes on HEAD. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On 8/8/21, 11:54 PM, "Peter Smith" <smithpb2250@gmail.com> wrote:
> v11 -> v12
>
> * A rebase was needed due to some recent pgindent changes on HEAD.
The patch looks correct to me. I have a couple of small comments.
+ /* Start out with cleared opts. */
+ memset(opts, 0, sizeof(SubOpts));
Should we stop initializing opts in the callers?
- if (opts->enabled &&
- IsSet(supported_opts, SUBOPT_ENABLED) &&
- !IsSet(opts->specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "enabled = false")));
IMO the way these 'if' statements are written isn't super readable.
Right now, it's written like this:
if (opt && IsSet(someopt))
ereport(ERROR, ...);
if (otheropt && IsSet(someotheropt))
ereport(ERROR, ...);
if (opt)
ereport(ERROR, ...);
if (otheropt)
ereport(ERROR, ...);
I think it would be easier to understand if it was written more like
this:
if (opt)
{
if (IsSet(someopt))
ereport(ERROR, ...);
else
ereport(ERROR, ...);
}
if (otheropt)
{
if (IsSet(someotheropt))
ereport(ERROR, ...);
else
ereport(ERROR, ...);
}
Of course, this does result in a small behavior change because the
order of the checks is different, but I'm not sure that's a big deal.
Ultimately, it would probably be nice to report all the errors instead
of just the first one that is hit, but again, I don't know if that's
worth the effort.
I attached a new version of the patch with my suggestions. However, I
think v12 is committable.
Nathan
Attachment
On Fri, Dec 3, 2021 at 11:02 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 8/8/21, 11:54 PM, "Peter Smith" <smithpb2250@gmail.com> wrote:
> > v11 -> v12
> >
> > * A rebase was needed due to some recent pgindent changes on HEAD.
>
> The patch looks correct to me. I have a couple of small comments.
Thank you for taking an interest in my patch and moving it to a
"Ready" state in the CF.
>
> + /* Start out with cleared opts. */
> + memset(opts, 0, sizeof(SubOpts));
>
> Should we stop initializing opts in the callers?
For the initialization of opts I put memset within the function to
make it explicit that the bit-masks will work as intended without
having to look back at calling code for the initial values. In any
case, I think the caller declarations of SubOpts are trivial, (e.g.
SubOpts opts = {0};) so I felt caller initializations don't need to be
changed regardless of the memset.
>
> - if (opts->enabled &&
> - IsSet(supported_opts, SUBOPT_ENABLED) &&
> - !IsSet(opts->specified_opts, SUBOPT_ENABLED))
> + if (opts->enabled)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> /*- translator: both %s are strings of the form "option = value" */
> errmsg("subscription with %s must also set %s",
> "slot_name = NONE", "enabled = false")));
>
> IMO the way these 'if' statements are written isn't super readable.
> Right now, it's written like this:
>
> if (opt && IsSet(someopt))
> ereport(ERROR, ...);
>
> if (otheropt && IsSet(someotheropt))
> ereport(ERROR, ...);
>
> if (opt)
> ereport(ERROR, ...);
>
> if (otheropt)
> ereport(ERROR, ...);
>
> I think it would be easier to understand if it was written more like
> this:
>
> if (opt)
> {
> if (IsSet(someopt))
> ereport(ERROR, ...);
> else
> ereport(ERROR, ...);
> }
>
> if (otheropt)
> {
> if (IsSet(someotheropt))
> ereport(ERROR, ...);
> else
> ereport(ERROR, ...);
> }
>
> Of course, this does result in a small behaviour change because the
> order of the checks is different, but I'm not sure that's a big deal.
> Ultimately, it would probably be nice to report all the errors instead
> of just the first one that is hit, but again, I don't know if that's
> worth the effort.
>
My patch was meant only to remove all the redundant conditions of the
HEAD code, so I did not rearrange any of the logic at all. Personally,
I also think your v13 is better and easier to read, but those subtle
behaviour differences were something I'd deliberately avoided in v12.
However, if the committer thinks it does not matter then your v13 is
fine by me.
> I attached a new version of the patch with my suggestions. However, I
> think v12 is committable.
>
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Mon, Dec 06, 2021 at 11:28:12AM +1100, Peter Smith wrote:
> For the initialization of opts I put memset within the function to
> make it explicit that the bit-masks will work as intended without
> having to look back at calling code for the initial values. In any
> case, I think the caller declarations of SubOpts are trivial, (e.g.
> SubOpts opts = {0};) so I felt caller initializations don't need to be
> changed regardless of the memset.
It seems to me that not initializing these may cause some compilation
warnings. memset(0) at the beginning of parse_subscription_options()
is an improvement.
> My patch was meant only to remove all the redundant conditions of the
> HEAD code, so I did not rearrange any of the logic at all. Personally,
> I also think your v13 is better and easier to read, but those subtle
> behaviour differences were something I'd deliberately avoided in v12.
> However, if the committer thinks it does not matter then your v13 is
> fine by me.
Well, there is always the argument that it could be confusing as a
different combination of options generates a slightly-different error,
but the user would get warned about each one of his/her mistakes at
the end, so the result is the same.
- if (opts->enabled &&
- IsSet(supported_opts, SUBOPT_ENABLED) &&
- !IsSet(opts->specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled)
I see. The last condition on the specified options in the last two
checks is removed thanks to the first two checks. As a matter of
consistency with those error strings, keeping each !IsSet() would be
cleaner. But I agree that v13 is better than that, without removing
the two initializations.
--
Michael
Attachment
On 12/5/21, 9:21 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Mon, Dec 06, 2021 at 11:28:12AM +1100, Peter Smith wrote:
>> For the initialization of opts I put memset within the function to
>> make it explicit that the bit-masks will work as intended without
>> having to look back at calling code for the initial values. In any
>> case, I think the caller declarations of SubOpts are trivial, (e.g.
>> SubOpts opts = {0};) so I felt caller initializations don't need to be
>> changed regardless of the memset.
>
> It seems to me that not initializing these may cause some compilation
> warnings. memset(0) at the beginning of parse_subscription_options()
> is an improvement.
I'll admit I was surprised that my compiler didn't complain about
this, but I wouldn't be surprised at all if others did. I agree that
there is no strong need to remove the initializations from the calling
functions.
>> My patch was meant only to remove all the redundant conditions of the
>> HEAD code, so I did not rearrange any of the logic at all. Personally,
>> I also think your v13 is better and easier to read, but those subtle
>> behaviour differences were something I'd deliberately avoided in v12.
>> However, if the committer thinks it does not matter then your v13 is
>> fine by me.
>
> Well, there is always the argument that it could be confusing as a
> different combination of options generates a slightly-different error,
> but the user would get warned about each one of his/her mistakes at
> the end, so the result is the same.
>
> - if (opts->enabled &&
> - IsSet(supported_opts, SUBOPT_ENABLED) &&
> - !IsSet(opts->specified_opts, SUBOPT_ENABLED))
> + if (opts->enabled)
>
> I see. The last condition on the specified options in the last two
> checks is removed thanks to the first two checks. As a matter of
> consistency with those error strings, keeping each !IsSet() would be
> cleaner. But I agree that v13 is better than that, without removing
> the two initializations.
Attached a v14 with the initializations added back.
Nathan
Attachment
On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 12/5/21, 9:21 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> > On Mon, Dec 06, 2021 at 11:28:12AM +1100, Peter Smith wrote:
> >> For the initialization of opts I put memset within the function to
> >> make it explicit that the bit-masks will work as intended without
> >> having to look back at calling code for the initial values. In any
> >> case, I think the caller declarations of SubOpts are trivial, (e.g.
> >> SubOpts opts = {0};) so I felt caller initializations don't need to be
> >> changed regardless of the memset.
> >
> > It seems to me that not initializing these may cause some compilation
> > warnings. memset(0) at the beginning of parse_subscription_options()
> > is an improvement.
>
> I'll admit I was surprised that my compiler didn't complain about
> this, but I wouldn't be surprised at all if others did. I agree that
> there is no strong need to remove the initializations from the calling
> functions.
>
> >> My patch was meant only to remove all the redundant conditions of the
> >> HEAD code, so I did not rearrange any of the logic at all. Personally,
> >> I also think your v13 is better and easier to read, but those subtle
> >> behaviour differences were something I'd deliberately avoided in v12.
> >> However, if the committer thinks it does not matter then your v13 is
> >> fine by me.
> >
> > Well, there is always the argument that it could be confusing as a
> > different combination of options generates a slightly-different error,
> > but the user would get warned about each one of his/her mistakes at
> > the end, so the result is the same.
> >
> > - if (opts->enabled &&
> > - IsSet(supported_opts, SUBOPT_ENABLED) &&
> > - !IsSet(opts->specified_opts, SUBOPT_ENABLED))
> > + if (opts->enabled)
> >
> > I see. The last condition on the specified options in the last two
> > checks is removed thanks to the first two checks. As a matter of
> > consistency with those error strings, keeping each !IsSet() would be
> > cleaner. But I agree that v13 is better than that, without removing
> > the two initializations.
>
> Attached a v14 with the initializations added back.
>
LGTM.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 07, 2021 at 08:12:59AM +1100, Peter Smith wrote: > On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan <bossartn@amazon.com> wrote: >> Attached a v14 with the initializations added back. > > LGTM. All the code paths previously covered still are, so applied this one. Thanks! -- Michael
Attachment
On Wed, Dec 8, 2021 at 2:51 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Dec 07, 2021 at 08:12:59AM +1100, Peter Smith wrote: > > On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan <bossartn@amazon.com> wrote: > >> Attached a v14 with the initializations added back. > > > > LGTM. > > All the code paths previously covered still are, so applied this one. > Thanks! Thanks for pushing! ------ Kind Regards, Peter Smith. Fujitsu Australia