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

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