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
