Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>
>>Thanks for reviewing and cleaning this up, Tom. I think I understand
>>most of your changes, but I wasn't sure why you changed
>> PointerGetDatum(PG_GETARG_TEXT_P(0))
>>to
>> PG_GETARG_DATUM(0)
>
>
> The former was okay, but seemed a little redundant; you have a Datum
> already, why convert to Text pointer and back to Datum? There is
> a runtime cost here, it's not only a cast: GETARG_TEXT_P implies a
> detoasting check. Since text_out will do that anyway, it doesn't
> seem necessary to do it here.
OK. Makes sense.
>
> One thing that possibly needs discussion is the handling of
> GUC_NO_SHOW_ALL. I moved that test into the SHOW ALL code because
> the intended behavior is for the marked variable to not be in the
> SHOW ALL output at all, rather than be there with a null value as
> your patch originally behaved. Now that was fine for SHOW ALL because
> it can examine the config record directly anyway, but what of external
> callers of GetConfigOptionByNum? There aren't any right now so I'm
> kind of speculating in a vacuum about what they'll want.
But there will be as soon as I submit contrib/tablefunc
> But it seems possible that they will want to be able to discover
> whether the var is marked NO_SHOW_ALL or not. Maybe that should be
> an additional return variable from GetConfigOptionByNum, along the
> lines of
>
> GetConfigOptionByNum(..., bool *noshow)
> {
> if (noshow)
> *noshow = (conf->flags & GUC_NO_SHOW_ALL) ? true : false;
> }
>
> Any thoughts?
I see seed and session_authorization as the only two (at least
currently) settings marked GUC_NO_SHOW_ALL. I tried searching the
archives, but I can't find an explanation anywhere as to why there are
some settings we don't want to see in SHOW ALL. You can still do:
test=# SHOW session_authorization;
session_authorization
-----------------------
postgres
(1 row)
and see the setting, so why leave it out of SHOW ALL results? Or is this
behavior an artifact of my patch?
in 7.2.1 i get:
test=# SHOW seed;
NOTICE: Seed for random number generator is unavailable
SHOW VARIABLE
and cvs:
test=# SHOW seed;
seed
-------------
unavailable
(1 row)
>
> Oh btw: an Assert() verifying that the arg of GetConfigOptionByNum is
> in range wouldn't be out of place, I think.
>
OK -- I'll add one if I end up modifying this again.
Thanks,
Joe
