Thread: [gsmith@gregsmith.com: 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.
Attached patch extends the last one Alvaro sent me to add a default_val
column to pg_settings.  I renumbered the new columns the sourcefile/line
patch added to fit it into a logical order (the default is just after the
enumvalues).  That and minimizing the number of times everybody has to
test in this area are why I wanted to squeeze this in at the same time.

Sample that shows the sort of thing I expect tuning tools will use this
for:

select name,setting,default_val from pg_settings where not
setting=default_val;

             name            |      setting       |    default_val
----------------------------+--------------------+-------------------
  archive_command            | (disabled)         |
  client_encoding            | UTF8               | SQL_ASCII
  default_text_search_config | pg_catalog.english | pg_catalog.simple
  lc_collate                 | en_US.UTF-8        | C
  lc_ctype                   | en_US.UTF-8        | C
  lc_messages                | en_US.UTF-8        |
  lc_monetary                | en_US.UTF-8        | C
  lc_numeric                 | en_US.UTF-8        | C
  lc_time                    | en_US.UTF-8        | C
  log_timezone               | US/Eastern         | UNKNOWN
  max_fsm_pages              | 204800             | 20000
  max_stack_depth            | 2048               | 100
  server_encoding            | UTF8               | SQL_ASCII
  shared_buffers             | 4096               | 1024
  TimeZone                   | US/Eastern         | UNKNOWN
  timezone_abbreviations     | Default            | UNKNOWN

While there is a "default" for source, it's impossible to distinguish
cases where someone set the value explicitly, but to the default value,
unless tool writers have their own database of what the defaults are.
That situation often happens when people uncomment the parameter in their
postgresql.conf but don't actually change it from the default.  Example:

select name,setting,default_val,sourcefile,sourceline from pg_settings
where name='max_connections';

       name       | setting | default_val |                   sourcefile
| sourceline
-----------------+---------+-------------+-------------------------------------------------+------------
  max_connections | 100     | 100         |
/home/gsmith/pgproject/guc/data/postgresql.conf |         61

Hope I didn't introduce any bugs in your otherwise clean patch.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

Alvaro Herrera <alvherre@commandprompt.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

Alvaro Herrera <alvherre@commandprompt.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

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.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

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@gregsmith.com http://www.gregsmith.com Baltimore, MD

Greg Smith <gsmith@gregsmith.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@gregsmith.com http://www.gregsmith.com Baltimore, MD

On Wed, 3 Sep 2008 16:04:12 -0400 (EDT)
Greg Smith <gsmith@gregsmith.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@commandprompt.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@gregsmith.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@gregsmith.com http://www.gregsmith.com Baltimore, MD

On Wed, 3 Sep 2008, Tom Lane wrote:

> Greg Smith <gsmith@gregsmith.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@gregsmith.com http://www.gregsmith.com Baltimore, MD

>>> Greg Smith <gsmith@gregsmith.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@gregsmith.com http://www.gregsmith.com Baltimore, MD

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.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@gregsmith.com http://www.gregsmith.com Baltimore, MD

Alvaro Herrera wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@commandprompt.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@gregsmith.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