Re: pgsql/ oc/src/sgml/release.sgml rc/backend/com ...

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)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