From: | Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Improve GetConfigOptionValues function |
Date: | 2023-01-25 12:06:22 |
Message-ID: | CAMm1aWbMTEJWsKVtHOx3mU8toF-uLs5gGF_jbQ=jY56oFu7inw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)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
>
From | Date | Subject | |
---|---|---|---|
Next Message | Nitin Jadhav | 2023-01-25 12:16:56 | Re: Improve GetConfigOptionValues function |
Previous Message | Amit Kapila | 2023-01-25 11:25:57 | Re: Logical replication timeout problem |