> 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
>