Re: parse_subscription_options - suggested improvements
On Wed, Dec 8, 2021 at 2:51 PM Michael Paquier 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 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
Re: parse_subscription_options - suggested improvements
On Tue, Dec 07, 2021 at 08:12:59AM +1100, Peter Smith wrote: > On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan wrote: >> Attached a v14 with the initializations added back. > > LGTM. All the code paths previously covered still are, so applied this one. Thanks! -- Michael signature.asc Description: PGP signature
Re: parse_subscription_options - suggested improvements
On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan wrote:
>
> On 12/5/21, 9:21 PM, "Michael Paquier" 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
Re: parse_subscription_options - suggested improvements
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
signature.asc
Description: PGP signature
Re: parse_subscription_options - suggested improvements
On Fri, Dec 3, 2021 at 11:02 AM Bossart, Nathan wrote:
>
> On 8/8/21, 11:54 PM, "Peter Smith" 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.
Re: parse_subscription_options - suggested improvements
v11 -> v12 * A rebase was needed due to some recent pgindent changes on HEAD. -- Kind Regards, Peter Smith. Fujitsu Australia v12-0001-Simplify-recent-parse_subscription_option-change.patch Description: Binary data
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
wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila
> wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera
> > 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 =
