Thread: Improve GetConfigOptionValues function

Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
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

Attachment

Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Tom Lane
Date:
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



Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
> 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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
> 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

Attachment

Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
> 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

Attachment

Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Tom Lane
Date:
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);


Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Tom Lane
Date:
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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
> 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
>



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
>>> 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



Re: Improve GetConfigOptionValues function

From
Tom Lane
Date:
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



Re: Improve GetConfigOptionValues function

From
Bharath Rupireddy
Date:
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



Re: Improve GetConfigOptionValues function

From
Nitin Jadhav
Date:
> 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



Re: Improve GetConfigOptionValues function

From
Tom Lane
Date:
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