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:
Show quoted text
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
Attachments:
v1-0001-Improve-GetConfigOptionValues-function.patchapplication/octet-stream; name=v1-0001-Improve-GetConfigOptionValues-function.patchDownload
From bd096c4add9b2c04e39f80015d3c48900c44573a Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjadhav@microsoft.com>
Date: Wed, 18 Jan 2023 07:44:41 +0000
Subject: [PATCH] Improve GetConfigOptionValues function.
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. Its unnecessary to fetch all the values during noshow. Hence added
return statement.
---
src/backend/utils/misc/guc_funcs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index d59706231b..c6ac31dd59 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -595,7 +595,10 @@ GetConfigOptionValues(struct config_generic *conf, const char **values,
if ((conf->flags & GUC_NO_SHOW_ALL) ||
((conf->flags & GUC_SUPERUSER_ONLY) &&
!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+ {
*noshow = true;
+ return;
+ }
else
*noshow = false;
}
--
2.25.1
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 ofstatic 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:
Show quoted text
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 ofstatic 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
Show quoted text
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 ofstatic bool
ConfigOptionIsShowable(struct config_generic *conf)static void
GetConfigOptionValues(struct config_generic *conf, const char **values)regards, tom lane
Attachments:
v2-0001-Refactor-GetConfigOptionValues-function.patchapplication/octet-stream; name=v2-0001-Refactor-GetConfigOptionValues-function.patchDownload
From ed1cb165ab7bce3acc5daa1ddb84666bd03ca6cc Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjadhav@microsoft.com>
Date: Thu, 19 Jan 2023 09:49:51 +0000
Subject: [PATCH] Refactor GetConfigOptionValues function.
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. Its unnecessary to fetch all the values during noshow. Hence refactored
GetConfigOptionValues function. Added a new function ConfigOptionIsShowable
which decides whether the parameter is visible or not.
---
src/backend/utils/misc/guc.c | 4 +--
src/backend/utils/misc/guc_funcs.c | 57 +++++++++++++++---------------
src/include/utils/guc_tables.h | 3 ++
3 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d52069f446..7add80dfa7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5160,9 +5160,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 (!ConfigOptionIsShowable(conf))
continue;
/* return only options that are different from their boot values */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index d59706231b..95dd5c5053 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -492,9 +492,7 @@ 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)))
+ if (!ConfigOptionIsShowable(conf))
continue;
/* assign to the values array */
@@ -581,25 +579,29 @@ pg_settings_get_flags(PG_FUNCTION_ARGS)
PG_RETURN_ARRAYTYPE_P(a);
}
+/*
+ * Check whether the GUC variable is visible or not.
+ * Return true if visible, false otherwise.
+ */
+bool
+ConfigOptionIsShowable(struct config_generic *conf)
+{
+ if ((conf->flags & GUC_NO_SHOW_ALL) ||
+ ((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 */
@@ -943,27 +945,24 @@ show_all_settings(PG_FUNCTION_ARGS)
if (call_cntr < max_calls) /* do when there is more left to send */
{
char *values[NUM_PG_SETTINGS_ATTS];
- bool noshow;
HeapTuple tuple;
Datum result;
/*
- * Get the next visible GUC variable name and value
+ * Get the next visible GUC variable name
*/
- do
+ while (!ConfigOptionIsShowable(guc_vars[call_cntr]))
{
- 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;
+ /* bump the counter and get the next config setting */
+ call_cntr = ++funcctx->call_cntr;
- /* make sure we haven't gone too far now */
- if (call_cntr >= max_calls)
- SRF_RETURN_DONE(funcctx);
- }
- } while (noshow);
+ /* make sure we haven't gone too far now */
+ if (call_cntr >= max_calls)
+ SRF_RETURN_DONE(funcctx);
+ }
+
+ /* Extract values for the given variable */
+ GetConfigOptionValues(guc_vars[call_cntr], (const char **) values);
/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1195abaa3d..56d1010d62 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -306,4 +306,7 @@ extern char *config_enum_get_options(struct config_enum *record,
const char *suffix,
const char *separator);
+/* check whether the GUC variable is visible */
+extern bool ConfigOptionIsShowable(struct config_generic *conf);
+
#endif /* GUC_TABLES_H */
--
2.25.1
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 ofstatic 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:
Show quoted text
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 ofstatic 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
Attachments:
v3-0001-Refactor-GetConfigOptionValues-function.patchapplication/octet-stream; name=v3-0001-Refactor-GetConfigOptionValues-function.patchDownload
From c52b394c811ec980096d248b227552ce854631f5 Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjadhav@microsoft.com>
Date: Mon, 23 Jan 2023 09:54:13 +0000
Subject: [PATCH] Refactor GetConfigOptionValues function.
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. Its unnecessary to fetch all the values during noshow. Hence refactored
GetConfigOptionValues function. Added a new function ConfigOptionIsShowable
which decides whether the parameter is visible or not.
---
src/backend/utils/misc/guc.c | 4 +--
src/backend/utils/misc/guc_funcs.c | 56 ++++++++++++++----------------
src/include/utils/guc_tables.h | 3 ++
3 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d52069f446..7add80dfa7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5160,9 +5160,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 (!ConfigOptionIsShowable(conf))
continue;
/* return only options that are different from their boot values */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index d59706231b..3cd991688f 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -492,9 +492,7 @@ 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)))
+ if (!ConfigOptionIsShowable(conf))
continue;
/* assign to the values array */
@@ -581,25 +579,28 @@ pg_settings_get_flags(PG_FUNCTION_ARGS)
PG_RETURN_ARRAYTYPE_P(a);
}
+/*
+ * Return whether the GUC variable is visible or not.
+ */
+bool
+ConfigOptionIsShowable(struct config_generic *conf)
+{
+ if ((conf->flags & GUC_NO_SHOW_ALL) ||
+ ((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 */
@@ -943,27 +944,24 @@ show_all_settings(PG_FUNCTION_ARGS)
if (call_cntr < max_calls) /* do when there is more left to send */
{
char *values[NUM_PG_SETTINGS_ATTS];
- bool noshow;
HeapTuple tuple;
Datum result;
/*
- * Get the next visible GUC variable name and value
+ * Get the next visible GUC variable name
*/
- do
+ while (!ConfigOptionIsShowable(guc_vars[call_cntr]))
{
- 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;
+ /* bump the counter and get the next config setting */
+ call_cntr = ++funcctx->call_cntr;
- /* make sure we haven't gone too far now */
- if (call_cntr >= max_calls)
- SRF_RETURN_DONE(funcctx);
- }
- } while (noshow);
+ /* make sure we haven't gone too far now */
+ if (call_cntr >= max_calls)
+ SRF_RETURN_DONE(funcctx);
+ }
+
+ /* Extract values for the given variable */
+ GetConfigOptionValues(guc_vars[call_cntr], (const char **) values);
/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1195abaa3d..b579967168 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -306,4 +306,7 @@ extern char *config_enum_get_options(struct config_enum *record,
const char *suffix,
const char *separator);
+/* return whether the GUC variable is visible or not */
+extern bool ConfigOptionIsShowable(struct config_generic *conf);
+
#endif /* GUC_TABLES_H */
--
2.25.1
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.
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
Attachments:
v4-0001-Refactor-GetConfigOptionValues-function.patchtext/x-diff; charset=us-ascii; name=v4-0001-Refactor-GetConfigOptionValues-function.patchDownload
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
Show quoted text
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
Show quoted text
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
Show quoted text
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