On Mon, Jan 23, 2023 at 9:51 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostg...@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
