Thread: Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.

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

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.comPostgreSQL Development, 24x7 Support, Training and Services

>> 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
 

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.comPostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus <josh@agliodbs.com> 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

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?
        regards, tom lane

Robert Haas wrote:
> On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus <josh@agliodbs.com> 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

On Tue, Jul 6, 2010 at 10:42 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
previousemail. 

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

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. +

On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian <bruce@momjian.us> 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

Robert Haas <robertmhaas@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian <bruce@momjian.us> 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

On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian <bruce@momjian.us> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

On Tue, Jul 6, 2010 at 3:40 PM, Greg Smith <greg@2ndquadrant.com> 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

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

On Tue, Jul 6, 2010 at 5:49 PM, Greg Smith <greg@2ndquadrant.com> 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

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
 

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