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