Обсуждение: Should we increase the default vacuum_cost_limit?
Hi, I've had to do quite a bit of performance investigation work this year and it seems that I only too often discover that the same problem is repeating itself... A vacuum_cost_limit that is still set to the 200 default value along with all 3 auto-vacuum workers being flat chat trying and failing to keep up with the demand. I understand we often keep the default config aimed at low-end servers, but I don't believe we should categorise this option the same way as we do with shared_buffers and work_mem. What's to say that having an auto-vacuum that runs too slowly is better than one that runs too quickly? I have in mind that performance problems arising from having auto-vacuum run too quickly might be easier to diagnose and fix than the ones that arise from it running too slowly. Certainly, the aftermath cleanup involved with it running too slowly is quite a bit more tricky to solve. Ideally, we'd have something smarter than the cost limits we have today, something that perhaps is adaptive and can make more use of an idle server than we do now, but that sounds like a pretty large project to consider having it working this late in the cycle. In the meantime, should we consider not having vacuum_cost_limit set so low by default? I have in mind something in the ballpark of a 5x to 10x increase. It seems the standard settings only allow for a maximum of ~3.9MB/s dirty rate and ~7.8MB/s shared buffer miss rate. That seems pretty slow even for the micro SD card that's in my 4-year-old phone. I think we should be aiming for setting this to something good for the slightly better than average case of modern hardware. The current default vacuum_cost_limit of 200 seems to be 15 years old and was added in f425b605f4e. Any supporters for raising the default? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Feb 24, 2019 at 9:42 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > The current default vacuum_cost_limit of 200 seems to be 15 years old > and was added in f425b605f4e. > > Any supporters for raising the default? I also think that the current default limit is far too conservative. -- Peter Geoghegan
I support rising the default.
From standpoint of no-clue database admin, it's easier to give more resources to Postgres and google what process called "autovacuum" does than to learn why is it being slow on read.
It's also tricky that index only scans depend on working autovacuum, and autovacuum never comes to those tables. Since PG11 it's safe to call vacuum on table with indexes, since index is no longer being scanned in its entirety. I would also propose to include "tuples inserted" into formula for autovacuum threshold the same way it is done for autoanalyze threshold. This will fix the situation where you delete 50 rows in 100-gigabyte table and autovacuum suddenly goes to rewrite and reread hint bits on all of it, since it never touched it before.
On Mon, Feb 25, 2019 at 8:42 AM David Rowley <david.rowley@2ndquadrant.com> wrote:
Hi,
I've had to do quite a bit of performance investigation work this year
and it seems that I only too often discover that the same problem is
repeating itself... A vacuum_cost_limit that is still set to the 200
default value along with all 3 auto-vacuum workers being flat chat
trying and failing to keep up with the demand.
I understand we often keep the default config aimed at low-end
servers, but I don't believe we should categorise this option the same
way as we do with shared_buffers and work_mem. What's to say that
having an auto-vacuum that runs too slowly is better than one that
runs too quickly?
I have in mind that performance problems arising from having
auto-vacuum run too quickly might be easier to diagnose and fix than
the ones that arise from it running too slowly. Certainly, the
aftermath cleanup involved with it running too slowly is quite a bit
more tricky to solve.
Ideally, we'd have something smarter than the cost limits we have
today, something that perhaps is adaptive and can make more use of an
idle server than we do now, but that sounds like a pretty large
project to consider having it working this late in the cycle.
In the meantime, should we consider not having vacuum_cost_limit set
so low by default?
I have in mind something in the ballpark of a 5x to 10x increase. It
seems the standard settings only allow for a maximum of ~3.9MB/s dirty
rate and ~7.8MB/s shared buffer miss rate. That seems pretty slow
even for the micro SD card that's in my 4-year-old phone. I think we
should be aiming for setting this to something good for the slightly
better than average case of modern hardware.
The current default vacuum_cost_limit of 200 seems to be 15 years old
and was added in f425b605f4e.
Any supporters for raising the default?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Darafei Praliaskouski
Support me: http://patreon.com/komzpa
On 2/25/19 1:17 AM, Peter Geoghegan wrote: > On Sun, Feb 24, 2019 at 9:42 PM David Rowley > <david.rowley@2ndquadrant.com> wrote: >> The current default vacuum_cost_limit of 200 seems to be 15 years old >> and was added in f425b605f4e. >> >> Any supporters for raising the default? > > I also think that the current default limit is far too conservative. I agree entirely. In my experience you are usually much better off if vacuum finishes quickly. Personally I think our default scale factors are horrible too, especially when there are tables with large numbers of rows. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Tue, 26 Feb 2019 at 02:06, Joe Conway <mail@joeconway.com> wrote: > > On 2/25/19 1:17 AM, Peter Geoghegan wrote: > > On Sun, Feb 24, 2019 at 9:42 PM David Rowley > > <david.rowley@2ndquadrant.com> wrote: > >> The current default vacuum_cost_limit of 200 seems to be 15 years old > >> and was added in f425b605f4e. > >> > >> Any supporters for raising the default? > > > > I also think that the current default limit is far too conservative. > > I agree entirely. In my experience you are usually much better off if > vacuum finishes quickly. Personally I think our default scale factors > are horrible too, especially when there are tables with large numbers of > rows. Agreed that the scale factors are not perfect, but I don't think changing them is as quite a no-brainer as the vacuum_cost_limit, so the attached patch just does the vacuum_cost_limit. I decided to do the times by 10 option that I had mentioned.... Ensue debate about that... I'll add this to the March commitfest and set the target version as PG12. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
David Rowley wrote: > On Tue, 26 Feb 2019 at 02:06, Joe Conway <mail@joeconway.com> wrote: > > > > On 2/25/19 1:17 AM, Peter Geoghegan wrote: > > > On Sun, Feb 24, 2019 at 9:42 PM David Rowley > > > <david.rowley@2ndquadrant.com> wrote: > > >> The current default vacuum_cost_limit of 200 seems to be 15 years old > > >> and was added in f425b605f4e. > > >> > > >> Any supporters for raising the default? > > > > > > I also think that the current default limit is far too conservative. > > > > I agree entirely. In my experience you are usually much better off if > > vacuum finishes quickly. Personally I think our default scale factors > > are horrible too, especially when there are tables with large numbers of > > rows. > > Agreed that the scale factors are not perfect, but I don't think > changing them is as quite a no-brainer as the vacuum_cost_limit, so > the attached patch just does the vacuum_cost_limit. > > I decided to do the times by 10 option that I had mentioned.... Ensue > debate about that... > > I'll add this to the March commitfest and set the target version as PG12. I think this is a good move. It is way easier to recover from an over-eager autovacuum than from one that didn't manage to finish... Yours, Laurenz Albe
On Mon, Feb 25, 2019 at 4:44 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote: > > David Rowley wrote: > > On Tue, 26 Feb 2019 at 02:06, Joe Conway <mail@joeconway.com> wrote: > > > > > > On 2/25/19 1:17 AM, Peter Geoghegan wrote: > > > > On Sun, Feb 24, 2019 at 9:42 PM David Rowley > > > > <david.rowley@2ndquadrant.com> wrote: > > > >> The current default vacuum_cost_limit of 200 seems to be 15 years old > > > >> and was added in f425b605f4e. > > > >> > > > >> Any supporters for raising the default? > > [...] > > I'll add this to the March commitfest and set the target version as PG12. > > I think this is a good move. > > It is way easier to recover from an over-eager autovacuum than from > one that didn't manage to finish... +1
On Mon, Feb 25, 2019 at 8:39 AM David Rowley <david.rowley@2ndquadrant.com> wrote: > I decided to do the times by 10 option that I had mentioned.... Ensue > debate about that... +1 for raising the default substantially. In my experience, and it seems others are in a similar place, nobody ever gets into trouble because the default is too high, but sometimes people get in trouble because the default is too low. If we raise it enough that a few people have to reduce it and a few people have to further increase it, IMHO that would be about right. Not sure exactly what value would accomplish that goal. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > Not sure exactly what value would accomplish that goal. I think autovacuum_vacuum_cost_limit = 2000 is a good starting point. Yours, Laurenz Albe
On Mon, Feb 25, 2019 at 8:48 AM Robert Haas <robertmhaas@gmail.com> wrote: > +1 for raising the default substantially. In my experience, and it > seems others are in a similar place, nobody ever gets into trouble > because the default is too high, but sometimes people get in trouble > because the default is too low. Does anyone want to make an argument against the idea of raising the default? They should speak up now. -- Peter Geoghegan
On 3/5/19 1:14 AM, Peter Geoghegan wrote: > On Mon, Feb 25, 2019 at 8:48 AM Robert Haas <robertmhaas@gmail.com> wrote: >> +1 for raising the default substantially. In my experience, and it >> seems others are in a similar place, nobody ever gets into trouble >> because the default is too high, but sometimes people get in trouble >> because the default is too low. > > Does anyone want to make an argument against the idea of raising the > default? They should speak up now. > I don't know. On the one hand I don't feel very strongly about this change, and I have no intention to block it (because in most cases I do actually increase the value anyway). I wonder if those with small systems will be happy about it, though. But on the other hand it feels a bit weird that we increase this one value and leave all the other (also very conservative) defaults alone. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 5, 2019 at 7:53 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > But on the other hand it feels a bit weird that we increase this one > value and leave all the other (also very conservative) defaults alone. Are you talking about vacuum-related defaults or defaults in general? In 2014, we increased the defaults for work_mem and maintenance_work_mem by 4x and the default for effective_cache_size by 32x; in 2012, we increased the default for shared_buffers from by 4x. It's possible some of those parameters should be further increased at some point, but deciding not to increase any of them until we can increase all of them is tantamount to giving up on changing anything at all. I think it's OK to be conservative by default, but we should increase parameters where we know that the default is likely to be too conservative for 99% of users. My only question about this change is whether to go for a lesser multiple (e.g. 4x) rather than the proposed 10x. But I think even if 10x turns out to be too much for a few more people than we'd like, we're still going to be better off increasing it and having some people have to turn it back down again than leaving it the way it is and having users regularly suffer vacuum-starvation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/25/19 8:38 AM, David Rowley wrote: > On Tue, 26 Feb 2019 at 02:06, Joe Conway <mail@joeconway.com> wrote: >> On 2/25/19 1:17 AM, Peter Geoghegan wrote: >>> On Sun, Feb 24, 2019 at 9:42 PM David Rowley >>> <david.rowley@2ndquadrant.com> wrote: >>>> The current default vacuum_cost_limit of 200 seems to be 15 years old >>>> and was added in f425b605f4e. >>>> >>>> Any supporters for raising the default? >>> I also think that the current default limit is far too conservative. >> I agree entirely. In my experience you are usually much better off if >> vacuum finishes quickly. Personally I think our default scale factors >> are horrible too, especially when there are tables with large numbers of >> rows. > Agreed that the scale factors are not perfect, but I don't think > changing them is as quite a no-brainer as the vacuum_cost_limit, so > the attached patch just does the vacuum_cost_limit. > > I decided to do the times by 10 option that I had mentioned.... Ensue > debate about that... > > I'll add this to the March commitfest and set the target version as PG12. > This patch is tiny, seems perfectly reasonable, and has plenty of support. I'm going to commit it shortly unless there are last minute objections. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-03-05 17:14:55 -0500, Andrew Dunstan wrote: > This patch is tiny, seems perfectly reasonable, and has plenty of > support. I'm going to commit it shortly unless there are last minute > objections. +1
Thanks for chipping in on this. On Wed, 6 Mar 2019 at 01:53, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > But on the other hand it feels a bit weird that we increase this one > value and leave all the other (also very conservative) defaults alone. Which others did you have in mind? Like work_mem, shared_buffers? If so, I mentioned in the initial post that I don't see vacuum_cost_limit as in the same category as those. It's not like PostgreSQL won't start on a tiny server if vacuum_cost_limit is too high, but you will have issues with too big a shared_buffers, for example. I think if we insist that this patch is a review of all the "how big is your server" GUCs then that's raising the bar significantly and unnecessarily for what I'm proposing here. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 3/5/19 14:14, Andrew Dunstan wrote: > This patch is tiny, seems perfectly reasonable, and has plenty of > support. I'm going to commit it shortly unless there are last minute > objections. +1 -- Jeremy Schneider Database Engineer Amazon Web Services
On 3/6/19 1:38 PM, Jeremy Schneider wrote: > On 3/5/19 14:14, Andrew Dunstan wrote: >> This patch is tiny, seems perfectly reasonable, and has plenty of >> support. I'm going to commit it shortly unless there are last minute >> objections. > +1 > done. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/6/19 12:10 AM, David Rowley wrote: > Thanks for chipping in on this. > > On Wed, 6 Mar 2019 at 01:53, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: >> But on the other hand it feels a bit weird that we increase this one >> value and leave all the other (also very conservative) defaults alone. > > Which others did you have in mind? Like work_mem, shared_buffers? If > so, I mentioned in the initial post that I don't see vacuum_cost_limit > as in the same category as those. It's not like PostgreSQL won't > start on a tiny server if vacuum_cost_limit is too high, but you will > have issues with too big a shared_buffers, for example. I think if > we insist that this patch is a review of all the "how big is your > server" GUCs then that's raising the bar significantly and > unnecessarily for what I'm proposing here. > On second thought, I think you're right. It's still true that you need to bump up various other GUCs on reasonably current hardware, but it's true vacuum_cost_limit is special enough to increase it separately. so +1 (I see Andrew already pushed it, but anyway ...) -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 7 Mar 2019 at 08:54, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > > > On 3/6/19 1:38 PM, Jeremy Schneider wrote: > > On 3/5/19 14:14, Andrew Dunstan wrote: > >> This patch is tiny, seems perfectly reasonable, and has plenty of > >> support. I'm going to commit it shortly unless there are last minute > >> objections. > > +1 > > > > done. Thanks! -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Mar 6, 2019 at 2:54 PM Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
On 3/6/19 1:38 PM, Jeremy Schneider wrote:
> On 3/5/19 14:14, Andrew Dunstan wrote:
>> This patch is tiny, seems perfectly reasonable, and has plenty of
>> support. I'm going to commit it shortly unless there are last minute
>> objections.
> +1
>
done.
Now that this is done, the default value is only 5x below the hard-coded maximum of 10,000.
This seems a bit odd, and not very future-proof. Especially since the hard-coded maximum appears to have no logic to it anyway, at least none that is documented. Is it just mindless nannyism?
Any reason not to increase by at least a factor of 10, but preferably the largest value that does not cause computational problems (which I think would be INT_MAX)?
Cheers,
Jeff
Jeff Janes <jeff.janes@gmail.com> writes:
> Now that this is done, the default value is only 5x below the hard-coded
> maximum of 10,000.
> This seems a bit odd, and not very future-proof. Especially since the
> hard-coded maximum appears to have no logic to it anyway, at least none
> that is documented. Is it just mindless nannyism?
Hm. I think the idea was that rather than setting it to "something very
large", you'd want to just disable the feature via vacuum_cost_delay.
But I agree that the threshold for what is ridiculously large probably
ought to be well more than 5x the default, and maybe it is just mindless
nannyism to have a limit less than what the implementation can handle.
regards, tom lane
On Sat, 9 Mar 2019 at 07:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Jeff Janes <jeff.janes@gmail.com> writes: > > Now that this is done, the default value is only 5x below the hard-coded > > maximum of 10,000. > > This seems a bit odd, and not very future-proof. Especially since the > > hard-coded maximum appears to have no logic to it anyway, at least none > > that is documented. Is it just mindless nannyism? > > Hm. I think the idea was that rather than setting it to "something very > large", you'd want to just disable the feature via vacuum_cost_delay. > But I agree that the threshold for what is ridiculously large probably > ought to be well more than 5x the default, and maybe it is just mindless > nannyism to have a limit less than what the implementation can handle. Yeah, +1 to increasing it. I imagine that the 10,000 limit would not allow people to explore the upper limits of a modern PCI-E SSD with the standard delay time and dirty/miss scores. Also, it doesn't seem entirely unreasonable that someone somewhere might also want to fine-tune the hit/miss/dirty scores so that they're some larger factor apart from each other the standard scores are. The 10,000 limit does not allow much wiggle room for that. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 3/8/19 6:47 PM, David Rowley wrote: > On Sat, 9 Mar 2019 at 07:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Jeff Janes <jeff.janes@gmail.com> writes: >>> Now that this is done, the default value is only 5x below the hard-coded >>> maximum of 10,000. >>> This seems a bit odd, and not very future-proof. Especially since the >>> hard-coded maximum appears to have no logic to it anyway, at least none >>> that is documented. Is it just mindless nannyism? >> Hm. I think the idea was that rather than setting it to "something very >> large", you'd want to just disable the feature via vacuum_cost_delay. >> But I agree that the threshold for what is ridiculously large probably >> ought to be well more than 5x the default, and maybe it is just mindless >> nannyism to have a limit less than what the implementation can handle. > Yeah, +1 to increasing it. I imagine that the 10,000 limit would not > allow people to explore the upper limits of a modern PCI-E SSD with > the standard delay time and dirty/miss scores. Also, it doesn't seem > entirely unreasonable that someone somewhere might also want to > fine-tune the hit/miss/dirty scores so that they're some larger factor > apart from each other the standard scores are. The 10,000 limit does > not allow much wiggle room for that. > Increase it to what? cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Increase it to what?
Jeff's opinion that it could be INT_MAX without causing trouble is
a bit over-optimistic, see vacuum_delay_point():
if (VacuumCostActive && !InterruptPending &&
VacuumCostBalance >= VacuumCostLimit)
{
int msec;
msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit;
In the first place, if VacuumCostLimit is too close to INT_MAX,
it'd be possible for VacuumCostBalance (also an int) to overflow
between visits to vacuum_delay_point, wrapping around to negative
and thus missing the need to nap altogether.
In the second place, since large VacuumCostLimit implies large
VacuumCostBalance when we do get through this if-check, there's
a hazard of integer overflow in the VacuumCostDelay * VacuumCostBalance
multiplication. The final value of the msec calculation should be
easily within integer range, since VacuumCostDelay is constrained to
not be very large, but that's no help if we have intermediate overflow.
Possibly we could forestall both of those problems by changing
VacuumCostBalance to double, but that would make the cost
bookkeeping noticeably more expensive than it used to be.
I think it'd be better to keep VacuumCostBalance as int,
which would then mean we'd better limit VacuumCostLimit to no
more than say INT_MAX/2 --- call it 1 billion for the sake of
a round number.
That'd still leave us at risk of an integer overflow in the
msec-to-sleep calculation, but that calculation could be changed
to double at little price, since once we get here we're going
to sleep awhile anyway.
BTW, don't forget autovacuum_cost_limit should have the same maximum.
regards, tom lane
I wrote:
> [ worries about overflow with VacuumCostLimit approaching INT_MAX ]
Actually, now that I think a bit harder, that disquisition was silly.
In fact, I'm inclined to argue that the already-committed patch
is taking the wrong approach, and we should revert it in favor of a
different idea.
The reason is this: what we want to do is throttle VACUUM's I/O demand,
and by "throttle" I mean "gradually reduce". There is nothing gradual
about issuing a few million I/Os and then sleeping for many milliseconds;
that'll just produce spikes and valleys in the I/O demand. Ideally,
what we'd have it do is sleep for a very short interval after each I/O.
But that's not too practical, both for code-structure reasons and because
most platforms don't give us a way to so finely control the length of a
sleep. Hence the design of sleeping for awhile after every so many I/Os.
However, the current settings are predicated on the assumption that
you can't get the kernel to give you a sleep of less than circa 10ms.
That assumption is way outdated, I believe; poking around on systems
I have here, the minimum delay time using pg_usleep(1) seems to be
generally less than 100us, and frequently less than 10us, on anything
released in the last decade.
I propose therefore that instead of increasing vacuum_cost_limit,
what we ought to be doing is reducing vacuum_cost_delay by a similar
factor. And, to provide some daylight for people to reduce it even
more, we ought to arrange for it to be specifiable in microseconds
not milliseconds. There's no GUC_UNIT_US right now, but it's time.
(Perhaps we should also look into using other delay APIs, such as
nanosleep(2), where available.)
I don't have any particular objection to kicking up the maximum
value of vacuum_cost_limit by 10X or so, if anyone's hot to do that.
But that's not where we ought to be focusing our concern. And there
really is a good reason, not just nannyism, not to make that
setting huge --- it's just the wrong thing to do, as compared to
reducing vacuum_cost_delay.
regards, tom lane
On Sat, 9 Mar 2019 at 16:11, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I propose therefore that instead of increasing vacuum_cost_limit, > what we ought to be doing is reducing vacuum_cost_delay by a similar > factor. And, to provide some daylight for people to reduce it even > more, we ought to arrange for it to be specifiable in microseconds > not milliseconds. There's no GUC_UNIT_US right now, but it's time. > (Perhaps we should also look into using other delay APIs, such as > nanosleep(2), where available.) It does seem like a genuine concern that there might be too much all or nothing. It's no good being on a highspeed train if it stops at every platform. I agree that vacuum_cost_delay might not be granular enough, however. If we're going to change the vacuum_cost_delay into microseconds, then I'm a little concerned that it'll silently break existing code that sets it. Scripts that do manual off-peak vacuums are pretty common out in the wild. In an ideal world we'd just redesign the vacuum throttling to have MB/s for hit/read/dirty, and possible also WAL write rate. I'm not sure exactly how they'd cooperate together, but we could likely minimise gettimeofday() calls by sampling the time it took to process N pages, and if N pages didn't take the time we wanted them to take we could set N = Min(N * ($target_gettimeofday_sample_rate / $timetaken), 1); e.g if N was 2000 and it just took us 1 second to do 2000 pages, but we want to sleep every millisecond, then just do N *= (0.001 / 1), so the next run we only do 2 pages before checking how long we should sleep for. If we happened to process those 2 pages in 0.5 milliseconds, then N would become 4, etc. We'd just need to hard code the $target_gettimeofday_sample_rate. Probably 1 millisecond would be about right and we'd need to just guess the first value of N, but if we guess a low value, it'll be quick to correct itself after the first batch of pages. If anyone thinks that idea has any potential, then maybe it's better to leave the new vacuum_cost_limit default in place and consider redesigning this for PG13... as such a change is too late for PG12. It may also be possible to make this a vacuum rate limit in %. Say 10% would just sleep for 10x as long is it took to process the last set of pages. The problem with this is that if the server was under heavy load then auto-vacuum might crawl along, but that might be the exact opposite of what's required as it might be crawling due to inadequate vacuuming. > I don't have any particular objection to kicking up the maximum > value of vacuum_cost_limit by 10X or so, if anyone's hot to do that. > But that's not where we ought to be focusing our concern. And there > really is a good reason, not just nannyism, not to make that > setting huge --- it's just the wrong thing to do, as compared to > reducing vacuum_cost_delay. My vote is to 10x the maximum for vacuum_cost_limit and consider changing how it all works in PG13. If nothing happens before this time next year then we can consider making vacuum_cost_delay a microseconds GUC. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 3/9/19 4:28 AM, David Rowley wrote: > On Sat, 9 Mar 2019 at 16:11, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I propose therefore that instead of increasing vacuum_cost_limit, >> what we ought to be doing is reducing vacuum_cost_delay by a similar >> factor. And, to provide some daylight for people to reduce it even >> more, we ought to arrange for it to be specifiable in microseconds >> not milliseconds. There's no GUC_UNIT_US right now, but it's time. >> (Perhaps we should also look into using other delay APIs, such as >> nanosleep(2), where available.) > It does seem like a genuine concern that there might be too much all > or nothing. It's no good being on a highspeed train if it stops at > every platform. > > I agree that vacuum_cost_delay might not be granular enough, however. > If we're going to change the vacuum_cost_delay into microseconds, then > I'm a little concerned that it'll silently break existing code that > sets it. Scripts that do manual off-peak vacuums are pretty common > out in the wild. Maybe we could leave the default units as msec but store it and allow specifying as usec. Not sure how well the GUC mechanism would cope with that. [other good ideas] >> I don't have any particular objection to kicking up the maximum >> value of vacuum_cost_limit by 10X or so, if anyone's hot to do that. >> But that's not where we ought to be focusing our concern. And there >> really is a good reason, not just nannyism, not to make that >> setting huge --- it's just the wrong thing to do, as compared to >> reducing vacuum_cost_delay. > My vote is to 10x the maximum for vacuum_cost_limit and consider > changing how it all works in PG13. If nothing happens before this > time next year then we can consider making vacuum_cost_delay a > microseconds GUC. > +1. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
> I agree that vacuum_cost_delay might not be granular enough, however.
> If we're going to change the vacuum_cost_delay into microseconds, then
> I'm a little concerned that it'll silently break existing code that
> sets it. Scripts that do manual off-peak vacuums are pretty common
> out in the wild.
True. Perhaps we could keep the units as ms but make it a float?
Not sure if the "units" logic can cope though.
> My vote is to 10x the maximum for vacuum_cost_limit and consider
> changing how it all works in PG13. If nothing happens before this
> time next year then we can consider making vacuum_cost_delay a
> microseconds GUC.
I'm not really happy with the idea of changing the defaults in this area
and then changing them again next year. That's going to lead to a lot
of confusion, and a mess for people who may have changed (some) of
the settings manually.
regards, tom lane
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 3/9/19 4:28 AM, David Rowley wrote:
>> I agree that vacuum_cost_delay might not be granular enough, however.
>> If we're going to change the vacuum_cost_delay into microseconds, then
>> I'm a little concerned that it'll silently break existing code that
>> sets it. Scripts that do manual off-peak vacuums are pretty common
>> out in the wild.
> Maybe we could leave the default units as msec but store it and allow
> specifying as usec. Not sure how well the GUC mechanism would cope with
> that.
I took a quick look at that and I'm afraid it'd be a mess. GUC doesn't
really distinguish between a variable's storage unit, its default input
unit, or its default output unit (as seen in e.g. pg_settings). Perhaps
we could split those into two or three distinct concepts, but it seems
complicated and bug-prone. Also I think we'd still be forced into
making obviously-incompatible changes in what pg_settings shows for
this variable, since what it shows right now is integer ms. That last
isn't a deal-breaker perhaps, but 100% compatibility isn't going to
happen this way.
The idea of converting vacuum_cost_delay into a float variable, while
keeping its native unit as ms, seems probably more feasible from a
compatibility standpoint. There are two sub-possibilities:
1. Just do that and lose units support for the variable. I don't
think this is totally unreasonable, because up to now ms is the
*only* workable unit for it:
regression=# set vacuum_cost_delay = '1s';
ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
Still, it'd mean that anyone who'd been explicitly setting it with
an "ms" qualifier would have to change their postgresql.conf entry.
2. Add support for units for float variables, too. I don't think
this'd be a huge amount of work, and we'd surely have other uses
for it in the long run.
I'm inclined to go look into #2. Anybody think this is a bad idea?
regards, tom lane
On 3/9/19 12:55 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 3/9/19 4:28 AM, David Rowley wrote: >>> I agree that vacuum_cost_delay might not be granular enough, however. >>> If we're going to change the vacuum_cost_delay into microseconds, then >>> I'm a little concerned that it'll silently break existing code that >>> sets it. Scripts that do manual off-peak vacuums are pretty common >>> out in the wild. >> Maybe we could leave the default units as msec but store it and allow >> specifying as usec. Not sure how well the GUC mechanism would cope with >> that. > I took a quick look at that and I'm afraid it'd be a mess. GUC doesn't > really distinguish between a variable's storage unit, its default input > unit, or its default output unit (as seen in e.g. pg_settings). Perhaps > we could split those into two or three distinct concepts, but it seems > complicated and bug-prone. Also I think we'd still be forced into > making obviously-incompatible changes in what pg_settings shows for > this variable, since what it shows right now is integer ms. That last > isn't a deal-breaker perhaps, but 100% compatibility isn't going to > happen this way. > > The idea of converting vacuum_cost_delay into a float variable, while > keeping its native unit as ms, seems probably more feasible from a > compatibility standpoint. There are two sub-possibilities: > > 1. Just do that and lose units support for the variable. I don't > think this is totally unreasonable, because up to now ms is the > *only* workable unit for it: > > regression=# set vacuum_cost_delay = '1s'; > ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100) > > Still, it'd mean that anyone who'd been explicitly setting it with > an "ms" qualifier would have to change their postgresql.conf entry. > > 2. Add support for units for float variables, too. I don't think > this'd be a huge amount of work, and we'd surely have other uses > for it in the long run. > > I'm inclined to go look into #2. Anybody think this is a bad idea? > > Sounds good to me, seems much more likely to be future-proof. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Mar 9, 2019 at 7:58 PM Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > > On 3/9/19 12:55 PM, Tom Lane wrote: > >> Maybe we could leave the default units as msec but store it and allow > >> specifying as usec. Not sure how well the GUC mechanism would cope with > >> that. > > I took a quick look at that and I'm afraid it'd be a mess. GUC doesn't > > really distinguish between a variable's storage unit, its default input > > unit, or its default output unit (as seen in e.g. pg_settings). Perhaps > > we could split those into two or three distinct concepts, but it seems > > complicated and bug-prone. Also I think we'd still be forced into > > making obviously-incompatible changes in what pg_settings shows for > > this variable, since what it shows right now is integer ms. That last > > isn't a deal-breaker perhaps, but 100% compatibility isn't going to > > happen this way. > > > > The idea of converting vacuum_cost_delay into a float variable, while > > keeping its native unit as ms, seems probably more feasible from a > > compatibility standpoint. There are two sub-possibilities: > > > > 1. Just do that and lose units support for the variable. I don't > > think this is totally unreasonable, because up to now ms is the > > *only* workable unit for it: > > > > regression=# set vacuum_cost_delay = '1s'; > > ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100) > > > > Still, it'd mean that anyone who'd been explicitly setting it with > > an "ms" qualifier would have to change their postgresql.conf entry. > > > > 2. Add support for units for float variables, too. I don't think > > this'd be a huge amount of work, and we'd surely have other uses > > for it in the long run. > > > > I'm inclined to go look into #2. Anybody think this is a bad idea? > > Sounds good to me, seems much more likely to be future-proof. Agreed.
On 10/03/2019 06:55, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 3/9/19 4:28 AM, David Rowley wrote: >>> I agree that vacuum_cost_delay might not be granular enough, however. >>> If we're going to change the vacuum_cost_delay into microseconds, then >>> I'm a little concerned that it'll silently break existing code that >>> sets it. Scripts that do manual off-peak vacuums are pretty common >>> out in the wild. >> Maybe we could leave the default units as msec but store it and allow >> specifying as usec. Not sure how well the GUC mechanism would cope with >> that. > I took a quick look at that and I'm afraid it'd be a mess. GUC doesn't > really distinguish between a variable's storage unit, its default input > unit, or its default output unit (as seen in e.g. pg_settings). Perhaps > we could split those into two or three distinct concepts, but it seems > complicated and bug-prone. Also I think we'd still be forced into > making obviously-incompatible changes in what pg_settings shows for > this variable, since what it shows right now is integer ms. That last > isn't a deal-breaker perhaps, but 100% compatibility isn't going to > happen this way. > > The idea of converting vacuum_cost_delay into a float variable, while > keeping its native unit as ms, seems probably more feasible from a > compatibility standpoint. There are two sub-possibilities: > > 1. Just do that and lose units support for the variable. I don't > think this is totally unreasonable, because up to now ms is the > *only* workable unit for it: > > regression=# set vacuum_cost_delay = '1s'; > ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100) > > Still, it'd mean that anyone who'd been explicitly setting it with > an "ms" qualifier would have to change their postgresql.conf entry. > > 2. Add support for units for float variables, too. I don't think > this'd be a huge amount of work, and we'd surely have other uses > for it in the long run. > > I'm inclined to go look into #2. Anybody think this is a bad idea? > > regards, tom lane > Hope about keeping the default unit of ms, but converting it to a 'double' for input, but storing it as int (or long?) number of nanoseconds. Gives finer grain of control withouthaving to specify a unit, while still allowing calculations to be fast? Cheers, Gavin
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 9, 2019 at 7:58 PM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>> On 3/9/19 12:55 PM, Tom Lane wrote:
>>> The idea of converting vacuum_cost_delay into a float variable, while
>>> keeping its native unit as ms, seems probably more feasible from a
>>> compatibility standpoint. There are two sub-possibilities:
>>> ...
>>> 2. Add support for units for float variables, too. I don't think
>>> this'd be a huge amount of work, and we'd surely have other uses
>>> for it in the long run.
>>> ...
>>> I'm inclined to go look into #2. Anybody think this is a bad idea?
>> Sounds good to me, seems much more likely to be future-proof.
> Agreed.
I tried this, and it seems to work pretty well. The first of the two
attached patches just teaches guc.c to support units for float values,
incidentally allowing "us" as an input unit for time-based GUCs.
The second converts [autovacuum_]cost_delay to float GUCs, and changes
the default value for autovacuum_cost_delay from 20ms to 2ms.
We'd want to revert the previous patch that changed the default value
of vacuum_cost_limit, else this means a 100x not 10x change in the
default autovac speed ... but I've not included that in this patch.
Some notes:
1. I hadn't quite absorbed until doing this that we'd need a catversion
bump because of format change in StdRdOptions. Since this isn't proposed
for back-patching, that doesn't seem problematic.
2. It's always bugged me that we don't allow fractional unit
specifications, say "0.1GB", even for GUCs that are integers underneath.
That would be a simple additional change on top of this, but I didn't
do it here.
3. I noticed that parse_real doesn't reject infinity or NaN values
for float GUCs. This seems like a bug, maybe even a back-patchable one;
I doubt the planner will react sanely to SET seq_page_cost TO 'NaN'
for instance. I didn't do anything about that here either.
4. I've not done anything here about increasing the max value of
[autovacuum_]vacuum_cost_limit. I have no objection to kicking that
up 10x or so if somebody wants to do the work, but I'm not sure it's
very useful given this patch.
Comments?
regards, tom lane
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index cdf1f4a..8ca36ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1198,7 +1198,7 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
{
relopt_real *optreal = (relopt_real *) option->gen;
- parsed = parse_real(value, &option->values.real_val);
+ parsed = parse_real(value, &option->values.real_val, 0, NULL);
if (validate && !parsed)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bb6052a..5226733 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -755,13 +755,13 @@ const char *const config_type_names[] =
* For each supported conversion from one unit to another, we have an entry
* in the table.
*
- * To keep things simple, and to avoid intermediate-value overflows,
+ * To keep things simple, and to avoid possible roundoff error,
* conversions are never chained. There needs to be a direct conversion
* between all units (of the same type).
*
- * The conversions from each base unit must be kept in order from greatest
- * to smallest unit; convert_from_base_unit() relies on that. (The order of
- * the base units does not matter.)
+ * The conversions for each base unit must be kept in order from greatest to
+ * smallest human-friendly unit; convert_xxx_from_base_unit() rely on that.
+ * (The order of the base-unit groups does not matter.)
*/
#define MAX_UNIT_LEN 3 /* length of longest recognized unit string */
@@ -770,9 +770,7 @@ typedef struct
char unit[MAX_UNIT_LEN + 1]; /* unit, as a string, like "kB" or
* "min" */
int base_unit; /* GUC_UNIT_XXX */
- int64 multiplier; /* If positive, multiply the value with this
- * for unit -> base_unit conversion. If
- * negative, divide (with the absolute value) */
+ double multiplier; /* Factor for converting unit -> base_unit */
} unit_conversion;
/* Ensure that the constants in the tables don't overflow or underflow */
@@ -787,45 +785,40 @@ static const char *memory_units_hint = gettext_noop("Valid units for this parame
static const unit_conversion memory_unit_conversion_table[] =
{
- /*
- * TB -> bytes conversion always overflows 32-bit integer, so this always
- * produces an error. Include it nevertheless for completeness, and so
- * that you get an "out of range" error, rather than "invalid unit".
- */
- {"TB", GUC_UNIT_BYTE, INT64CONST(1024) * 1024 * 1024 * 1024},
- {"GB", GUC_UNIT_BYTE, 1024 * 1024 * 1024},
- {"MB", GUC_UNIT_BYTE, 1024 * 1024},
- {"kB", GUC_UNIT_BYTE, 1024},
- {"B", GUC_UNIT_BYTE, 1},
-
- {"TB", GUC_UNIT_KB, 1024 * 1024 * 1024},
- {"GB", GUC_UNIT_KB, 1024 * 1024},
- {"MB", GUC_UNIT_KB, 1024},
- {"kB", GUC_UNIT_KB, 1},
- {"B", GUC_UNIT_KB, -1024},
-
- {"TB", GUC_UNIT_MB, 1024 * 1024},
- {"GB", GUC_UNIT_MB, 1024},
- {"MB", GUC_UNIT_MB, 1},
- {"kB", GUC_UNIT_MB, -1024},
- {"B", GUC_UNIT_MB, -(1024 * 1024)},
-
- {"TB", GUC_UNIT_BLOCKS, (1024 * 1024 * 1024) / (BLCKSZ / 1024)},
- {"GB", GUC_UNIT_BLOCKS, (1024 * 1024) / (BLCKSZ / 1024)},
- {"MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024)},
- {"kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024)},
- {"B", GUC_UNIT_BLOCKS, -BLCKSZ},
-
- {"TB", GUC_UNIT_XBLOCKS, (1024 * 1024 * 1024) / (XLOG_BLCKSZ / 1024)},
- {"GB", GUC_UNIT_XBLOCKS, (1024 * 1024) / (XLOG_BLCKSZ / 1024)},
- {"MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024)},
- {"kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024)},
- {"B", GUC_UNIT_XBLOCKS, -XLOG_BLCKSZ},
+ {"TB", GUC_UNIT_BYTE, 1024.0 * 1024.0 * 1024.0 * 1024.0},
+ {"GB", GUC_UNIT_BYTE, 1024.0 * 1024.0 * 1024.0},
+ {"MB", GUC_UNIT_BYTE, 1024.0 * 1024.0},
+ {"kB", GUC_UNIT_BYTE, 1024.0},
+ {"B", GUC_UNIT_BYTE, 1.0},
+
+ {"TB", GUC_UNIT_KB, 1024.0 * 1024.0 * 1024.0},
+ {"GB", GUC_UNIT_KB, 1024.0 * 1024.0},
+ {"MB", GUC_UNIT_KB, 1024.0},
+ {"kB", GUC_UNIT_KB, 1.0},
+ {"B", GUC_UNIT_KB, 1.0 / 1024.0},
+
+ {"TB", GUC_UNIT_MB, 1024.0 * 1024.0},
+ {"GB", GUC_UNIT_MB, 1024.0},
+ {"MB", GUC_UNIT_MB, 1.0},
+ {"kB", GUC_UNIT_MB, 1.0 / 1024.0},
+ {"B", GUC_UNIT_MB, 1.0 / (1024.0 * 1024.0)},
+
+ {"TB", GUC_UNIT_BLOCKS, (1024.0 * 1024.0 * 1024.0) / (BLCKSZ / 1024)},
+ {"GB", GUC_UNIT_BLOCKS, (1024.0 * 1024.0) / (BLCKSZ / 1024)},
+ {"MB", GUC_UNIT_BLOCKS, 1024.0 / (BLCKSZ / 1024)},
+ {"kB", GUC_UNIT_BLOCKS, 1.0 / (BLCKSZ / 1024)},
+ {"B", GUC_UNIT_BLOCKS, 1.0 / BLCKSZ},
+
+ {"TB", GUC_UNIT_XBLOCKS, (1024.0 * 1024.0 * 1024.0) / (XLOG_BLCKSZ / 1024)},
+ {"GB", GUC_UNIT_XBLOCKS, (1024.0 * 1024.0) / (XLOG_BLCKSZ / 1024)},
+ {"MB", GUC_UNIT_XBLOCKS, 1024.0 / (XLOG_BLCKSZ / 1024)},
+ {"kB", GUC_UNIT_XBLOCKS, 1.0 / (XLOG_BLCKSZ / 1024)},
+ {"B", GUC_UNIT_XBLOCKS, 1.0 / XLOG_BLCKSZ},
{""} /* end of table marker */
};
-static const char *time_units_hint = gettext_noop("Valid units for this parameter are \"ms\", \"s\", \"min\", \"h\",
and\"d\".");
+static const char *time_units_hint = gettext_noop("Valid units for this parameter are \"us\", \"ms\", \"s\", \"min\",
\"h\",and \"d\".");
static const unit_conversion time_unit_conversion_table[] =
{
@@ -834,18 +827,21 @@ static const unit_conversion time_unit_conversion_table[] =
{"min", GUC_UNIT_MS, 1000 * 60},
{"s", GUC_UNIT_MS, 1000},
{"ms", GUC_UNIT_MS, 1},
+ {"us", GUC_UNIT_MS, 1.0 / 1000},
{"d", GUC_UNIT_S, 60 * 60 * 24},
{"h", GUC_UNIT_S, 60 * 60},
{"min", GUC_UNIT_S, 60},
{"s", GUC_UNIT_S, 1},
- {"ms", GUC_UNIT_S, -1000},
+ {"ms", GUC_UNIT_S, 1.0 / 1000},
+ {"us", GUC_UNIT_S, 1.0 / (1000 * 1000)},
{"d", GUC_UNIT_MIN, 60 * 24},
{"h", GUC_UNIT_MIN, 60},
{"min", GUC_UNIT_MIN, 1},
- {"s", GUC_UNIT_MIN, -60},
- {"ms", GUC_UNIT_MIN, -1000 * 60},
+ {"s", GUC_UNIT_MIN, 1.0 / 60},
+ {"ms", GUC_UNIT_MIN, 1.0 / (1000 * 60)},
+ {"us", GUC_UNIT_MIN, 1.0 / (1000 * 1000 * 60)},
{""} /* end of table marker */
};
@@ -5960,17 +5956,35 @@ ReportGUCOption(struct config_generic *record)
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
- * to convert from. The converted value is stored in *base_value.
+ * to convert from (there can be trailing spaces in the unit string).
+ * The converted value is stored in *base_value.
+ * It's caller's responsibility to round off the converted value as necessary
+ * and check for out-of-range.
*
* Returns true on success, false if the input unit is not recognized.
*/
static bool
-convert_to_base_unit(int64 value, const char *unit,
- int base_unit, int64 *base_value)
+convert_to_base_unit(double value, const char *unit,
+ int base_unit, double *base_value)
{
+ char unitstr[MAX_UNIT_LEN + 1];
+ int unitlen;
const unit_conversion *table;
int i;
+ /* extract unit string to compare to table entries */
+ unitlen = 0;
+ while (*unit != '\0' && !isspace((unsigned char) *unit) &&
+ unitlen < MAX_UNIT_LEN)
+ unitstr[unitlen++] = *(unit++);
+ unitstr[unitlen] = '\0';
+ /* allow whitespace after unit */
+ while (isspace((unsigned char) *unit))
+ unit++;
+ if (*unit != '\0')
+ return false; /* unit too long, or garbage after it */
+
+ /* now search the appropriate table */
if (base_unit & GUC_UNIT_MEMORY)
table = memory_unit_conversion_table;
else
@@ -5979,12 +5993,9 @@ convert_to_base_unit(int64 value, const char *unit,
for (i = 0; *table[i].unit; i++)
{
if (base_unit == table[i].base_unit &&
- strcmp(unit, table[i].unit) == 0)
+ strcmp(unitstr, table[i].unit) == 0)
{
- if (table[i].multiplier < 0)
- *base_value = value / (-table[i].multiplier);
- else
- *base_value = value * table[i].multiplier;
+ *base_value = value * table[i].multiplier;
return true;
}
}
@@ -5992,14 +6003,15 @@ convert_to_base_unit(int64 value, const char *unit,
}
/*
- * Convert a value in some base unit to a human-friendly unit. The output
- * unit is chosen so that it's the greatest unit that can represent the value
- * without loss. For example, if the base unit is GUC_UNIT_KB, 1024 is
- * converted to 1 MB, but 1025 is represented as 1025 kB.
+ * Convert an integer value in some base unit to a human-friendly unit.
+ *
+ * The output unit is chosen so that it's the greatest unit that can represent
+ * the value without loss. For example, if the base unit is GUC_UNIT_KB, 1024
+ * is converted to 1 MB, but 1025 is represented as 1025 kB.
*/
static void
-convert_from_base_unit(int64 base_value, int base_unit,
- int64 *value, const char **unit)
+convert_int_from_base_unit(int64 base_value, int base_unit,
+ int64 *value, const char **unit)
{
const unit_conversion *table;
int i;
@@ -6020,15 +6032,15 @@ convert_from_base_unit(int64 base_value, int base_unit,
* assume that the conversions for each base unit are ordered from
* greatest unit to the smallest!
*/
- if (table[i].multiplier < 0)
+ if (table[i].multiplier <= 1.0)
{
- *value = base_value * (-table[i].multiplier);
+ *value = rint(base_value / table[i].multiplier);
*unit = table[i].unit;
break;
}
- else if (base_value % table[i].multiplier == 0)
+ else if (base_value % (int64) table[i].multiplier == 0)
{
- *value = base_value / table[i].multiplier;
+ *value = rint(base_value / table[i].multiplier);
*unit = table[i].unit;
break;
}
@@ -6038,6 +6050,44 @@ convert_from_base_unit(int64 base_value, int base_unit,
Assert(*unit != NULL);
}
+/*
+ * Convert a floating-point value in some base unit to a human-friendly unit.
+ *
+ * Same as above, except we have to do the math a bit differently, and
+ * there's a possibility that we don't find any exact divisor.
+ */
+static void
+convert_real_from_base_unit(double base_value, int base_unit,
+ double *value, const char **unit)
+{
+ const unit_conversion *table;
+ int i;
+
+ *unit = NULL;
+
+ if (base_unit & GUC_UNIT_MEMORY)
+ table = memory_unit_conversion_table;
+ else
+ table = time_unit_conversion_table;
+
+ for (i = 0; *table[i].unit; i++)
+ {
+ if (base_unit == table[i].base_unit)
+ {
+ /*
+ * Accept the first conversion that divides the value evenly; or
+ * if there is none, use the smallest (last) target unit.
+ */
+ *value = base_value / table[i].multiplier;
+ *unit = table[i].unit;
+ if (*value == rint(*value))
+ break;
+ }
+ }
+
+ Assert(*unit != NULL);
+}
+
/*
* Try to parse value as an integer. The accepted formats are the
@@ -6082,26 +6132,14 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
/* Handle possible unit */
if (*endptr != '\0')
{
- char unit[MAX_UNIT_LEN + 1];
- int unitlen;
- bool converted = false;
+ double cval;
if ((flags & GUC_UNIT) == 0)
return false; /* this setting does not accept a unit */
- unitlen = 0;
- while (*endptr != '\0' && !isspace((unsigned char) *endptr) &&
- unitlen < MAX_UNIT_LEN)
- unit[unitlen++] = *(endptr++);
- unit[unitlen] = '\0';
- /* allow whitespace after unit */
- while (isspace((unsigned char) *endptr))
- endptr++;
-
- if (*endptr == '\0')
- converted = convert_to_base_unit(val, unit, (flags & GUC_UNIT),
- &val);
- if (!converted)
+ if (!convert_to_base_unit((double) val,
+ endptr, (flags & GUC_UNIT),
+ &cval))
{
/* invalid unit, or garbage after the unit; set hint and fail. */
if (hintmsg)
@@ -6114,13 +6152,15 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
return false;
}
- /* Check for overflow due to units conversion */
- if (val != (int64) ((int32) val))
+ /* Round to int, then check for overflow due to units conversion */
+ cval = rint(cval);
+ if (cval > INT_MAX || cval < INT_MIN)
{
if (hintmsg)
*hintmsg = gettext_noop("Value exceeds integer range.");
return false;
}
+ val = (int64) cval;
}
if (result)
@@ -6136,24 +6176,47 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
* If okay and result is not NULL, return the value in *result.
*/
bool
-parse_real(const char *value, double *result)
+parse_real(const char *value, double *result, int flags, const char **hintmsg)
{
double val;
char *endptr;
+ /* To suppress compiler warnings, always set output params */
if (result)
- *result = 0; /* suppress compiler warning */
+ *result = 0;
+ if (hintmsg)
+ *hintmsg = NULL;
errno = 0;
val = strtod(value, &endptr);
if (endptr == value || errno == ERANGE)
return false;
- /* allow whitespace after number */
+ /* allow whitespace between number and unit */
while (isspace((unsigned char) *endptr))
endptr++;
+
+ /* Handle possible unit */
if (*endptr != '\0')
- return false;
+ {
+ if ((flags & GUC_UNIT) == 0)
+ return false; /* this setting does not accept a unit */
+
+ if (!convert_to_base_unit(val,
+ endptr, (flags & GUC_UNIT),
+ &val))
+ {
+ /* invalid unit, or garbage after the unit; set hint and fail. */
+ if (hintmsg)
+ {
+ if (flags & GUC_UNIT_MEMORY)
+ *hintmsg = memory_units_hint;
+ else
+ *hintmsg = time_units_hint;
+ }
+ return false;
+ }
+ }
if (result)
*result = val;
@@ -6336,13 +6399,16 @@ parse_and_validate_value(struct config_generic *record,
case PGC_REAL:
{
struct config_real *conf = (struct config_real *) record;
+ const char *hintmsg;
- if (!parse_real(value, &newval->realval))
+ if (!parse_real(value, &newval->realval,
+ conf->gen.flags, &hintmsg))
{
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("parameter \"%s\" requires a numeric value",
- name)));
+ errmsg("invalid value for parameter \"%s\": \"%s\"",
+ name, value),
+ hintmsg ? errhint("%s", _(hintmsg)) : 0));
return false;
}
@@ -8687,48 +8753,44 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[1] = _ShowOption(conf, false);
/* unit */
- if (conf->vartype == PGC_INT)
+ switch (conf->flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME))
{
- switch (conf->flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME))
- {
- case GUC_UNIT_BYTE:
- values[2] = "B";
- break;
- case GUC_UNIT_KB:
- values[2] = "kB";
- break;
- case GUC_UNIT_MB:
- values[2] = "MB";
- break;
- case GUC_UNIT_BLOCKS:
- snprintf(buffer, sizeof(buffer), "%dkB", BLCKSZ / 1024);
- values[2] = pstrdup(buffer);
- break;
- case GUC_UNIT_XBLOCKS:
- snprintf(buffer, sizeof(buffer), "%dkB", XLOG_BLCKSZ / 1024);
- values[2] = pstrdup(buffer);
- break;
- case GUC_UNIT_MS:
- values[2] = "ms";
- break;
- case GUC_UNIT_S:
- values[2] = "s";
- break;
- case GUC_UNIT_MIN:
- values[2] = "min";
- break;
- case 0:
- values[2] = NULL;
- break;
- default:
- elog(ERROR, "unrecognized GUC units value: %d",
- conf->flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME));
- values[2] = NULL;
- break;
- }
+ case 0:
+ /* no unit specified */
+ values[2] = NULL;
+ break;
+ case GUC_UNIT_BYTE:
+ values[2] = "B";
+ break;
+ case GUC_UNIT_KB:
+ values[2] = "kB";
+ break;
+ case GUC_UNIT_MB:
+ values[2] = "MB";
+ break;
+ case GUC_UNIT_BLOCKS:
+ snprintf(buffer, sizeof(buffer), "%dkB", BLCKSZ / 1024);
+ values[2] = pstrdup(buffer);
+ break;
+ case GUC_UNIT_XBLOCKS:
+ snprintf(buffer, sizeof(buffer), "%dkB", XLOG_BLCKSZ / 1024);
+ values[2] = pstrdup(buffer);
+ break;
+ case GUC_UNIT_MS:
+ values[2] = "ms";
+ break;
+ case GUC_UNIT_S:
+ values[2] = "s";
+ break;
+ case GUC_UNIT_MIN:
+ values[2] = "min";
+ break;
+ default:
+ elog(ERROR, "unrecognized GUC units value: %d",
+ conf->flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME));
+ values[2] = NULL;
+ break;
}
- else
- values[2] = NULL;
/* group */
values[3] = _(config_group_names[conf->group]);
@@ -9257,10 +9319,9 @@ _ShowOption(struct config_generic *record, bool use_units)
const char *unit;
if (use_units && result > 0 && (record->flags & GUC_UNIT))
- {
- convert_from_base_unit(result, record->flags & GUC_UNIT,
- &result, &unit);
- }
+ convert_int_from_base_unit(result,
+ record->flags & GUC_UNIT,
+ &result, &unit);
else
unit = "";
@@ -9279,8 +9340,18 @@ _ShowOption(struct config_generic *record, bool use_units)
val = conf->show_hook();
else
{
- snprintf(buffer, sizeof(buffer), "%g",
- *conf->variable);
+ double result = *conf->variable;
+ const char *unit;
+
+ if (use_units && result > 0 && (record->flags & GUC_UNIT))
+ convert_real_from_base_unit(result,
+ record->flags & GUC_UNIT,
+ &result, &unit);
+ else
+ unit = "";
+
+ snprintf(buffer, sizeof(buffer), "%g%s",
+ result, unit);
val = buffer;
}
}
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index c07e7b9..2712a77 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -361,7 +361,8 @@ extern void BeginReportingGUCOptions(void);
extern void ParseLongOption(const char *string, char **name, char **value);
extern bool parse_int(const char *value, int *result, int flags,
const char **hintmsg);
-extern bool parse_real(const char *value, double *result);
+extern bool parse_real(const char *value, double *result, int flags,
+ const char **hintmsg);
extern int set_config_option(const char *name, const char *value,
GucContext context, GucSource source,
GucAction action, bool changeVal, int elevel,
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7bbe8f5..9618aca 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1845,7 +1845,7 @@ include_dir 'conf.d'
<variablelist>
<varlistentry id="guc-vacuum-cost-delay" xreflabel="vacuum_cost_delay">
- <term><varname>vacuum_cost_delay</varname> (<type>integer</type>)
+ <term><varname>vacuum_cost_delay</varname> (<type>floating point</type>)
<indexterm>
<primary><varname>vacuum_cost_delay</varname> configuration parameter</primary>
</indexterm>
@@ -1856,18 +1856,19 @@ include_dir 'conf.d'
when the cost limit has been exceeded.
The default value is zero, which disables the cost-based vacuum
delay feature. Positive values enable cost-based vacuuming.
- Note that on many systems, the effective resolution
- of sleep delays is 10 milliseconds; setting
- <varname>vacuum_cost_delay</varname> to a value that is
- not a multiple of 10 might have the same results as setting it
- to the next higher multiple of 10.
</para>
<para>
When using cost-based vacuuming, appropriate values for
<varname>vacuum_cost_delay</varname> are usually quite small, perhaps
- 10 or 20 milliseconds. Adjusting vacuum's resource consumption
- is best done by changing the other vacuum cost parameters.
+ less than 1 millisecond. While <varname>vacuum_cost_delay</varname>
+ can be set to fractional-millisecond values, such delays may not be
+ measured accurately on older platforms. On such platforms,
+ increasing <command>VACUUM</command>'s throttled resource consumption
+ above what you get at 1ms will require changing the other vacuum cost
+ parameters. You should, nonetheless,
+ keep <varname>vacuum_cost_delay</varname> as small as your platform
+ will consistently measure; large delays are not helpful.
</para>
</listitem>
</varlistentry>
@@ -7020,7 +7021,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</varlistentry>
<varlistentry id="guc-autovacuum-vacuum-cost-delay" xreflabel="autovacuum_vacuum_cost_delay">
- <term><varname>autovacuum_vacuum_cost_delay</varname> (<type>integer</type>)
+ <term><varname>autovacuum_vacuum_cost_delay</varname> (<type>floating point</type>)
<indexterm>
<primary><varname>autovacuum_vacuum_cost_delay</varname> configuration parameter</primary>
</indexterm>
@@ -7030,7 +7031,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
Specifies the cost delay value that will be used in automatic
<command>VACUUM</command> operations. If -1 is specified, the regular
<xref linkend="guc-vacuum-cost-delay"/> value will be used.
- The default value is 20 milliseconds.
+ The default value is 2 milliseconds.
This parameter can only be set in the <filename>postgresql.conf</filename>
file or on the server command line;
but the setting can be overridden for individual tables by
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 22dbc07..e94fe2c 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1385,7 +1385,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</varlistentry>
<varlistentry>
- <term><literal>autovacuum_vacuum_cost_delay</literal>, <literal>toast.autovacuum_vacuum_cost_delay</literal>
(<type>integer</type>)</term>
+ <term><literal>autovacuum_vacuum_cost_delay</literal>, <literal>toast.autovacuum_vacuum_cost_delay</literal>
(<type>floatingpoint</type>)</term>
<listitem>
<para>
Per-table value for <xref linkend="guc-autovacuum-vacuum-cost-delay"/>
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8ca36ab..3b0b138 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1359,8 +1359,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
{"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
- {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_cost_delay)},
{"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_cost_limit)},
{"autovacuum_freeze_min_age", RELOPT_TYPE_INT,
@@ -1379,6 +1377,8 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, log_min_duration)},
{"toast_tuple_target", RELOPT_TYPE_INT,
offsetof(StdRdOptions, toast_tuple_target)},
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL,
+ offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_cost_delay)},
{"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_scale_factor)},
{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index e91df21..da13a5a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1834,13 +1834,13 @@ vacuum_delay_point(void)
if (VacuumCostActive && !InterruptPending &&
VacuumCostBalance >= VacuumCostLimit)
{
- int msec;
+ double msec;
msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit;
if (msec > VacuumCostDelay * 4)
msec = VacuumCostDelay * 4;
- pg_usleep(msec * 1000L);
+ pg_usleep((long) (msec * 1000));
VacuumCostBalance = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 347f91e..e9fe0a6 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -120,7 +120,7 @@ double autovacuum_anl_scale;
int autovacuum_freeze_max_age;
int autovacuum_multixact_freeze_max_age;
-int autovacuum_vac_cost_delay;
+double autovacuum_vac_cost_delay;
int autovacuum_vac_cost_limit;
int Log_autovacuum_min_duration = -1;
@@ -189,7 +189,7 @@ typedef struct autovac_table
Oid at_relid;
int at_vacoptions; /* bitmask of VacuumOption */
VacuumParams at_params;
- int at_vacuum_cost_delay;
+ double at_vacuum_cost_delay;
int at_vacuum_cost_limit;
bool at_dobalance;
bool at_sharedrel;
@@ -225,7 +225,7 @@ typedef struct WorkerInfoData
TimestampTz wi_launchtime;
bool wi_dobalance;
bool wi_sharedrel;
- int wi_cost_delay;
+ double wi_cost_delay;
int wi_cost_limit;
int wi_cost_limit_base;
} WorkerInfoData;
@@ -1785,7 +1785,7 @@ autovac_balance_cost(void)
*/
int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
autovacuum_vac_cost_limit : VacuumCostLimit);
- int vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
+ double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
autovacuum_vac_cost_delay : VacuumCostDelay);
double cost_total;
double cost_avail;
@@ -1840,7 +1840,7 @@ autovac_balance_cost(void)
}
if (worker->wi_proc != NULL)
- elog(DEBUG2, "autovac_balance_cost(pid=%u db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d,
cost_delay=%d)",
+ elog(DEBUG2, "autovac_balance_cost(pid=%u db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d,
cost_delay=%g)",
worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid,
worker->wi_dobalance ? "yes" : "no",
worker->wi_cost_limit, worker->wi_cost_limit_base,
@@ -2302,7 +2302,7 @@ do_autovacuum(void)
autovac_table *tab;
bool isshared;
bool skipit;
- int stdVacuumCostDelay;
+ double stdVacuumCostDelay;
int stdVacuumCostLimit;
dlist_iter iter;
@@ -2831,7 +2831,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
int multixact_freeze_min_age;
int multixact_freeze_table_age;
int vac_cost_limit;
- int vac_cost_delay;
+ double vac_cost_delay;
int log_min_duration;
/*
@@ -2993,7 +2993,7 @@ relation_needs_vacanalyze(Oid relid,
* table), or the autovacuum GUC variables.
*/
- /* -1 in autovac setting means use plain vacuum_cost_delay */
+ /* -1 in autovac setting means use plain vacuum_scale_factor */
vac_scale_factor = (relopts && relopts->vacuum_scale_factor >= 0)
? relopts->vacuum_scale_factor
: autovacuum_vac_scale;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index a6ce184..6d1e94f 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -138,7 +138,7 @@ int VacuumCostPageHit = 1; /* GUC parameters for vacuum */
int VacuumCostPageMiss = 10;
int VacuumCostPageDirty = 20;
int VacuumCostLimit = 2000;
-int VacuumCostDelay = 0;
+double VacuumCostDelay = 0;
int VacuumPageHit = 0;
int VacuumPageMiss = 0;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5226733..d219b7d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2270,28 +2270,6 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"vacuum_cost_delay", PGC_USERSET, RESOURCES_VACUUM_DELAY,
- gettext_noop("Vacuum cost delay in milliseconds."),
- NULL,
- GUC_UNIT_MS
- },
- &VacuumCostDelay,
- 0, 0, 100,
- NULL, NULL, NULL
- },
-
- {
- {"autovacuum_vacuum_cost_delay", PGC_SIGHUP, AUTOVACUUM,
- gettext_noop("Vacuum cost delay in milliseconds, for autovacuum."),
- NULL,
- GUC_UNIT_MS
- },
- &autovacuum_vac_cost_delay,
- 20, -1, 100,
- NULL, NULL, NULL
- },
-
- {
{"autovacuum_vacuum_cost_limit", PGC_SIGHUP, AUTOVACUUM,
gettext_noop("Vacuum cost amount available before napping, for autovacuum."),
NULL
@@ -3317,6 +3295,28 @@ static struct config_real ConfigureNamesReal[] =
},
{
+ {"vacuum_cost_delay", PGC_USERSET, RESOURCES_VACUUM_DELAY,
+ gettext_noop("Vacuum cost delay in milliseconds."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &VacuumCostDelay,
+ 0, 0, 100,
+ NULL, NULL, NULL
+ },
+
+ {
+ {"autovacuum_vacuum_cost_delay", PGC_SIGHUP, AUTOVACUUM,
+ gettext_noop("Vacuum cost delay in milliseconds, for autovacuum."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &autovacuum_vac_cost_delay,
+ 2, -1, 100,
+ NULL, NULL, NULL
+ },
+
+ {
{"autovacuum_vacuum_scale_factor", PGC_SIGHUP, AUTOVACUUM,
gettext_noop("Number of tuple updates or deletes prior to vacuum as a fraction of reltuples."),
NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 99f1666..acd7daf 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -155,7 +155,7 @@
# - Cost-Based Vacuum Delay -
-#vacuum_cost_delay = 0 # 0-100 milliseconds
+#vacuum_cost_delay = 0 # 0-100 milliseconds (0 disables)
#vacuum_cost_page_hit = 1 # 0-10000 credits
#vacuum_cost_page_miss = 10 # 0-10000 credits
#vacuum_cost_page_dirty = 20 # 0-10000 credits
@@ -589,7 +589,7 @@
#autovacuum_multixact_freeze_max_age = 400000000 # maximum multixact age
# before forced vacuum
# (change requires restart)
-#autovacuum_vacuum_cost_delay = 20ms # default vacuum cost delay for
+#autovacuum_vacuum_cost_delay = 2ms # default vacuum cost delay for
# autovacuum, in milliseconds;
# -1 means use vacuum_cost_delay
#autovacuum_vacuum_cost_limit = -1 # default vacuum cost limit for
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index c9e3500..b677c7e 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -250,7 +250,7 @@ extern int VacuumCostPageHit;
extern int VacuumCostPageMiss;
extern int VacuumCostPageDirty;
extern int VacuumCostLimit;
-extern int VacuumCostDelay;
+extern double VacuumCostDelay;
extern int VacuumPageHit;
extern int VacuumPageMiss;
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index 79e99f0..722ef1c 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -37,7 +37,7 @@ extern int autovacuum_anl_thresh;
extern double autovacuum_anl_scale;
extern int autovacuum_freeze_max_age;
extern int autovacuum_multixact_freeze_max_age;
-extern int autovacuum_vac_cost_delay;
+extern double autovacuum_vac_cost_delay;
extern int autovacuum_vac_cost_limit;
/* autovacuum launcher PID, only valid when worker is shutting down */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 9d805ca..5402851 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -243,7 +243,6 @@ typedef struct AutoVacOpts
bool enabled;
int vacuum_threshold;
int analyze_threshold;
- int vacuum_cost_delay;
int vacuum_cost_limit;
int freeze_min_age;
int freeze_max_age;
@@ -252,6 +251,7 @@ typedef struct AutoVacOpts
int multixact_freeze_max_age;
int multixact_freeze_table_age;
int log_min_duration;
+ float8 vacuum_cost_delay;
float8 vacuum_scale_factor;
float8 analyze_scale_factor;
} AutoVacOpts;
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index b0d7351..aa5fe58 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -149,11 +149,11 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
(1 row)
SAVEPOINT first_sp;
-SET vacuum_cost_delay TO 80;
+SET vacuum_cost_delay TO 80.1;
SHOW vacuum_cost_delay;
vacuum_cost_delay
-------------------
- 80ms
+ 80100us
(1 row)
SET datestyle = 'German, DMY';
@@ -183,7 +183,7 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
(1 row)
SAVEPOINT second_sp;
-SET vacuum_cost_delay TO 90;
+SET vacuum_cost_delay TO '900us';
SET datestyle = 'SQL, YMD';
SHOW datestyle;
DateStyle
@@ -222,7 +222,7 @@ ROLLBACK TO third_sp;
SHOW vacuum_cost_delay;
vacuum_cost_delay
-------------------
- 90ms
+ 900us
(1 row)
SHOW datestyle;
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 3b854ac..4fd8dc3 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -47,7 +47,7 @@ SET datestyle = 'MDY';
SHOW datestyle;
SELECT '2006-08-13 12:34:56'::timestamptz;
SAVEPOINT first_sp;
-SET vacuum_cost_delay TO 80;
+SET vacuum_cost_delay TO 80.1;
SHOW vacuum_cost_delay;
SET datestyle = 'German, DMY';
SHOW datestyle;
@@ -56,7 +56,7 @@ ROLLBACK TO first_sp;
SHOW datestyle;
SELECT '2006-08-13 12:34:56'::timestamptz;
SAVEPOINT second_sp;
-SET vacuum_cost_delay TO 90;
+SET vacuum_cost_delay TO '900us';
SET datestyle = 'SQL, YMD';
SHOW datestyle;
SELECT '2006-08-13 12:34:56'::timestamptz;
Gavin Flower <GavinFlower@archidevsys.co.nz> writes:
> Hope about keeping the default unit of ms, but converting it to a
> 'double' for input, but storing it as int (or long?) number of
> nanoseconds. Gives finer grain of control withouthaving to specify a
> unit, while still allowing calculations to be fast?
Don't really see the point. The only places where we do any calculations
with the value are where we're about to sleep, so shaving a few nanosec
doesn't seem very interesting.
regards, tom lane
BTW ... I noticed while fooling with this that GUC's out-of-range
messages can be confusing:
regression=# set vacuum_cost_delay = '1s';
ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
One's immediate reaction to that is "I put in 1, not 1000". I think
it'd be much clearer if we included the unit we'd converted to, thus:
ERROR: 1000 ms is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
(Notice that this also implicitly tells what units the range limits
are being quoted in. We could repeat the unit name in that part,
viz "(0 .. 100 ms)", but it seems unnecessary.)
A small problem with this idea is that GUC_UNIT_[X]BLOCK variables don't
really have a natural unit name. If we follow the lead of pg_settings,
such errors would look something like
ERROR: 1000 8kB is outside the valid range for ...
I can't think of a better idea, though, and it'd still be clearer than
what happens now.
Barring objections I'll go make this happen.
regards, tom lane
On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I tried this, and it seems to work pretty well. The first of the two > attached patches just teaches guc.c to support units for float values, > incidentally allowing "us" as an input unit for time-based GUCs. Why not allowing third party extensions to declare a GUC stored in us? We need a backward-compatible format for vacuum setting, but I don't see a good reason to force external extensions to do the same, and it wouldn't require much extra work. > The second converts [autovacuum_]cost_delay to float GUCs, and changes > the default value for autovacuum_cost_delay from 20ms to 2ms. > We'd want to revert the previous patch that changed the default value > of vacuum_cost_limit, else this means a 100x not 10x change in the > default autovac speed ... but I've not included that in this patch. Otherwise everything looks good to me. BTW the patches didn't apply cleanly with git-apply, but patch -p1 didn't complain. > 2. It's always bugged me that we don't allow fractional unit > specifications, say "0.1GB", even for GUCs that are integers underneath. > That would be a simple additional change on top of this, but I didn't > do it here. It annoyed me multiple times, so +1 for making that happen.
On Sat, Mar 9, 2019 at 11:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > BTW ... I noticed while fooling with this that GUC's out-of-range > messages can be confusing: > > regression=# set vacuum_cost_delay = '1s'; > ERROR: 1000 is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100) > > One's immediate reaction to that is "I put in 1, not 1000". I think > it'd be much clearer if we included the unit we'd converted to, thus: > > ERROR: 1000 ms is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100) > > (Notice that this also implicitly tells what units the range limits > are being quoted in. I like it! > A small problem with this idea is that GUC_UNIT_[X]BLOCK variables don't > really have a natural unit name. If we follow the lead of pg_settings, > such errors would look something like > > ERROR: 1000 8kB is outside the valid range for ... > > I can't think of a better idea, though, and it'd still be clearer than > what happens now. > > Barring objections I'll go make this happen. No objection here.
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I tried this, and it seems to work pretty well. The first of the two
>> attached patches just teaches guc.c to support units for float values,
>> incidentally allowing "us" as an input unit for time-based GUCs.
> Why not allowing third party extensions to declare a GUC stored in us?
I think that adding a new base unit type (GUC_UNIT_US) is possible but
I'm disinclined to do it on the basis of zero evidence that it's needed.
Only three of the five already-known time units are allowed to be base
units (ms, s, min are but d and h aren't) so it's not like there's no
precedent for excluding this one. Anyway, such a patch would be mostly
orthogonal to what I've done here, so it should be considered on its
own merits.
(BTW, if we're expecting to have GUCs that are meant to measure only
very short time intervals, maybe it'd be more forward-looking for
their base unit to be NS not US.)
>> 2. It's always bugged me that we don't allow fractional unit
>> specifications, say "0.1GB", even for GUCs that are integers underneath.
>> That would be a simple additional change on top of this, but I didn't
>> do it here.
> It annoyed me multiple times, so +1 for making that happen.
OK, will do.
regards, tom lane
On Sun, Mar 10, 2019 at 4:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Julien Rouhaud <rjuju123@gmail.com> writes: > > On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> I tried this, and it seems to work pretty well. The first of the two > >> attached patches just teaches guc.c to support units for float values, > >> incidentally allowing "us" as an input unit for time-based GUCs. > > > Why not allowing third party extensions to declare a GUC stored in us? > > I think that adding a new base unit type (GUC_UNIT_US) is possible but > I'm disinclined to do it on the basis of zero evidence that it's needed. > Only three of the five already-known time units are allowed to be base > units (ms, s, min are but d and h aren't) so it's not like there's no > precedent for excluding this one. Anyway, such a patch would be mostly > orthogonal to what I've done here, so it should be considered on its > own merits. > (BTW, if we're expecting to have GUCs that are meant to measure only > very short time intervals, maybe it'd be more forward-looking for > their base unit to be NS not US.) That's fair. > >> 2. It's always bugged me that we don't allow fractional unit > >> specifications, say "0.1GB", even for GUCs that are integers underneath. > >> That would be a simple additional change on top of this, but I didn't > >> do it here. > > > It annoyed me multiple times, so +1 for making that happen. > > OK, will do. Thanks!
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. It's always bugged me that we don't allow fractional unit
>> specifications, say "0.1GB", even for GUCs that are integers underneath.
>> That would be a simple additional change on top of this, but I didn't
>> do it here.
> It annoyed me multiple times, so +1 for making that happen.
The first patch below does that, but I noticed that if we just do it
without any subtlety, you get results like this:
regression=# set work_mem = '30.1GB';
SET
regression=# show work_mem;
work_mem
------------
31562138kB
(1 row)
The second patch is a delta that rounds off to the next smaller unit
if there is one, producing a less noisy result:
regression=# set work_mem = '30.1GB';
SET
regression=# show work_mem;
work_mem
----------
30822MB
(1 row)
I'm not sure if that's a good idea or just overthinking the problem.
Thoughts?
regards, tom lane
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index fe17357..3b59565 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -51,14 +51,21 @@
In general, enclose the value in single quotes, doubling any single
quotes within the value. Quotes can usually be omitted if the value
is a simple number or identifier, however.
+ (Values that match a SQL keyword require quoting in some contexts.)
</para>
</listitem>
<listitem>
<para>
<emphasis>Numeric (integer and floating point):</emphasis>
- A decimal point is permitted only for floating-point parameters.
- Do not use thousands separators. Quotes are not required.
+ Numeric parameters can be specified in the customary integer and
+ floating-point formats; fractional values are rounded to the nearest
+ integer if the parameter is of type integer. Integer parameters
+ additionally accept hexadecimal input (beginning
+ with <literal>0x</literal>) and octal input (beginning
+ with <literal>0</literal>), but these formats cannot have a fraction.
+ Do not use thousands separators.
+ Quotes are not required, except for hexadecimal input.
</para>
</listitem>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fe6c6f8..d374f53 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6141,8 +6141,10 @@ get_config_unit_name(int flags)
/*
* Try to parse value as an integer. The accepted formats are the
- * usual decimal, octal, or hexadecimal formats, optionally followed by
- * a unit name if "flags" indicates a unit is allowed.
+ * usual decimal, octal, or hexadecimal formats, as well as floating-point
+ * formats (which will be rounded to integer after any units conversion).
+ * Optionally, the value can be followed by a unit name if "flags" indicates
+ * a unit is allowed.
*
* If the string parses okay, return true, else false.
* If okay and result is not NULL, return the value in *result.
@@ -6152,7 +6154,11 @@ get_config_unit_name(int flags)
bool
parse_int(const char *value, int *result, int flags, const char **hintmsg)
{
- int64 val;
+ /*
+ * We assume here that double is wide enough to represent any integer
+ * value with adequate precision.
+ */
+ double val;
char *endptr;
/* To suppress compiler warnings, always set output params */
@@ -6161,35 +6167,42 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
if (hintmsg)
*hintmsg = NULL;
- /* We assume here that int64 is at least as wide as long */
+ /*
+ * Try to parse as an integer (allowing octal or hex input). If the
+ * conversion stops at a decimal point or 'e', or overflows, re-parse as
+ * float. This should work fine as long as we have no unit names starting
+ * with 'e'. If we ever do, the test could be extended to check for a
+ * sign or digit after 'e', but for now that's unnecessary.
+ */
errno = 0;
val = strtol(value, &endptr, 0);
-
- if (endptr == value)
- return false; /* no HINT for integer syntax error */
-
- if (errno == ERANGE || val != (int64) ((int32) val))
+ if (*endptr == '.' || *endptr == 'e' || *endptr == 'E' ||
+ errno == ERANGE)
{
- if (hintmsg)
- *hintmsg = gettext_noop("Value exceeds integer range.");
- return false;
+ errno = 0;
+ val = strtod(value, &endptr);
}
- /* allow whitespace between integer and unit */
+ if (endptr == value || errno == ERANGE)
+ return false; /* no HINT for these cases */
+
+ /* reject NaN (infinities will fail range check below) */
+ if (isnan(val))
+ return false; /* treat same as syntax error; no HINT */
+
+ /* allow whitespace between number and unit */
while (isspace((unsigned char) *endptr))
endptr++;
/* Handle possible unit */
if (*endptr != '\0')
{
- double cval;
-
if ((flags & GUC_UNIT) == 0)
return false; /* this setting does not accept a unit */
- if (!convert_to_base_unit((double) val,
+ if (!convert_to_base_unit(val,
endptr, (flags & GUC_UNIT),
- &cval))
+ &val))
{
/* invalid unit, or garbage after the unit; set hint and fail. */
if (hintmsg)
@@ -6201,16 +6214,16 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
}
return false;
}
+ }
- /* Round to int, then check for overflow due to units conversion */
- cval = rint(cval);
- if (cval > INT_MAX || cval < INT_MIN)
- {
- if (hintmsg)
- *hintmsg = gettext_noop("Value exceeds integer range.");
- return false;
- }
- val = (int64) cval;
+ /* Round to int, then check for overflow */
+ val = rint(val);
+
+ if (val > INT_MAX || val < INT_MIN)
+ {
+ if (hintmsg)
+ *hintmsg = gettext_noop("Value exceeds integer range.");
+ return false;
}
if (result)
@@ -6218,10 +6231,10 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
return true;
}
-
-
/*
* Try to parse value as a floating point number in the usual format.
+ * Optionally, the value can be followed by a unit name if "flags" indicates
+ * a unit is allowed.
*
* If the string parses okay, return true, else false.
* If okay and result is not NULL, return the value in *result.
diff --git a/src/test/regress/expected/reloptions.out b/src/test/regress/expected/reloptions.out
index f90c267..5266490 100644
--- a/src/test/regress/expected/reloptions.out
+++ b/src/test/regress/expected/reloptions.out
@@ -26,8 +26,9 @@ ERROR: unrecognized parameter "not_existing_option"
CREATE TABLE reloptions_test2(i INT) WITH (not_existing_namespace.fillfactor=2);
ERROR: unrecognized parameter namespace "not_existing_namespace"
-- Fail while setting improper values
-CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=30.5);
-ERROR: invalid value for integer option "fillfactor": 30.5
+CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=-30.1);
+ERROR: value -30.1 out of bounds for option "fillfactor"
+DETAIL: Valid values are between "10" and "100".
CREATE TABLE reloptions_test2(i INT) WITH (fillfactor='string');
ERROR: invalid value for integer option "fillfactor": string
CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=true);
diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/reloptions.sql
index 44fcd8c..8551851 100644
--- a/src/test/regress/sql/reloptions.sql
+++ b/src/test/regress/sql/reloptions.sql
@@ -15,7 +15,7 @@ CREATE TABLE reloptions_test2(i INT) WITH (not_existing_option=2);
CREATE TABLE reloptions_test2(i INT) WITH (not_existing_namespace.fillfactor=2);
-- Fail while setting improper values
-CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=30.5);
+CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=-30.1);
CREATE TABLE reloptions_test2(i INT) WITH (fillfactor='string');
CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=true);
CREATE TABLE reloptions_test2(i INT) WITH (autovacuum_enabled=12);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d374f53..cdb6a61 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5995,7 +5995,19 @@ convert_to_base_unit(double value, const char *unit,
if (base_unit == table[i].base_unit &&
strcmp(unitstr, table[i].unit) == 0)
{
- *base_value = value * table[i].multiplier;
+ double cvalue = value * table[i].multiplier;
+
+ /*
+ * If the user gave a fractional value such as "30.1GB", round it
+ * off to the nearest multiple of the next smaller unit, if there
+ * is one.
+ */
+ if (*table[i + 1].unit &&
+ base_unit == table[i + 1].base_unit)
+ cvalue = rint(cvalue / table[i + 1].multiplier) *
+ table[i + 1].multiplier;
+
+ *base_value = cvalue;
return true;
}
}
On Mon, 11 Mar 2019 at 09:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The second patch is a delta that rounds off to the next smaller unit > if there is one, producing a less noisy result: > > regression=# set work_mem = '30.1GB'; > SET > regression=# show work_mem; > work_mem > ---------- > 30822MB > (1 row) > > I'm not sure if that's a good idea or just overthinking the problem. > Thoughts? I don't think you're over thinking it. I often have to look at such settings and I'm probably not unique in when I glance at 30822MB I can see that's roughly 30GB, whereas when I look at 31562138kB, I'm either counting digits or reaching for a calculator. This is going to reduce the time it takes for a human to process the pg_settings output, so I think it's a good idea. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 11, 2019 at 10:03 AM David Rowley <david.rowley@2ndquadrant.com> wrote: > > On Mon, 11 Mar 2019 at 09:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > The second patch is a delta that rounds off to the next smaller unit > > if there is one, producing a less noisy result: > > > > regression=# set work_mem = '30.1GB'; > > SET > > regression=# show work_mem; > > work_mem > > ---------- > > 30822MB > > (1 row) > > > > I'm not sure if that's a good idea or just overthinking the problem. > > Thoughts? > > I don't think you're over thinking it. I often have to look at such > settings and I'm probably not unique in when I glance at 30822MB I can > see that's roughly 30GB, whereas when I look at 31562138kB, I'm either > counting digits or reaching for a calculator. This is going to reduce > the time it takes for a human to process the pg_settings output, so I > think it's a good idea. Definitely, rounding up will spare people from wasting time to check what's the actual value.
At Mon, 11 Mar 2019 13:57:21 +0100, Julien Rouhaud <rjuju123@gmail.com> wrote in
<CAOBaU_a2tLyonOMJ62=SiDmo84Xo1fy81YA8K=B+=OtTc3sYSQ@mail.gmail.com>
> On Mon, Mar 11, 2019 at 10:03 AM David Rowley
> <david.rowley@2ndquadrant.com> wrote:
> >
> > On Mon, 11 Mar 2019 at 09:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > The second patch is a delta that rounds off to the next smaller unit
> > > if there is one, producing a less noisy result:
> > >
> > > regression=# set work_mem = '30.1GB';
> > > SET
> > > regression=# show work_mem;
> > > work_mem
> > > ----------
> > > 30822MB
> > > (1 row)
> > >
> > > I'm not sure if that's a good idea or just overthinking the problem.
> > > Thoughts?
> >
> > I don't think you're over thinking it. I often have to look at such
> > settings and I'm probably not unique in when I glance at 30822MB I can
> > see that's roughly 30GB, whereas when I look at 31562138kB, I'm either
> > counting digits or reaching for a calculator. This is going to reduce
> > the time it takes for a human to process the pg_settings output, so I
> > think it's a good idea.
>
> Definitely, rounding up will spare people from wasting time to check
> what's the actual value.
+1. I don't think it overthinking, too.
Anyone who specifies memory size in GB won't care under-MB
fraction. I don't think '0.01GB' is a sane setting but it being
10MB doesn't matter. However, I don't think that '0.1d' becoming
'2h' is reasonable. "10 times per day" is "rounded" to "12 times
per day" by that.
Is it worth showing values with at most two or three fraction
digits instead of rounding the value on setting? In the attached
PoC patch - instead of the 'roundoff-fractions-harder' patch -
shows values in the shortest exact representation.
work_mem:
31562138 => '30.1 GB'
31562137 => '31562137 kB'
'0.1GB' => '0.1 GB'
'0.01GB' => '0.01 GB'
'0.001GB' => '1049 kB'
lock_timeout:
'0.1h' => '6 min'
'90 min' => '90 min'
'120 min' => '2 h'
'0.1 d' => '0.1 d'
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d374f5372c..3ca2d6dec4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6011,7 +6011,7 @@ convert_to_base_unit(double value, const char *unit,
*/
static void
convert_int_from_base_unit(int64 base_value, int base_unit,
- int64 *value, const char **unit)
+ double *value, const char **unit, int *digits)
{
const unit_conversion *table;
int i;
@@ -6027,6 +6027,9 @@ convert_int_from_base_unit(int64 base_value, int base_unit,
{
if (base_unit == table[i].base_unit)
{
+ const double frac_digits = 2;
+ double rval;
+
/*
* Accept the first conversion that divides the value evenly. We
* assume that the conversions for each base unit are ordered from
@@ -6035,7 +6038,42 @@ convert_int_from_base_unit(int64 base_value, int base_unit,
if (table[i].multiplier <= 1.0 ||
base_value % (int64) table[i].multiplier == 0)
{
- *value = (int64) rint(base_value / table[i].multiplier);
+ *digits = 0;
+ *value = rint(base_value / table[i].multiplier);
+ *unit = table[i].unit;
+ break;
+ }
+
+ /*
+ * If base_value is exactly represented by a number with at most
+ * two fraction digits, we prefer it than lower units.
+ */
+ rval = (double)base_value / table[i].multiplier;
+ rval = rint(rval * pow(10, frac_digits)) *
+ pow(10, -frac_digits);
+
+ /*
+ * Acceptable range for rval is quite arbitrary.
+ */
+ if ((rval >= 1.0 ||
+ (table[i + 1].unit &&
+ table[i].multiplier / table[i + 1].multiplier < 1000) &&
+ (int64)rint(rval * table[i].multiplier) == base_value)
+ {
+ int frac;
+
+ /* count required fraction digits */
+ for (frac = 1 ; frac < frac_digits ; frac++)
+ {
+ if (rval * 10.0 - floor(rval * 10.0) < 0.1)
+ {
+ *digits = frac;
+ break;
+ }
+ }
+ if (frac == frac_digits)
+ *digits = frac_digits;
+ *value = rval;
*unit = table[i].unit;
break;
}
@@ -9359,18 +9397,19 @@ _ShowOption(struct config_generic *record, bool use_units)
* Use int64 arithmetic to avoid overflows in units
* conversion.
*/
- int64 result = *conf->variable;
+ double result = *conf->variable;
const char *unit;
+ int digits = 0;
if (use_units && result > 0 && (record->flags & GUC_UNIT))
- convert_int_from_base_unit(result,
+ convert_int_from_base_unit(*conf->variable,
record->flags & GUC_UNIT,
- &result, &unit);
+ &result, &unit, &digits);
else
unit = "";
- snprintf(buffer, sizeof(buffer), INT64_FORMAT "%s",
- result, unit);
+ snprintf(buffer, sizeof(buffer), "%.*f %s",
+ digits, result, unit);
val = buffer;
}
}
Sorry, I sent a wrong patch. The attached is the right one.
At Mon, 11 Mar 2019 13:57:21 +0100, Julien Rouhaud <rjuju123@gmail.com> wrote in
<CAOBaU_a2tLyonOMJ62=SiDmo84Xo1fy81YA8K=B+=OtTc3sYSQ@mail.gmail.com>
> On Mon, Mar 11, 2019 at 10:03 AM David Rowley
> <david.rowley@2ndquadrant.com> wrote:
> >
> > On Mon, 11 Mar 2019 at 09:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > The second patch is a delta that rounds off to the next smaller unit
> > > if there is one, producing a less noisy result:
> > >
> > > regression=# set work_mem = '30.1GB';
> > > SET
> > > regression=# show work_mem;
> > > work_mem
> > > ----------
> > > 30822MB
> > > (1 row)
> > >
> > > I'm not sure if that's a good idea or just overthinking the problem.
> > > Thoughts?
> >
> > I don't think you're over thinking it. I often have to look at such
> > settings and I'm probably not unique in when I glance at 30822MB I can
> > see that's roughly 30GB, whereas when I look at 31562138kB, I'm either
> > counting digits or reaching for a calculator. This is going to reduce
> > the time it takes for a human to process the pg_settings output, so I
> > think it's a good idea.
>
> Definitely, rounding up will spare people from wasting time to check
> what's the actual value.
+1. I don't think it overthinking, too.
Anyone who specifies memory size in GB won't care under-MB
fraction. I don't think '0.01GB' is a sane setting but it being
10MB doesn't matter. However, I don't think that '0.1d' becoming
'2h' is reasonable. "10 times per day" is "rounded" to "12 times
per day" by that.
Is it worth showing values with at most two or three fraction
digits instead of rounding the value on setting? In the attached
PoC patch - instead of the 'roundoff-fractions-harder' patch -
shows values in the shortest exact representation.
work_mem:
31562138 => '30.1 GB'
31562137 => '31562137 kB'
'0.1GB' => '0.1 GB'
'0.01GB' => '0.01 GB'
'0.001GB' => '1049 kB'
lock_timeout:
'0.1h' => '6 min'
'90 min' => '90 min'
'120 min' => '2 h'
'0.1 d' => '0.1 d'
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d374f5372c..21e0807728 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6005,16 +6005,19 @@ convert_to_base_unit(double value, const char *unit,
/*
* Convert an integer value in some base unit to a human-friendly unit.
*
- * The output unit is chosen so that it's the greatest unit that can represent
- * the value without loss. For example, if the base unit is GUC_UNIT_KB, 1024
- * is converted to 1 MB, but 1025 is represented as 1025 kB.
+ * The output unit is chosen so that it's the shortest representation that can
+ * represent the value without loss. For example, if the base unit is
+ * GUC_UNIT_KB, 1024 is converted to 1 MB, but 1025 is represented as 1025 kB.
+ * Also 104858 is converted to '0.1 GB', which is shorter than other
+ * representations.
*/
static void
convert_int_from_base_unit(int64 base_value, int base_unit,
- int64 *value, const char **unit)
+ double *value, const char **unit, int *digits)
{
const unit_conversion *table;
int i;
+ int len = 0;
*unit = NULL;
@@ -6027,17 +6030,49 @@ convert_int_from_base_unit(int64 base_value, int base_unit,
{
if (base_unit == table[i].base_unit)
{
+ const double frac_digits = 2;
+ double rval;
+
/*
- * Accept the first conversion that divides the value evenly. We
- * assume that the conversions for each base unit are ordered from
- * greatest unit to the smallest!
+ * Round off the representation at the digit where it is exactly
+ * the same with base_value.
*/
- if (table[i].multiplier <= 1.0 ||
- base_value % (int64) table[i].multiplier == 0)
+ rval = (double)base_value / table[i].multiplier;
+ rval = rint(rval * pow(10, frac_digits)) *
+ pow(10, -frac_digits);
+
+ /* Try the unit if it is exact representation */
+ if ((int64)rint(rval * table[i].multiplier) == base_value)
{
- *value = (int64) rint(base_value / table[i].multiplier);
- *unit = table[i].unit;
- break;
+ int nfrac = 0;
+ int newlen = 1;
+
+ /* Count fraction digits */
+ for (nfrac = 0 ; nfrac < frac_digits ; nfrac++)
+ {
+ double p = pow(10, nfrac);
+ if (rval * p - floor(rval * p) < 0.1)
+ break;
+ }
+
+ /* Caclculate width of the representation */
+ if (rval >= 1.0)
+ newlen += floor(log10(rval));
+ newlen += nfrac;
+ if (nfrac > 0)
+ newlen++; /* for decimal point */
+
+ if (len == 0 || newlen < len)
+ {
+ *digits = nfrac;
+ *value = rval;
+ *unit = table[i].unit;
+ len = newlen;
+ }
+
+ /* We found the integer representation, exit. */
+ if (nfrac == 0)
+ break;
}
}
}
@@ -9359,18 +9394,19 @@ _ShowOption(struct config_generic *record, bool use_units)
* Use int64 arithmetic to avoid overflows in units
* conversion.
*/
- int64 result = *conf->variable;
+ double result = *conf->variable;
const char *unit;
+ int digits = 0;
if (use_units && result > 0 && (record->flags & GUC_UNIT))
- convert_int_from_base_unit(result,
+ convert_int_from_base_unit(*conf->variable,
record->flags & GUC_UNIT,
- &result, &unit);
+ &result, &unit, &digits);
else
unit = "";
- snprintf(buffer, sizeof(buffer), INT64_FORMAT "%s",
- result, unit);
+ snprintf(buffer, sizeof(buffer), "%.*f %s",
+ digits, result, unit);
val = buffer;
}
}