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.

Search Discussions

  • Tom Lane at Sep 3, 2008 at 2:06 am

    Alvaro Herrera 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 at Sep 3, 2008 at 4:39 am

    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
  • Tom Lane at Sep 3, 2008 at 4:14 am

    Alvaro Herrera 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
  • Greg Smith at Sep 3, 2008 at 6:19 pm

    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
  • Tom Lane at Sep 3, 2008 at 6:55 pm

    Greg Smith 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
  • Greg Smith at Sep 3, 2008 at 8:04 pm

    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
  • Joshua Drake at Sep 3, 2008 at 8:08 pm

    On Wed, 3 Sep 2008 16:04:12 -0400 (EDT) Greg Smith 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
  • Tom Lane at Sep 3, 2008 at 10:58 pm

    Joshua Drake 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
  • Tom Lane at Sep 3, 2008 at 11:03 pm

    Greg Smith 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
  • Greg Smith at Sep 4, 2008 at 12:43 am
    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
  • Kevin Grittner at Sep 4, 2008 at 2:20 pm

    Greg Smith 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
  • Greg Smith at Sep 4, 2008 at 7:20 pm

    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
  • Greg Smith at Sep 4, 2008 at 12:49 am

    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
  • Alvaro Herrera at Sep 9, 2008 at 2:47 am

    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
  • Greg Smith at Sep 9, 2008 at 5:43 am

    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
  • Magnus Hagander at Sep 9, 2008 at 8:18 am

    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
  • Greg Smith at Sep 9, 2008 at 9:28 am

    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
  • Alvaro Herrera at Sep 9, 2008 at 1:05 pm

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 2, '08 at 1:17a
activeSep 9, '08 at 1:05p
posts19
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase