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

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.

It's a minor judgment call, really; I would not have bothered to change
it if I hadn't been making nearby edits for other reasons.

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

Oh btw: an Assert() verifying that the arg of GetConfigOptionByNum is
in range wouldn't be out of place, I think.

regards, tom lane