Обсуждение: pgsql/ oc/src/sgml/release.sgml rc/backend/com ...
CVSROOT: /cvsroot
Module name: pgsql
Changes by: tgl@postgresql.org 02/07/20 11:12:56
Modified files:
doc/src/sgml : release.sgml
src/backend/commands: explain.c
src/backend/utils/misc: guc.c
src/include/executor: executor.h
src/include/utils: guc.h
src/test/regress/expected: horology-no-DST-before-1970.out
horology-solaris-1947.out
horology.out
Log message:
Code review for SHOW output changes; fix horology expected files for
new SHOW output format.
Tom Lane wrote:
> CVSROOT: /cvsroot
> Module name: pgsql
> Changes by: tgl@postgresql.org 02/07/20 11:12:56
>
> Modified files:
> doc/src/sgml : release.sgml
> src/backend/commands: explain.c
> src/backend/utils/misc: guc.c
> src/include/executor: executor.h
> src/include/utils: guc.h
> src/test/regress/expected: horology-no-DST-before-1970.out
> horology-solaris-1947.out
> horology.out
>
> Log message:
> Code review for SHOW output changes; fix horology expected files for
> new SHOW output format.
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)
Somewhere I got the impression that the former was preferred, although
the two are equivalent and the latter is more compact.
Joe
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.
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
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
Joe Conway <mail@joeconway.com> writes:
>> 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
Okay, so the question is what tablefunc wants to do. I'd guess it
wants to duplicate the set of info returned by SHOW ALL.
> 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.
I put in GUC_NO_SHOW_ALL when I was hacking on GUC a few months ago
to make it able to implement the last few SET variables that had
one-of-a-kind behavior. One of those one-of-a-kind behaviors was that
some of them didn't show up in SHOW ALL. I suppose this is arguably
a bug and not really behavior we want to preserve --- although SHOW SEED
will *never* return anything useful and so it's not clear why SHOW ALL
should bother to show it.
If NO_SHOW_ALL bothers you, feel free to put its removal up to a
pghackers vote. I'm not wedded to it.
regards, tom lane
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: > Okay, so the question is what tablefunc wants to do. I'd guess it > wants to duplicate the set of info returned by SHOW ALL. Yes, I think that makes sense. > I put in GUC_NO_SHOW_ALL when I was hacking on GUC a few months ago > to make it able to implement the last few SET variables that had > one-of-a-kind behavior. One of those one-of-a-kind behaviors was that > some of them didn't show up in SHOW ALL. I suppose this is arguably > a bug and not really behavior we want to preserve --- although SHOW SEED > will *never* return anything useful and so it's not clear why SHOW ALL > should bother to show it. > > If NO_SHOW_ALL bothers you, feel free to put its removal up to a > pghackers vote. I'm not wedded to it. It doesn't really bother me ;). I just didn't understand why it existed. I do think that it is conceivable that we want to be able to suppress the examination of some settings, but I would think that would apply to SHOW just the same as SHOW ALL. It seems that anything I can SHOW should be there when I do SHOW ALL. Maybe it should be GUC_NO_SHOW and apply to both? Joe
Joe Conway <mail@joeconway.com> writes:
> I do think that it is conceivable that we want to be able to suppress
> the examination of some settings, but I would think that would apply to
> SHOW just the same as SHOW ALL. It seems that anything I can SHOW should
> be there when I do SHOW ALL. Maybe it should be GUC_NO_SHOW and apply to
> both?
Nah, that can be implemented by the show_hook subroutine refusing to
disclose anything. NO_SHOW_ALL is only useful for making SHOW ALL
vary from what SHOW does.
regards, tom lane