Hi,
GetConfigOptionValues function extracts the config parameters for the
given variable irrespective of whether it results in noshow or not.
But the parent function show_all_settings ignores the values parameter
if it results in noshow. It's unnecessary to fetch all the values
during noshow. So a
Attaching the patch.
On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav
wrote:
>
> Hi,
>
> GetConfigOptionValues function extracts the config parameters for the
> given variable irrespective of whether it results in noshow or not.
> But the parent function show_all_settings ignores the values parameter
On Wed, Jan 18, 2023 at 1:24 PM Nitin Jadhav
wrote:
>
> Attaching the patch.
>
> On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav
> wrote:
> >
> > Hi,
> >
> > GetConfigOptionValues function extracts the config parameters for the
> > given variable irrespective of whether it results in noshow or not.
Nitin Jadhav writes:
> GetConfigOptionValues function extracts the config parameters for the
> given variable irrespective of whether it results in noshow or not.
> But the parent function show_all_settings ignores the values parameter
> if it results in noshow. It's unnecessary to fetch all the v
On Wed, Jan 18, 2023 at 9:44 PM Tom Lane wrote:
>
> Possibly a better answer is to refactor into separate functions,
> along the lines of
>
> static bool
> ConfigOptionIsShowable(struct config_generic *conf)
>
> static void
> GetConfigOptionValues(struct config_generic *conf, const char **values)
> Yes, the existing caller isn't using the fetched values when noshow is
> set to true. However, I think returning from GetConfigOptionValues()
> when noshow is set to true without fetching values limits the use of
> the function. What if someother caller wants to use the function to
> get the valu
> Possibly a better answer is to refactor into separate functions,
> along the lines of
>
> static bool
> ConfigOptionIsShowable(struct config_generic *conf)
>
> static void
> GetConfigOptionValues(struct config_generic *conf, const char **values)
Nice suggestion. Attached a patch for the same. Pl
On Thu, Jan 19, 2023 at 3:27 PM Nitin Jadhav
wrote:
>
> > Possibly a better answer is to refactor into separate functions,
> > along the lines of
> >
> > static bool
> > ConfigOptionIsShowable(struct config_generic *conf)
> >
> > static void
> > GetConfigOptionValues(struct config_generic *conf, c
> The v2 patch looks good to me except the comment around
> ConfigOptionIsShowable() which is too verbose. How about just "Return
> whether the GUC variable is visible or not."?
Sounds good. Updated in the v3 patch attached.
> I think you can add it to CF, if not done, to not lose track of it.
On Mon, Jan 23, 2023 at 3:29 PM Nitin Jadhav
wrote:
>
> > The v2 patch looks good to me except the comment around
> > ConfigOptionIsShowable() which is too verbose. How about just "Return
> > whether the GUC variable is visible or not."?
>
> Sounds good. Updated in the v3 patch attached.
>
> > I t
Bharath Rupireddy writes:
> LGTM. I've marked it RfC.
After looking at this, it seemed to me that the factorization
wasn't quite right after all: specifically, the new function
could be used in several more places if it confines itself to
being a privilege check and doesn't consider GUC_NO_SHOW_A
On Mon, Jan 23, 2023 at 9:51 PM Tom Lane wrote:
>
> Bharath Rupireddy writes:
> > LGTM. I've marked it RfC.
>
> After looking at this, it seemed to me that the factorization
> wasn't quite right after all: specifically, the new function
> could be used in several more places if it confines itself
Bharath Rupireddy writes:
> On Mon, Jan 23, 2023 at 9:51 PM Tom Lane wrote:
>> Also, I intentionally dropped the GUC_NO_SHOW_ALL check in
>> get_explain_guc_options, because it seems redundant given
>> the preceding GUC_EXPLAIN check. It's unlikely we'd ever have
>> a variable that's marked both
> After looking at this, it seemed to me that the factorization
> wasn't quite right after all: specifically, the new function
> could be used in several more places if it confines itself to
> being a privilege check and doesn't consider GUC_NO_SHOW_ALL.
> So more like the attached.
>
> You could a
>>> Also, I intentionally dropped the GUC_NO_SHOW_ALL check in
>>> get_explain_guc_options, because it seems redundant given
>>> the preceding GUC_EXPLAIN check. It's unlikely we'd ever have
>>> a variable that's marked both GUC_EXPLAIN and GUC_NO_SHOW_ALL ...
>>> but if we did, shouldn't the form
Nitin Jadhav writes:
> I agree that the developer can use both GUC_NO_SHOW_ALL and
> GUC_EXPLAIN knowingly or unknowingly for a single GUC. If used by
> mistake then according to the existing code (without patch),
> GUC_NO_SHOW_ALL takes higher precedence whether it is marked first or
> last in th
On Tue, Jan 24, 2023 at 8:43 PM Tom Lane wrote:
>
> Bharath Rupireddy writes:
> > On Mon, Jan 23, 2023 at 9:51 PM Tom Lane wrote:
> >> Also, I intentionally dropped the GUC_NO_SHOW_ALL check in
> >> get_explain_guc_options, because it seems redundant given
> >> the preceding GUC_EXPLAIN check.
> Both of you are arguing as though GUC_NO_SHOW_ALL is a security
> property. It is not, or at least it's so trivially bypassable
> that it's useless to consider it one. All it is is a de-clutter
> mechanism.
Understood. If that is the case, then I am ok with the patch.
Thanks & Regards,
Nitin
Nitin Jadhav writes:
>> Both of you are arguing as though GUC_NO_SHOW_ALL is a security
>> property. It is not, or at least it's so trivially bypassable
>> that it's useless to consider it one. All it is is a de-clutter
>> mechanism.
> Understood. If that is the case, then I am ok with the patc
19 matches
Mail list logo
