FAQ

On 25/06/10 16:11, Simon Riggs wrote:
Log Message:
-----------
Fix log_temp_files docs and comments to say bytes not kilobytes.
stat(2) field st_size returns bytes not kilobytes.
Bug found during recent performance tuning for PostgreSQL user.

Modified Files:
--------------
pgsql/doc/src/sgml:
config.sgml (r1.282 -> r1.283)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/config.sgml?r1=1.282&r2=1.283)
pgsql/src/backend/utils/misc:
guc.c (r1.556 -> r1.557)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.556&r2=1.557)
Hmm, GUC_UNIT_KB doesn't seem appropriate anymore. I'd suggest changing
the code to match the documentation instead.

Shouldn't this be backpatched, or was this a new bug in 9.0?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Search Discussions

  • Simon Riggs at Jun 25, 2010 at 3:14 pm

    On Fri, 2010-06-25 at 17:25 +0300, Heikki Linnakangas wrote:
    On 25/06/10 16:11, Simon Riggs wrote:
    Log Message:
    -----------
    Fix log_temp_files docs and comments to say bytes not kilobytes.
    stat(2) field st_size returns bytes not kilobytes.
    Bug found during recent performance tuning for PostgreSQL user.

    Modified Files:
    --------------
    pgsql/doc/src/sgml:
    config.sgml (r1.282 -> r1.283)
    (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/config.sgml?r1=1.282&r2=1.283)
    pgsql/src/backend/utils/misc:
    guc.c (r1.556 -> r1.557)
    (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.556&r2=1.557)
    Hmm, GUC_UNIT_KB doesn't seem appropriate anymore. I'd suggest changing
    the code to match the documentation instead.
    Well, perhaps it does. Maybe I should fix the report to output KB
    instead and revert all the comments to "kilo"
    Shouldn't this be backpatched, or was this a new bug in 9.0?
    Spose so.

    --
    Simon Riggs www.2ndQuadrant.com
    PostgreSQL Development, 24x7 Support, Training and Services
  • Josh Berkus at Jun 25, 2010 at 5:56 pm
    Shouldn't this be backpatched, or was this a new bug in 9.0?
    We've always output bytes. I'd have noticed the discrepancy myself if
    I'd read the actual docs ;-)

    KB would be more useful. And I don't think people have enough scripts
    built on this yet to make this break anything. We should backport to 8.4.

    --
    -- Josh Berkus
    PostgreSQL Experts Inc.
    http://www.pgexperts.com
  • Simon Riggs at Jun 27, 2010 at 11:49 am

    On Fri, 2010-06-25 at 10:56 -0700, Josh Berkus wrote:
    Shouldn't this be backpatched, or was this a new bug in 9.0?
    We've always output bytes. I'd have noticed the discrepancy myself if
    I'd read the actual docs ;-)
    We can still output bytes, no problem. The issue is that the parameter
    is actually measured in bytes, whereas the docs say kilobytes, so you
    get much more log output than you were expecting.
    KB would be more useful. And I don't think people have enough scripts
    built on this yet to make this break anything. We should backport to 8.4.
    OK

    --
    Simon Riggs www.2ndQuadrant.com
    PostgreSQL Development, 24x7 Support, Training and Services
  • Robert Haas at Jul 6, 2010 at 2:21 pm

    On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus wrote:
    Shouldn't this be backpatched, or was this a new bug in 9.0?
    We've always output bytes.  I'd have noticed the discrepancy myself if I'd
    read the actual docs ;-)

    KB would be more useful.  And I don't think people have enough scripts built
    on this yet to make this break anything.  We should backport to 8.4.
    log_temp_files was introduced in 8.3, so we'll need to backpatch this
    to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with
    other things, so I'm going to pick this up. Barring objections, I'm
    going to revert Simon's patch and change the behavior instead, per the
    attached patch. I will also back-patch the fix to 8.4 and 8.3.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Tom Lane at Jul 6, 2010 at 2:42 pm

    Robert Haas writes:
    log_temp_files was introduced in 8.3, so we'll need to backpatch this
    to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with
    other things, so I'm going to pick this up. Barring objections, I'm
    going to revert Simon's patch and change the behavior instead, per the
    attached patch. I will also back-patch the fix to 8.4 and 8.3.
    I agree with doing this for 9.0, but I'm much less sold that it's a good
    idea to change the meaning of the parameter in minor releases of 8.4 and
    8.3. Aren't you risking breaking installations that work now?

    regards, tom lane
  • Robert Haas at Jul 6, 2010 at 2:52 pm

    On Tue, Jul 6, 2010 at 10:42 AM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    log_temp_files was introduced in 8.3, so we'll need to backpatch this
    to 8.3, not just 8.4.  Greg Smith tells me Simon has been busy with
    other things, so I'm going to pick this up.  Barring objections, I'm
    going to revert Simon's patch and change the behavior instead, per the
    attached patch.  I will also back-patch the fix to 8.4 and 8.3.
    I agree with doing this for 9.0, but I'm much less sold that it's a good
    idea to change the meaning of the parameter in minor releases of 8.4 and
    8.3.  Aren't you risking breaking installations that work now?
    I think my least favorite option is changing the behavior only in
    HEAD. I think the reasonable options are:

    1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion.
    If we do this, we should do what I proposed in my previous email.

    2. Change the comments and documentation in 8.4 and 8.3 along the
    lines that Simon already did in HEAD. If we do this, we also need to
    change the GUC units to something other than GUC_UNIT_KB, as noted
    upthread. I'm not sure what would be appropriate.

    The reason I think it's OK to change the behavior in the back-branches
    is that (a) the only thing it affects is logging, so it shouldn't
    really "break" anything, and (b) apparently nobody has noticed that
    the interpretation of the GUC is off by three orders of magnitude, so
    either nobody's using it or they're not looking at what's actually
    happening too carefully. But I'm OK with going the other way and
    changing the code and docs in the back-branches, too. I just think we
    should be consistent.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Bruce Momjian at Jul 6, 2010 at 3:03 pm

    Robert Haas wrote:
    I think my least favorite option is changing the behavior only in
    HEAD. I think the reasonable options are:

    1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion.
    If we do this, we should do what I proposed in my previous email.

    2. Change the comments and documentation in 8.4 and 8.3 along the
    lines that Simon already did in HEAD. If we do this, we also need to
    change the GUC units to something other than GUC_UNIT_KB, as noted
    upthread. I'm not sure what would be appropriate.

    The reason I think it's OK to change the behavior in the back-branches
    is that (a) the only thing it affects is logging, so it shouldn't
    really "break" anything, and (b) apparently nobody has noticed that
    the interpretation of the GUC is off by three orders of magnitude, so
    either nobody's using it or they're not looking at what's actually
    happening too carefully. But I'm OK with going the other way and
    changing the code and docs in the back-branches, too. I just think we
    should be consistent.
    I normally don't backpatch anything unless it is either a possible cause
    of data loss, or a problem that is reported by multiple people.

    Anything backpatched risks causing instability, and might discourage
    people from performing minor upgrades. Minor fixes are rarely worth the
    risk of causing instability in back-branches.

    --
    Bruce Momjian <bruce@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + None of us is going to be here forever. +
  • Robert Haas at Jul 6, 2010 at 3:05 pm

    On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian wrote:
    Robert Haas wrote:
    I think my least favorite option is changing the behavior only in
    HEAD.  I think the reasonable options are:

    1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion.
    If we do this, we should do what I proposed in my previous email.

    2. Change the comments and documentation in 8.4 and 8.3 along the
    lines that Simon already did in HEAD.  If we do this, we also need to
    change the GUC units to something other than GUC_UNIT_KB, as noted
    upthread.  I'm not sure what would be appropriate.

    The reason I think it's OK to change the behavior in the back-branches
    is that (a) the only thing it affects is logging, so it shouldn't
    really "break" anything, and (b) apparently nobody has noticed that
    the interpretation of the GUC is off by three orders of magnitude, so
    either nobody's using it or they're not looking at what's actually
    happening too carefully.  But I'm OK with going the other way and
    changing the code and docs in the back-branches, too.  I just think we
    should be consistent.
    I normally don't backpatch anything unless it is either a possible cause
    of data loss, or a problem that is reported by multiple people.

    Anything backpatched risks causing instability, and might discourage
    people from performing minor upgrades.  Minor fixes are rarely worth the
    risk of causing instability in back-branches.
    OK. Well, in that case, I think we should backpatch the changes Simon
    already made, and also pick a new unit for the GUC.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Tom Lane at Jul 6, 2010 at 3:10 pm

    Robert Haas writes:
    On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian wrote:
    Anything backpatched risks causing instability, and might discourage
    people from performing minor upgrades.  Minor fixes are rarely worth the
    risk of causing instability in back-branches.
    OK. Well, in that case, I think we should backpatch the changes Simon
    already made, and also pick a new unit for the GUC.
    Changing the unit setting would also be a behavioral change. I think
    what Bruce is suggesting is that this is simply not worth worrying about
    in the back branches.

    regards, tom lane
  • Robert Haas at Jul 6, 2010 at 3:17 pm

    On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian wrote:
    Anything backpatched risks causing instability, and might discourage
    people from performing minor upgrades.  Minor fixes are rarely worth the
    risk of causing instability in back-branches.
    OK.  Well, in that case, I think we should backpatch the changes Simon
    already made, and also pick a new unit for the GUC.
    Changing the unit setting would also be a behavioral change.  I think
    what Bruce is suggesting is that this is simply not worth worrying about
    in the back branches.
    It seems pretty strange not to at least document it. And I'm not wild
    about adding documentation that says "Even though this value purports
    to be in kilobytes, it's really not", but I guess we can.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Tom Lane at Jul 6, 2010 at 3:25 pm

    Robert Haas writes:
    On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane wrote:
    Changing the unit setting would also be a behavioral change.  I think
    what Bruce is suggesting is that this is simply not worth worrying about
    in the back branches.
    It seems pretty strange not to at least document it. And I'm not wild
    about adding documentation that says "Even though this value purports
    to be in kilobytes, it's really not", but I guess we can.
    Uh, no, the suggestion is to do *nothing* in the back branches. Yes
    they're buggy, but without any field complaints, it's hard to argue that
    anyone much cares. And I agree with Greg Smith that for anyone who does
    care, a behavioral change in a minor release is much harder to deal with
    than a change at a major release.

    regards, tom lane
  • Robert Haas at Jul 6, 2010 at 3:29 pm

    On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane wrote:
    without any field complaints,
    I refer you to Simon's original commit message: "Bug found during
    recent performance tuning for PostgreSQL user."

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Robert Haas at Jul 6, 2010 at 6:39 pm

    On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane wrote:
    Changing the unit setting would also be a behavioral change.  I think
    what Bruce is suggesting is that this is simply not worth worrying about
    in the back branches.
    It seems pretty strange not to at least document it.  And I'm not wild
    about adding documentation that says "Even though this value purports
    to be in kilobytes, it's really not", but I guess we can.
    Uh, no, the suggestion is to do *nothing* in the back branches.  Yes
    they're buggy, but without any field complaints, it's hard to argue that
    anyone much cares.  And I agree with Greg Smith that for anyone who does
    care, a behavioral change in a minor release is much harder to deal with
    than a change at a major release.
    OK, so I talked to Bruce about this and I guess I've been persuaded
    that we should just apply the patch I sent upthread to HEAD and leave
    the back-branches broken, for fear of creating an incompatibility.
    I'll go do that unless someone wants to argue further...

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Greg Smith at Jul 6, 2010 at 7:40 pm

    Robert Haas wrote:
    OK, so I talked to Bruce about this and I guess I've been persuaded
    that we should just apply the patch I sent upthread to HEAD and leave
    the back-branches broken, for fear of creating an incompatibility.
    The only thing that might be appropriate to backport is the docs fix,
    change "kilobytes" to "bytes" in config.sgml where the parameter is
    described. The bug complaint here was mad at the documentation not
    matching the behavior, rather than the behavior itself. I agree with
    the change to HEAD you've suggested though, that's the right way to do
    this for future releases.

    This change is something worth mentioning in the release notes for 9.0 too.

    --
    Greg Smith 2ndQuadrant US Baltimore, MD
    PostgreSQL Training, Services and Support
    greg@2ndQuadrant.com www.2ndQuadrant.us
  • Robert Haas at Jul 6, 2010 at 8:01 pm

    On Tue, Jul 6, 2010 at 3:40 PM, Greg Smith wrote:
    Robert Haas wrote:
    OK, so I talked to Bruce about this and I guess I've been persuaded
    that we should just apply the patch I sent upthread to HEAD and leave
    the back-branches broken, for fear of creating an incompatibility.
    The only thing that might be appropriate to backport is the docs fix, change
    "kilobytes" to "bytes" in config.sgml where the parameter is described.  The
    bug complaint here was mad at the documentation not matching the behavior,
    rather than the behavior itself.  I agree with the change to HEAD you've
    suggested though, that's the right way to do this for future releases.

    This change is something worth mentioning in the release notes for 9.0 too.
    I talked about that with Bruce. It wouldn't actually be sufficient to
    just say "it's in bytes", because if you do "show log_temp_files",
    it'll tell you that it's in kB, but it's really not. So we'd need to
    basically explain that it's a bug we're not fixing for fear of
    breaking existing installations. Bruce felt it wasn't worth putting
    that amount of work into backbranch docs that nobody's likely to read
    anyway, but I suppose that view could be overruled if there's a strong
    consensus.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Greg Smith at Jul 6, 2010 at 9:49 pm

    Robert Haas wrote:
    Bruce felt it wasn't worth putting that amount of work into backbranch docs that nobody's likely to read
    anyway, but I suppose that view could be overruled if there's a strong consensus.
    I was never arguing in favor of touching anything in the back branches;
    if you recall I didn't even voice an opinion here until I got concerned
    about too many changes happening in them. I think a proper fix in 9.0
    combined with a release notes comment noting the old/new behavior, so
    it's clear what was broken in the old versions, would be quite enough here.

    Something like this maybe:

    Change log_temp_files to be set in and output its value in kilobytes.
    Older versions had the parameter labeled and documented as being in KB,
    while it was actually set and outputting to the logs in bytes.

    Thanks for following this through, I think it's a useful small bit to
    get sorted out fully.

    --
    Greg Smith 2ndQuadrant US Baltimore, MD
    PostgreSQL Training, Services and Support
    greg@2ndQuadrant.com www.2ndQuadrant.us
  • Robert Haas at Jul 6, 2010 at 11:09 pm

    On Tue, Jul 6, 2010 at 5:49 PM, Greg Smith wrote:
    I was never arguing in favor of touching anything in the back branches; if
    you recall I didn't even voice an opinion here until I got concerned about
    too many changes happening in them.  I think a proper fix in 9.0 combined
    with a release notes comment noting the old/new behavior, so it's clear what
    was broken in the old versions, would be quite enough here.
    OK, commit done in head, with a note that we're deliberately not
    touching the back-branches and should release-note the change. Open
    item removed, also.
    Thanks for following this through, I think it's a useful small bit to get
    sorted out fully.
    yw

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Tom Lane at Jul 6, 2010 at 3:06 pm

    Robert Haas writes:
    The reason I think it's OK to change the behavior in the back-branches
    is that (a) the only thing it affects is logging, so it shouldn't
    really "break" anything, and (b) apparently nobody has noticed that
    the interpretation of the GUC is off by three orders of magnitude, so
    either nobody's using it or they're not looking at what's actually
    happening too carefully.
    It might be that nobody's using any values other than 0 and -1 ...
    in which case it wouldn't matter anyway. I agree that the lack of
    bug reports is notable. But still, don't we try to avoid behavioral
    changes in stable branches?

    I'm not dead set against doing what you propose, just think it needs
    some discussion.

    regards, tom lane
  • Josh Berkus at Jul 7, 2010 at 10:48 pm

    On 7/6/10 8:06 AM, Tom Lane wrote:
    It might be that nobody's using any values other than 0 and -1 ...
    in which case it wouldn't matter anyway. I agree that the lack of
    bug reports is notable. But still, don't we try to avoid behavioral
    changes in stable branches?
    I think most people are doing what I was doing: looking at the values in
    the logs, and writing math appropriately. Most of the other log output
    isn't documented well, and the output values are obviously bytes, so
    frankly it never occurred to me to check the docs.

    Agreed that backporting the fix to 8.3 and 8.4 is infeasible.

    --
    -- Josh Berkus
    PostgreSQL Experts Inc.
    http://www.pgexperts.com
  • Cédric Villemain at Jul 8, 2010 at 12:32 am

    2010/7/8 Josh Berkus <josh@agliodbs.com>:
    On 7/6/10 8:06 AM, Tom Lane wrote:
    It might be that nobody's using any values other than 0 and -1 ...
    in which case it wouldn't matter anyway.  I agree that the lack of
    bug reports is notable.  But still, don't we try to avoid behavioral
    changes in stable branches?
    I think most people are doing what I was doing: looking at the values in
    the logs, and writing math appropriately.  Most of the other log output
    isn't documented well, and the output values are obviously bytes, so
    frankly it never occurred to me to check the docs.
    Samething here
    Agreed that backporting the fix to 8.3 and 8.4 is infeasible.
    +1
    But I don't understand why not backport a documentation patch. Thing
    is identified, clearly boring for one just trusting the docs.
    --
    -- Josh Berkus
    PostgreSQL Experts Inc.
    http://www.pgexperts.com

    --
    Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
    To make changes to your subscription:
    http://www.postgresql.org/mailpref/pgsql-hackers


    --
    Cédric Villemain 2ndQuadrant
    http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support
  • Greg Smith at Jul 6, 2010 at 2:52 pm

    Robert Haas wrote:
    On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus wrote:

    Shouldn't this be backpatched, or was this a new bug in 9.0?
    We've always output bytes. I'd have noticed the discrepancy myself if I'd
    read the actual docs ;-)

    KB would be more useful. And I don't think people have enough scripts built
    on this yet to make this break anything. We should backport to 8.4.
    log_temp_files was introduced in 8.3, so we'll need to backpatch this
    to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with
    other things, so I'm going to pick this up. Barring objections, I'm
    going to revert Simon's patch and change the behavior instead, per the
    attached patch. I will also back-patch the fix to 8.4 and 8.3.
    I was expecting Josh's suggestion to get argued down...I'm curious why
    anyone is considering a behavior change in 8.3 or 8.4 over this? It's
    reasonable to change the behavior in 9.0, and/or to correct the docs for
    8.3 or 8.4, but I would think a behavior change in the earlier versions
    would be out of line with the project's version policies. I know I have
    scripts that will mysteriously break if this goes into a stable
    version. I'm used to writing things that have code for detecting major
    version and acting accordingly, but I'd really prefer this project
    doesn't ever wander down the path where I need to start writing in code
    for "do this on <=8.3.11, this other thing for >=8.3.12".

    --
    Greg Smith 2ndQuadrant US Baltimore, MD
    PostgreSQL Training, Services and Support
    greg@2ndQuadrant.com www.2ndQuadrant.us

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJun 25, '10 at 2:26p
activeJul 8, '10 at 12:32a
posts22
users8
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase