Re: Improve GetConfigOptionValues function
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 > if it results in noshow. It's unnecessary to fetch all the values > during noshow. So a return statement in GetConfigOptionValues() when > noshow is set to true is needed. Attached the patch for the same. > Please share your thoughts. > > Thanks & Regards, > Nitin Jadhav v1-0001-Improve-GetConfigOptionValues-function.patch Description: Binary data
Re: Improve GetConfigOptionValues function
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. > > 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 return statement in GetConfigOptionValues() when > > noshow is set to true is needed. Attached the patch for the same. > > Please share your thoughts. 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 values with noshow passed in and use the values when noshow is set to true? Also, do we gain anything with the patch? I mean, can show_all_settings()/pg_settings/pg_show_all_settings() get faster, say with a non-superuser without pg_read_all_settings predefined role or with a superuser? I see there're about 6 GUC_NO_SHOW_ALL GUCs and 20 GUC_SUPERUSER_ONLY GUCs, I'm not sure if it leads to some benefit with the patch. Having said above, I don't mind keeping the things the way they're right now. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
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 values > during noshow. So a return statement in GetConfigOptionValues() when > noshow is set to true is needed. Attached the patch for the same. > Please share your thoughts. I do not think this is an improvement: it causes GetConfigOptionValues to be making assumptions about how its results will be used. If show_all_settings() were a big performance bottleneck, and there were a lot of no-show values that we could optimize, then maybe the extra coupling would be worthwhile. But I don't believe either of those things. 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) regards, tom lane
Re: Improve GetConfigOptionValues function
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) +1 and ConfigOptionIsShowable() function can replace explicit showable checks in two other places too. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
> 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 values with noshow passed in and use the values when noshow is > set to true? I agree that it limits the use of the function but I feel it is good to focus on the existing use cases and modify the functions accordingly. In future, if such a use case occurs, then the author should take care of modifying the required functions. The idea suggested by Tom to refactor the function looks good as it aligns with current use cases and it can be used in future cases as you were suggesting. > Also, do we gain anything with the patch? I mean, can > show_all_settings()/pg_settings/pg_show_all_settings() get faster, say > with a non-superuser without pg_read_all_settings predefined role or > with a superuser? I see there're about 6 GUC_NO_SHOW_ALL GUCs and 20 > GUC_SUPERUSER_ONLY GUCs, I'm not sure if it leads to some benefit with > the patch. As the number of such parameters (GUC_NO_SHOW_ALL and GUC_SUPERUSER_ONLY) are less, we may not see improvements in performance. We can treat it as a kind of refactoring. Thanks & Regards, Nitin Jadhav On Wed, Jan 18, 2023 at 1:47 PM Bharath Rupireddy wrote: > > 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. > > > 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 return statement in GetConfigOptionValues() when > > > noshow is set to true is needed. Attached the patch for the same. > > > Please share your thoughts. > > 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 values with noshow passed in and use the values when noshow is > set to true? > > Also, do we gain anything with the patch? I mean, can > show_all_settings()/pg_settings/pg_show_all_settings() get faster, say > with a non-superuser without pg_read_all_settings predefined role or > with a superuser? I see there're about 6 GUC_NO_SHOW_ALL GUCs and 20 > GUC_SUPERUSER_ONLY GUCs, I'm not sure if it leads to some benefit with > the patch. > > Having said above, I don't mind keeping the things the way they're right now. > > -- > Bharath Rupireddy > PostgreSQL Contributors Team > RDS Open Source Databases > Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
> 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. Please share the comments if any. Thanks & Regards, Nitin Jadhav On Wed, Jan 18, 2023 at 9:44 PM Tom Lane wrote: > > 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 values > > during noshow. So a return statement in GetConfigOptionValues() when > > noshow is set to true is needed. Attached the patch for the same. > > Please share your thoughts. > > I do not think this is an improvement: it causes GetConfigOptionValues > to be making assumptions about how its results will be used. If > show_all_settings() were a big performance bottleneck, and there were > a lot of no-show values that we could optimize, then maybe the extra > coupling would be worthwhile. But I don't believe either of those > things. > > 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) > > regards, tom lane v2-0001-Refactor-GetConfigOptionValues-function.patch Description: Binary data
Re: Improve GetConfigOptionValues function
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, const char **values) > > Nice suggestion. Attached a patch for the same. Please share the > comments if any. 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."? I think you can add it to CF, if not done, to not lose track of it. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
> 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. Added https://commitfest.postgresql.org/42/4140/ Thanks & Regards, Nitin Jadhav On Mon, Jan 23, 2023 at 11:30 AM Bharath Rupireddy wrote: > > 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, const char **values) > > > > Nice suggestion. Attached a patch for the same. Please share the > > comments if any. > > 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."? > > I think you can add it to CF, if not done, to not lose track of it. > > -- > Bharath Rupireddy > PostgreSQL Contributors Team > RDS Open Source Databases > Amazon Web Services: https://aws.amazon.com v3-0001-Refactor-GetConfigOptionValues-function.patch Description: Binary data
Re: Improve GetConfigOptionValues function
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 think you can add it to CF, if not done, to not lose track of it. > > Added https://commitfest.postgresql.org/42/4140/ LGTM. I've marked it RfC. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
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_ALL. So more like the attached. You could argue that the factorization is illusory since each of these additional call sites has an error message that knows exactly what the conditions are to succeed. But if we want to go that direction then I'd be inclined to forget about the permissions-check function altogether and just test the conditions in-line everywhere. 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 former take precedence here anyway? regards, tom lane diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index d52069f446..978b385568 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4187,8 +4187,7 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged) if (record == NULL) return NULL; if (restrict_privileged && - (record->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + !ConfigOptionIsVisible(record)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"", @@ -4234,8 +4233,7 @@ GetConfigOptionResetString(const char *name) record = find_option(name, false, false, ERROR); Assert(record != NULL); - if ((record->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + if (!ConfigOptionIsVisible(record)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"", @@ -5160,9 +5158,7 @@ get_explain_guc_options(int *num) continue; /* return only options visible to the current user */ - if ((conf->flags & GUC_NO_SHOW_ALL) || - ((conf->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) + if (!ConfigOptionIsVisible(conf)) continue; /* return only options that are different from their boot values */ @@ -5243,8 +5239,7 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok) return NULL; } - if ((record->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + if (!ConfigOptionIsVisible(record)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"", diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c index d59706231b..52c361e975 100644 --- a/src/backend/utils/misc/guc_funcs.c +++ b/src/backend/utils/misc/guc_funcs.c @@ -492,9 +492,12 @@ ShowAllGUCConfig(DestReceiver *dest) struct config_generic *conf = guc_vars[i]; char *setting; - if ((conf->flags & GUC_NO_SHOW_ALL) || - ((conf->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) + /* skip if marked NO_SHOW_ALL */ + if (conf->flags & GUC_NO_SHOW_ALL) + continue; + + /* return only options visible to the current user */ + if (!ConfigOptionIsVisible(conf)) continue; /* assign to the values array */ @@ -581,25 +584,27 @@ pg_settings_get_flags(PG_FUNCTION_ARGS) PG_RETURN_ARRAYTYPE_P(a); } +/* + * Return whether or not the GUC variable is visible to the current user. + */ +bool +ConfigOptionIsVisible(struct config_generic *conf) +{ + if ((conf->flags & GUC_SUPERUSER_ONLY) && + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + return false; + else + return true; +} + /* * Extract fields to show in pg_settings for given variable. */ static void -GetConfigOptionValues(struct config_generic *conf, const char **values, - bool *noshow) +GetConfigOptionValues(struct config_generic *conf, const char **values) { char buffer[256]; - if (noshow) - { - if ((conf->flags & GUC_NO_SHOW_ALL) || - ((conf->flags & GUC_SUPERUSER_ONLY) && - !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) - *noshow = true; - else - *noshow = false; - } - /* first get the generic attributes */ /* name */ @@ -940,30 +945,23 @@ show_all_settings(PG_FUNCTION_ARGS) max_calls = funcctx->max_calls; attinmeta = funcctx->attinmeta; - if (call_cntr < max_calls) /* do when there is more left to send */ + while (call_cntr < max_calls) /* do when there is more left to send */ { + struct config_generic *conf = guc_vars[call_cntr]; char *values[NUM_PG_SETTINGS_ATTS]; - bool
Re: Improve GetConfigOptionValues function
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 to > being a privilege check and doesn't consider GUC_NO_SHOW_ALL. > So more like the attached. Thanks. It looks even cleaner now. > 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 former take precedence here anyway? You're right, but there's nothing that prevents users writing GUCs with GUC_EXPLAIN and GUC_NO_SHOW_ALL. FWIW, I prefer retaining the behaviour as-is i.e. we can have explicit if (conf->flags & GUC_NO_SHOW_ALL) continue; there in get_explain_guc_options(). -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
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 GUC_EXPLAIN and GUC_NO_SHOW_ALL ... >> but if we did, shouldn't the former take precedence here anyway? > You're right, but there's nothing that prevents users writing GUCs > with GUC_EXPLAIN and GUC_NO_SHOW_ALL. "Users"? You do realize those flags are only settable by C code, right? Moreover, you haven't explained why it would be good that you can't get at the behavior that a GUC is both shown in EXPLAIN and not shown in SHOW ALL. If you want "not shown by either", that's already accessible by setting only the GUC_NO_SHOW_ALL flag. So I'd almost argue this is a bug fix, though I concede it's a bit hard to imagine why somebody would want that choice. Still, if we have two independent flags they should produce four behaviors, not just three. regards, tom lane
Re: Improve GetConfigOptionValues function
> 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 argue that the factorization is illusory since each > of these additional call sites has an error message that knows > exactly what the conditions are to succeed. But if we want to > go that direction then I'd be inclined to forget about the > permissions-check function altogether and just test the > conditions in-line everywhere. I am ok with the above changes. I thought of modifying the ConfigOptionIsVisible function to take an extra argument, say validate_superuser_only. If this argument is true then it only considers GUC_SUPERUSER_ONLY check and return based on that. Otherwise it considers both GUC_SUPERUSER_ONLY and GUC_NO_SHOW_ALL and returns based on that. I understand that this just complicates the function and has other disadvantages. Instead of testing the conditions in-line, I prefer the use of function as done in v4 patch as it reduces the code size. Thanks & Regards, Nitin Jadhav 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 to > being a privilege check and doesn't consider GUC_NO_SHOW_ALL. > So more like the attached. > > You could argue that the factorization is illusory since each > of these additional call sites has an error message that knows > exactly what the conditions are to succeed. But if we want to > go that direction then I'd be inclined to forget about the > permissions-check function altogether and just test the > conditions in-line everywhere. > > 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 former take precedence here anyway? > > regards, tom lane >
Re: Improve GetConfigOptionValues function
>>> 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 former take precedence here anyway? > >> You're right, but there's nothing that prevents users writing GUCs >> with GUC_EXPLAIN and GUC_NO_SHOW_ALL. > > "Users"? You do realize those flags are only settable by C code, > right? Moreover, you haven't explained why it would be good that > you can't get at the behavior that a GUC is both shown in EXPLAIN > and not shown in SHOW ALL. If you want "not shown by either", > that's already accessible by setting only the GUC_NO_SHOW_ALL > flag. So I'd almost argue this is a bug fix, though I concede > it's a bit hard to imagine why somebody would want that choice. > Still, if we have two independent flags they should produce four > behaviors, not just three. 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 the code. I am more convinced with this behaviour as I feel it is safer than exposing the information which the developer might not have intended. Thanks & Regards, Nitin Jadhav 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. 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 former take precedence here anyway? > > > You're right, but there's nothing that prevents users writing GUCs > > with GUC_EXPLAIN and GUC_NO_SHOW_ALL. > > "Users"? You do realize those flags are only settable by C code, > right? Moreover, you haven't explained why it would be good that > you can't get at the behavior that a GUC is both shown in EXPLAIN > and not shown in SHOW ALL. If you want "not shown by either", > that's already accessible by setting only the GUC_NO_SHOW_ALL > flag. So I'd almost argue this is a bug fix, though I concede > it's a bit hard to imagine why somebody would want that choice. > Still, if we have two independent flags they should produce four > behaviors, not just three. > > regards, tom lane
Re: Improve GetConfigOptionValues function
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 the code. I am more convinced with this behaviour as I feel it > is safer than exposing the information which the developer might not > have intended. 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. regards, tom lane
Re: Improve GetConfigOptionValues function
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. 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 former take precedence here anyway? > > > You're right, but there's nothing that prevents users writing GUCs > > with GUC_EXPLAIN and GUC_NO_SHOW_ALL. > > "Users"? You do realize those flags are only settable by C code, > right? I meant extensions here. > Moreover, you haven't explained why it would be good that > you can't get at the behavior that a GUC is both shown in EXPLAIN > and not shown in SHOW ALL. If you want "not shown by either", > that's already accessible by setting only the GUC_NO_SHOW_ALL > flag. So I'd almost argue this is a bug fix, though I concede > it's a bit hard to imagine why somebody would want that choice. > Still, if we have two independent flags they should produce four > behaviors, not just three. Got it. I think I should've looked at GUC_NO_SHOW_ALL and GUC_EXPLAIN as separate flags not depending on each other in any way, meaning, GUCs marked with GUC_NO_SHOW_ALL mustn't be shown in SHOW ALL and its friends pg_settings and pg_show_all_settings(), and GUCs marked with GUC_EXPLAIN must be shown in explain output. This understanding is clear and simple IMO. Having said that, I have no objection to the v4 patch posted upthread. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve GetConfigOptionValues function
> 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 Jadhav On Wed, Jan 25, 2023 at 9:23 PM Tom Lane wrote: > > 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 the code. I am more convinced with this behaviour as I feel it > > is safer than exposing the information which the developer might not > > have intended. > > 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. > > regards, tom lane
Re: Improve GetConfigOptionValues function
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 patch. Pushed v4, then. regards, tom lane