Re: [patch] GUC source file and line number]

Greg just sent me this patch, augmenting the one I sent to add source
file and line to GUC vars; Greg's patch adds a column with the default
value of each var.

I forward it to -hackers to have a public Message-Id to link to in the
Commitfest page.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Greg just sent me this patch, augmenting the one I sent to add source
> file and line to GUC vars; Greg's patch adds a column with the default
> value of each var.

I haven't tested, but doesn't this lose the source-location information
if a setting acquired from the config file is temporarily overridden via
SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to
NULL and not come back.

Since there is no possibility that any module outside GUC should ever
supply a config file location, I don't think that changing the API for
set_config_option is a good idea. Instead have ProcessConfigFile()
call some internal function that's not used by anyone else, and let
set_config_option assume that it's setting a non-file-sourced value.
That'd reduce the footprint of the patch quite a bit.

I dislike using the terminology "default" so cavalierly, because that
is a fairly slippery concept in GUC. Default for whom, with respect
to what? It looks like the patch actually means it to be the boot_val,
but I think a lot of users would think that "default" refers to the
reset value, ie, what their setting will be if they haven't said SET.
The fact that the session default didn't necessarily come from the
config file (see ALTER USER SET, ALTER DATABASE SET) complicates matters
still more. *Please* use some other word than "default" to title that
column. Also, I think that a reasonable case could be made for exposing
both boot_val and reset_val in the view --- if there is a use for one,
there is likely to be a use for the other.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Greg just sent me this patch, augmenting the one I sent to add source
> > file and line to GUC vars; Greg's patch adds a column with the default
> > value of each var.
>
> I haven't tested, but doesn't this lose the source-location information
> if a setting acquired from the config file is temporarily overridden via
> SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to
> NULL and not come back.

Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't,
because the location is saved as "reset location" and restored when the
variable is reset. It worked fine in all cases I tested.

> Since there is no possibility that any module outside GUC should ever
> supply a config file location, I don't think that changing the API for
> set_config_option is a good idea. Instead have ProcessConfigFile()
> call some internal function that's not used by anyone else, and let
> set_config_option assume that it's setting a non-file-sourced value.
> That'd reduce the footprint of the patch quite a bit.

Will look into it. FWIW I think most of the callers of
set_config_option are already abusing the API, because they should be
calling SetConfigOption instead. Maybe this needs some cleanup.

> Also, I think that a reasonable case could be made for exposing
> both boot_val and reset_val in the view --- if there is a use for one,
> there is likely to be a use for the other.

How about having two new columns "reset value" and "boot value"?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> I haven't tested, but doesn't this lose the source-location information
>> if a setting acquired from the config file is temporarily overridden via
>> SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to
>> NULL and not come back.

> Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't,
> because the location is saved as "reset location" and restored when the
> variable is reset. It worked fine in all cases I tested.

Hmm. Actually, why is there a need to save and restore at all? There
can certainly never be more than one recorded config-file location per
variable. What about saying that each variable has one and only one
filename/linenumber, but whether these are relevant to the current value
is determined by whether the current value's source is S_FILE?

(This would help to address one of the other things I find annoying
about the patch, which is the amount of storage it eats up for N copies
of what will always be the same filename in practice...)

> Will look into it. FWIW I think most of the callers of
> set_config_option are already abusing the API, because they should be
> calling SetConfigOption instead. Maybe this needs some cleanup.

Yeah, could be. Maybe set_config_option shouldn't be declared in guc.h?

>> Also, I think that a reasonable case could be made for exposing
>> both boot_val and reset_val in the view --- if there is a use for one,
>> there is likely to be a use for the other.

> How about having two new columns "reset value" and "boot value"?

Like it better than "default value" ...

regards, tom lane

On Tue, 2 Sep 2008, Tom Lane wrote:

>> How about having two new columns "reset value" and "boot value"?
>
> Like it better than "default value" ...

It's being a bit pedantic at the expense of the user, but I don't really
care that much here. I exposed the boot_val and described it in the
documentation as:

"Default value if the parameter is not explicitly set"

That's the value that people care about--if they comment out a setting
altogether and restart the server, what will it go back to. New admins
and people playing with the config files in a tuning content aren't often
using sighup in my experience, they just restart the server after changes.

I'm not aware of any specific use case for exposing the reset value other
than for completeness sake. Having both exposed with names that don't
mean anything to new admins is making the user experience more difficult
than it needs to be. That was why I just picked the more important one
and named it "default"; that makes the case for the average user so easy
they don't even need to look at the documentation.

I note the ongoing GUC units debate as a reminder that a technically
correct UI is usually preferred in this project to an easier to use but
slightly ambiguous one, and I'm not going to argue for "default" further
if everyone else is happy with a cryptic naming instead. The important
thing is that the boot_val gets exposed somehow so tool writers can
trivially present it as an option.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith <gsmith(at)gregsmith(dot)com> writes:
> On Tue, 2 Sep 2008, Tom Lane wrote:
>>> How about having two new columns "reset value" and "boot value"?
>>
>> Like it better than "default value" ...

> It's being a bit pedantic at the expense of the user, but I don't really
> care that much here. I exposed the boot_val and described it in the
> documentation as:

> "Default value if the parameter is not explicitly set"

If that statement were the truth, the whole truth, and nothing but the
truth, and if it didn't ignore the point about "explicitly set WHERE?",
I'd be fine with it.

> That was why I just picked the more important one
> and named it "default";

More important to whom? You are adopting a very narrow mindset,
which seems to be that only DBAs look at this view.

regards, tom lane

On Wed, 3 Sep 2008, Tom Lane wrote:

>> "Default value if the parameter is not explicitly set"
> If that statement were the truth, the whole truth, and nothing but the
> truth, and if it didn't ignore the point about "explicitly set WHERE?",
> I'd be fine with it.

First question--how about if I changed that description to read:

"Default value used at server startup if the parameter is not explicitly
set"?

I could then expose reset-val, named like that and with a description that
explained the context it applies in. And then we've give people a way to
experiment and understand the FAQ of "why didn't the value go back to the
default when I commented it out of the postgresql.conf and HUP'd the
server?".

Section question: with those changes, would it then be reasonable to you
to keep that column named "default" instead of giving it a less common
name?

> You are adopting a very narrow mindset, which seems to be that only DBAs
> look at this view.

DBAs are the only group I am always getting questions in this area from.
Everybody else seemed happy with the status quo, where the value wasn't
exposed at all and you just looked in guc.c to see what it was.

About once a month, somebody asks me "how can I tell what the default is
for *X*?" I want to be able to answer this question with "look in
pg_settings", which is easy enough to remember, and not have to say
anything else. That's the source of my mindset here, and I'm sure I'm not
alone in fielding that so often.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

On Wed, 3 Sep 2008 16:04:12 -0400 (EDT)
Greg Smith <gsmith(at)gregsmith(dot)com> wrote:

> Section question: with those changes, would it then be reasonable to
> you to keep that column named "default" instead of giving it a less
> common name?
>
> > You are adopting a very narrow mindset, which seems to be that only
> > DBAs look at this view.
>
> DBAs are the only group I am always getting questions in this area
> from. Everybody else seemed happy with the status quo, where the
> value wasn't exposed at all and you just looked in guc.c to see what
> it was.

I guess I would ask, "Who else would we be targeting this for?". DBAs
seem to be the only logical choice.

Joshua D. Drake
--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua Drake <jd(at)commandprompt(dot)com> writes:
> I guess I would ask, "Who else would we be targeting this for?". DBAs
> seem to be the only logical choice.

Regular users look at pg_settings too, you know. Maybe *you* only
get questions from DBAs...

regards, tom lane

Greg Smith <gsmith(at)gregsmith(dot)com> writes:
> First question--how about if I changed that description to read:

> "Default value used at server startup if the parameter is not explicitly
> set"?

"... not otherwise set" would probably be an accurate phrasing.
(I'm thinking of corner cases like stuff absorbed from environment
variables, which aren't really "explicitly" set by any normal usage
of that term.)

> I could then expose reset-val, named like that and with a description that
> explained the context it applies in. And then we've give people a way to
> experiment and understand the FAQ of "why didn't the value go back to the
> default when I commented it out of the postgresql.conf and HUP'd the
> server?".

You do know that's an ex-FAQ as of 8.3? If we're designing this feature
to respond to that, we are wasting a lot of effort.

> About once a month, somebody asks me "how can I tell what the default is
> for *X*?"

I wonder how certain you can be of which meaning of "default" they have
in mind. I don't think it means the same thing to everybody that it
means to you.

regards, tom lane

Before I respond to Tom's comments, let me step back a second and add the
intro the deadline didn't leave me time for. There are two specific
things the bit I added to this GUC patch is aimed at:

1) Somebody has a postgresql.conf from a random source (saw it on the
Internet and pasted dubious stuff in/previous person working on the
server/etc.) and wants to know the default value they'd get if they just
commented a specific line or lines out.

2) A GUC tuning tool author wants to provide a UI for modifying a GUC
parameter that shows the default as input to the person deciding what to
set a parameter to. The interface I've always wanted to make available
would be...wait a minute, I can provide a working example now. Picture
this:

