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
v11-0001-PS-Fix-v11.patch
Description: Binary data
