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