name | Recommended | Current | Min | Default | Max
-------------+-------------+---------+-------+---------+---------
wal_buffers | 1024kB | 64kB | 32 kB | 64 kB | 2048 MB

With your cursor lighting up either the "Recommended" or "Current" field,
depending on whether you're a default approve or deny kind of tool
designer. Pretty simple interface to decide what to do, right? I find
that much less useful without the default value being included, but right
now someone who is writing a tuning tool has to maintain their own
database with that information if they want to do that. I will actually
do that for earlier versions the minute I know what the 8.4 solution that
makes the problem go away looks like.

The above is the output from:

select name,
'1024kB' as "Recommended",
current_setting(name) as "Current",
case when unit='8kB' then pg_size_pretty(min_val::int8*8192) else
min_val end as "Min",
case when unit='8kB' then pg_size_pretty(default_val::int8*8192) else
default_val end as "Default",
case when unit='8kB' then pg_size_pretty(max_val::int8) else max_val end
as "Max"
from pg_settings where name='wal_buffers';

on my system with the patch installed.

That's what I wanted but was unable to get until now. Combine that with
being able to figure out what source file and line the setting was
actually taken from, and the top 3 obstacles to writing a simple and easy
to use read/modify/write tuning tool are all cleared.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

On Wed, 3 Sep 2008, Tom Lane wrote:

> Greg Smith <gsmith(at)gregsmith(dot)com> writes:
>> First question--how about if I changed that description to read:
>
>> "Default value used at server startup if the parameter is not explicitly
>> set"?
>
> "... not otherwise set" would probably be an accurate phrasing.
> (I'm thinking of corner cases like stuff absorbed from environment
> variables, which aren't really "explicitly" set by any normal usage
> of that term.)

My opinion is that setting something in an environment variable certainly
is explicitly setting it, but it doesn't matter; "if the parameter is not
otherwise set" works just as well as far as I'm concerned.

>> I could then expose reset-val, named like that and with a description that
>> explained the context it applies in. And then we've give people a way to
>> experiment and understand the FAQ of "why didn't the value go back to the
>> default...
>
> You do know that's an ex-FAQ as of 8.3? If we're designing this feature
> to respond to that, we are wasting a lot of effort.

Sure, but there are a lot of pre-8.3 installs out there. I don't really
care about the reset-val at all, so I'm not going to justify whether or
not it should be included.

> I wonder how certain you can be of which meaning of "default" they have
> in mind. I don't think it means the same thing to everybody that it
> means to you.

When most people say "the default" talking about a value in a
configuration file, they mean the value the software will assume if that
setting isn't there at all. In the postgresql.conf context, that means
what they'll get if they start the server with that line missing or
commented out (and no environment variables, etc.) which is why I mapped
that to the boot_val. While I'm aware there are other uses of "default"
that apply in this context, I think they are extremely rare compared to
the common usage. The subtle distictions that require both a boot_val and
a reset_val internally are only important to people who are also capable
of understanding that "default" is a mass-consumption oriented label
that's a touch fuzzy IMHO.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

>>> Greg Smith <gsmith(at)gregsmith(dot)com> wrote:

> name | Recommended | Current | Min | Default | Max
> -------------+-------------+---------+-------+---------+---------
> wal_buffers | 1024kB | 64kB | 32 kB | 64 kB | 2048 MB

Personally, I would take the "Min", "Default", and "Max" to mean what
Greg intends; it's the "Current" one that gives me pause. The current
value of this connection? The value that a new connection will
currently get? The value which new connections will get after a
reload with the current conf file? The value which new connections
will get after a restart with the current conf file? I can understand
how someone would take one of these four values to be what is meant by
"Default", though.

-Kevin

On Thu, 4 Sep 2008, Kevin Grittner wrote:

> Personally, I would take the "Min", "Default", and "Max" to mean what
> Greg intends; it's the "Current" one that gives me pause.

That's the output of current_setting(name) which shows what it is right
now; no more, no less. See
http://www.postgresql.org/docs/current/interactive/sql-show.html and
http://www.postgresql.org/docs/current/interactive/functions-admin.html

That shows the parameter as the admin/usr set it, whereas the "setting"
column in pg_settings displays in terms of the internal units. Example
session showing how that all fits together:

pg=# select current_setting('work_mem');
current_setting
-----------------
1GB
(1 row)

pg=# set work_mem='256MB';
SET

pg=# select current_setting('work_mem');
current_setting
-----------------
256MB
(1 row)

pg=# select setting,unit from pg_settings where name='work_mem';
setting | unit
---------+------
262144 | kB
(1 row)

Since the word "current" isn't actually in the patch anywhere, and only
appears in that little sample usage snippet I provided, whether or not
it's a good name for that doesn't impact the patch itself.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane wrote:

> > Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't,
> > because the location is saved as "reset location" and restored when the
> > variable is reset. It worked fine in all cases I tested.
>
> Hmm. Actually, why is there a need to save and restore at all? There
> can certainly never be more than one recorded config-file location per
> variable. What about saying that each variable has one and only one
> filename/linenumber, but whether these are relevant to the current value
> is determined by whether the current value's source is S_FILE?

Hmm, this does make the patch a lot simpler. Attached. (Magnus was
visionary enough to put the correct test in the pg_settings definition.)

I also dropped the change to set_config_option, and added a new routine
to set the source file/line, as you suggested elsewhere. The only
problem is that we now have two bsearch calls for each option set in a
config file ... I don't want to change set_config_option just to be
able to return the struct config_generic for this routine's sake ...
Better ideas? Is this OK as is?

I noticed some weird things when the config files contain errors, but I
think it's outside this patch's scope.

(I dropped the "default" stuff for now, as it doesn't seem that a
consensus has been reached on that topic.)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, 8 Sep 2008, Alvaro Herrera wrote:

> (I dropped the "default" stuff for now, as it doesn't seem that a
> consensus has been reached on that topic.)

I have multiple GUC-related projects that are all stalled waiting for that
capability to be added. The only thing there wasn't clear consensus on
was exactly what the name for it should be, and there I really don't care.
I made the argument for why I named it the way I did, but if it gets
committed with a less friendly name (like boot_val) I won't complain.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Alvaro Herrera wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>> Tom Lane wrote:
>
>>> Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't,
>>> because the location is saved as "reset location" and restored when the
>>> variable is reset. It worked fine in all cases I tested.
>> Hmm. Actually, why is there a need to save and restore at all? There
>> can certainly never be more than one recorded config-file location per
>> variable. What about saying that each variable has one and only one
>> filename/linenumber, but whether these are relevant to the current value
>> is determined by whether the current value's source is S_FILE?
>
> Hmm, this does make the patch a lot simpler. Attached. (Magnus was
> visionary enough to put the correct test in the pg_settings definition.)

:-)

Yeah, it does look at lot simpler. And it certainly takes away the
pieces of code of mine that I was entirely unable to make working :-)

> I also dropped the change to set_config_option, and added a new routine
> to set the source file/line, as you suggested elsewhere. The only
> problem is that we now have two bsearch calls for each option set in a
> config file ... I don't want to change set_config_option just to be
> able to return the struct config_generic for this routine's sake ...
> Better ideas? Is this OK as is?

Well, it's not like it's a performance critical path, is it? I think we
should be ok.

> I noticed some weird things when the config files contain errors, but I
> think it's outside this patch's scope.
>
> (I dropped the "default" stuff for now, as it doesn't seem that a
> consensus has been reached on that topic.)

This is one of the reasons I suggested keeping that one as a separate
patch in the first place. The other main reason being that once it gets
applied, you really want it to be two different revisions, to clearly
keep them apart ;-) I still think we should eventually get both in
there, but let's treat them as separate entities.

//Magnus

On Tue, 9 Sep 2008, Magnus Hagander wrote:

>> (I dropped the "default" stuff for now, as it doesn't seem that a
>> consensus has been reached on that topic.)
>
> This is one of the reasons I suggested keeping that one as a separate
> patch in the first place. The other main reason being that once it gets
> applied, you really want it to be two different revisions, to clearly
> keep them apart

This means some committer is going to have to make a second pass over the
same section of code and do testing there more than once, that's a waste
of time I was trying to avoid. Also, any standalone patch I submit right
now won't apply cleanly if the source file/line patch is committed.

If nobody cares about doing that work twice, I'll re-submit a separate
patch once this one is resolved one way or another. I hope you snagged
the documentation update I added to your patch though.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith wrote:
> On Tue, 9 Sep 2008, Magnus Hagander wrote:
>
>>> (I dropped the "default" stuff for now, as it doesn't seem that a
>>> consensus has been reached on that topic.)
>>
>> This is one of the reasons I suggested keeping that one as a separate
>> patch in the first place. The other main reason being that once it gets
>> applied, you really want it to be two different revisions, to clearly
>> keep them apart
>
> This means some committer is going to have to make a second pass over the
> same section of code and do testing there more than once, that's a waste
> of time I was trying to avoid.

Actually, this is done all the time.

> Also, any standalone patch I submit right now won't apply cleanly if
> the source file/line patch is committed.

You can always start from the patched version and use interdiff to
obtain a "patch difference" ...

> If nobody cares about doing that work twice, I'll re-submit a separate
> patch once this one is resolved one way or another. I hope you snagged
> the documentation update I added to your patch though.

Yeah, I did.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support