Re: Improve GetConfigOptionValues function

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Improve GetConfigOptionValues function
Date: 2023-01-23 16:21:53
Message-ID: 1662664.1674490913@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v4-0001-Refactor-GetConfigOptionValues-function.patch text/x-diff 5.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dean Rasheed 2023-01-23 16:22:35 Re: Non-decimal integer literals
Previous Message Peter Eisentraut 2023-01-23 15:55:17 Re: Non-decimal integer literals