Обсуждение: Improve GetConfigOptionValues function
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
Attaching the patch. On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
Вложения
On Wed, Jan 18, 2023 at 1:24 PM Nitin Jadhav <nitinjadhavpostgres@gmail.com> wrote: > > Attaching the patch. > > On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav > <nitinjadhavpostgres@gmail.com> 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
Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
On Wed, Jan 18, 2023 at 9:44 PM Tom Lane <tgl@sss.pgh.pa.us> 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
> 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 <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Wed, Jan 18, 2023 at 1:24 PM Nitin Jadhav > <nitinjadhavpostgres@gmail.com> wrote: > > > > Attaching the patch. > > > > On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav > > <nitinjadhavpostgres@gmail.com> 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
> 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 <tgl@sss.pgh.pa.us> wrote: > > Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
Вложения
On Thu, Jan 19, 2023 at 3:27 PM Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
> 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 <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Jan 19, 2023 at 3:27 PM Nitin Jadhav > <nitinjadhavpostgres@gmail.com> 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
Вложения
On Mon, Jan 23, 2023 at 3:29 PM Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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 noshow;
HeapTuple tuple;
Datum result;
- /*
- * Get the next visible GUC variable name and value
- */
- do
+ /* skip if marked NO_SHOW_ALL or if not visible to current user */
+ if ((conf->flags & GUC_NO_SHOW_ALL) ||
+ !ConfigOptionIsVisible(conf))
{
- GetConfigOptionValues(guc_vars[call_cntr], (const char **) values,
- &noshow);
- if (noshow)
- {
- /* bump the counter and get the next config setting */
- call_cntr = ++funcctx->call_cntr;
+ call_cntr = ++funcctx->call_cntr;
+ continue;
+ }
- /* make sure we haven't gone too far now */
- if (call_cntr >= max_calls)
- SRF_RETURN_DONE(funcctx);
- }
- } while (noshow);
+ /* extract values for the current variable */
+ GetConfigOptionValues(conf, (const char **) values);
/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
@@ -973,11 +971,9 @@ show_all_settings(PG_FUNCTION_ARGS)
SRF_RETURN_NEXT(funcctx, result);
}
- else
- {
- /* do when there is no more left */
- SRF_RETURN_DONE(funcctx);
- }
+
+ /* do when there is no more left */
+ SRF_RETURN_DONE(funcctx);
}
/*
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1195abaa3d..d5a0880678 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -292,6 +292,9 @@ extern struct config_generic **get_explain_guc_options(int *num);
/* get string value of variable */
extern char *ShowGUCOption(struct config_generic *record, bool use_units);
+/* get whether or not the GUC variable is visible to current user */
+extern bool ConfigOptionIsVisible(struct config_generic *conf);
+
/* get the current set of variables */
extern struct config_generic **get_guc_variables(int *num_vars);
On Mon, Jan 23, 2023 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Mon, Jan 23, 2023 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> 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
> 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 <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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 >
>>> 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 <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > On Mon, Jan 23, 2023 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
On Tue, Jan 24, 2023 at 8:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > On Mon, Jan 23, 2023 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> 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
> 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 <tgl@sss.pgh.pa.us> wrote: > > Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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
Nitin Jadhav <nitinjadhavpostgres@gmail.com> 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