Thread: Checkpoint throttling issues
Hi, working on the checkpoint sorting/flushing patch I noticed a number of preexisting issues: 1) The progress passed to CheckpointWriteDelay() will often be wrong - it's calculated as num_written / num_to_write, butnum_written is only incremented if the buffer hasn't since independently been written out. That's bad because it mean'swe'll think we're further and further behind if there's independent writeout activity. Simple enough to fix, we gotta split num_written into num_written (for stats purposes) and num_processed (for progress). This is pretty much a bug, but I'm a slightly worried about backpatching a fix because it can have a rather noticeable behavioural impact. 2) CheckpointWriteDelay()'s handling of config file changes et al is pretty weird: The config is reloaded during a checkpoingiff it's not an immediate checkpoint and we're on schedule. I see very little justification for having the got_SIGHUPblock inside that if block. 3) The static pg_usleep(100000L); in CheckpointWriteDelay() is a bad idea. On a system with low write activity (< 10 writes sec) this will lead to checkpoints finishing too early as there'll alwaysbe around ~10 writes a second. On slow IO, say a sdcard, that's bad. On system with a very high write throughput (say 1k+ buffers/sec) the unconditional 100ms sleep while on schedule willmean we're far behind after the sleep. This leads to rather bursty IO, and makes it more likely to run into dirty bufferlimits and such. Just reducing the sleep from 100ms to 10ms leads to significant latency improvements. On pgbenchrate limited to 5k tps: before: number of transactions skipped: 70352 (1.408 %) number of transactions above the 100.0 ms latency limit: 2817(0.056 %) latency average: 5.266 ms latency stddev: 11.723 ms rate limit schedule lag: avg 3.138 (max 0.000) ms after: number of transactions skipped: 41696 (0.834 %) number of transactions above the 100.0 ms latency limit: 1736(0.035 %) latency average: 4.929 ms latency stddev: 8.929 ms rate limit schedule lag: avg 2.835 (max 0.000) ms I think the sleep time should be computed adaptively based on the number of buffers remaining and the remaining time.There's probably better formulations, but that seems like an easy enough improvement and considerably better thannow. 4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule. Greetings, Andres Freund
> preexisting issues: > > 1) The progress passed to CheckpointWriteDelay() will often be wrong - > it's calculated as num_written / num_to_write, but num_written is only > incremented if the buffer hasn't since independently been written > out. That's bad because it mean's we'll think we're further and > further behind if there's independent writeout activity. > > Simple enough to fix, we gotta split num_written into num_written > (for stats purposes) and num_processed (for progress). > > This is pretty much a bug, but I'm a slightly worried about > backpatching a fix because it can have a rather noticeable > behavioural impact. I noticed this discrepancy, but decided agains doing anything about it, because it meant more arguing and explaning. In the pgbench run I used the case does not seem to arise often because most I/O is performed through the checkpointer, so it does not matter much. Another discrepancy is that there are two progress computations, one against time and one against wal buffer, and when one is used and why it is better than the other is not too clear to me. > 2) CheckpointWriteDelay()'s handling of config file changes et al is > pretty weird: The config is reloaded during a checkpoing iff it's not > an immediate checkpoint and we're on schedule. I see very little > justification for having the got_SIGHUP block inside that if block. No opinion. > 3) The static pg_usleep(100000L); in CheckpointWriteDelay() is a bad > idea. > > On a system with low write activity (< 10 writes sec) this will lead > to checkpoints finishing too early as there'll always be around ~10 > writes a second. On slow IO, say a sdcard, that's bad. > > On system with a very high write throughput (say 1k+ buffers/sec) the > unconditional 100ms sleep while on schedule will mean we're far > behind after the sleep. This leads to rather bursty IO, and makes it > more likely to run into dirty buffer limits and such. Just reducing > the sleep from 100ms to 10ms leads to significant latency > improvements. On pgbench rate limited to 5k tps: > > before: > number of transactions skipped: 70352 (1.408 %) > number of transactions above the 100.0 ms latency limit: 2817 (0.056 %) > latency average: 5.266 ms > latency stddev: 11.723 ms > rate limit schedule lag: avg 3.138 (max 0.000) ms > > after: > number of transactions skipped: 41696 (0.834 %) > number of transactions above the 100.0 ms latency limit: 1736 (0.035 %) > latency average: 4.929 ms > latency stddev: 8.929 ms > rate limit schedule lag: avg 2.835 (max 0.000) ms Do these figures include sorting & flushing ? > I think the sleep time should be computed adaptively based on the > number of buffers remaining and the remaining time. There's probably > better formulations, but that seems like an easy enough improvement > and considerably better than now. One this run. If there are few I/Os then the 1 0ms will result in more random/discontinuous I/Os, because there would be only a few writes in each bucket, so I'm not sure whether this would be an unconditionnal improvement. I would be interested to have figures *with* sorting and flushing on and several kind of run. > 4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule. No opinion! -- Fabien.
Fabien COELHO wrote: > >4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule. > > No opinion! My guess here, without looking, is that this was based on the idea of "oops, we're late here for the checkpoint, let's do as less work as possible to avoid getting even later", and thus skip some "unnecessary" tasks. I would vote for fixing this (and the SIGHUP issue too). I'm not sure how bad a behavioral change this is; IMO these fixes should be backpatched all the way back. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Oct 20, 2015 at 1:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabien COELHO wrote:
>
> > >4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule.
> >
> > No opinion!
>
> My guess here, without looking, is that this was based on the idea of
> "oops, we're late here for the checkpoint, let's do as less work as
> possible to avoid getting even later", and thus skip some "unnecessary"
> tasks.
>
> Fabien COELHO wrote:
>
> > >4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule.
> >
> > No opinion!
>
> My guess here, without looking, is that this was based on the idea of
> "oops, we're late here for the checkpoint, let's do as less work as
> possible to avoid getting even later", and thus skip some "unnecessary"
> tasks.
>
I also think thats what could be the reason for writing the code that way,
it seems to me that processing config file when not on schedule has
an advantage such that if user has modified checkpoint related parameters
(like checkpoint_completion_target), that could improve the checkpoint
behaviour. Now there could be some negative impact of this change as
well if user changes full_page_writes parameter, as to process that, it
needs to write a WAL record which could be costly when the checkpoint is
not on schedule, but I think the odds of that are less, so there doesn't
seem to be any harm in changing this behaviour.
On Mon, Oct 19, 2015 at 6:10 AM, Andres Freund <andres@anarazel.de> wrote: > 1) The progress passed to CheckpointWriteDelay() will often be wrong - > it's calculated as num_written / num_to_write, but num_written is only > incremented if the buffer hasn't since independently been written > out. That's bad because it mean's we'll think we're further and > further behind if there's independent writeout activity. > > Simple enough to fix, we gotta split num_written into num_written > (for stats purposes) and num_processed (for progress). > > This is pretty much a bug, but I'm a slightly worried about > backpatching a fix because it can have a rather noticeable > behavioural impact. I think this is an algorithmic improvement, not a bug fix. Actually, I don't really think any of these things are bugs, properly considered - they all look pretty intentional to me, even if we no longer agree with the reasoning. Maybe some of them could be back-patched anyway, but at any rate I definitely wouldn't backpatch this or #3, because even though changing this is probably better on the average, it's hard to be sure that it won't be worse for somebody. In the back-branches, I think stability takes priority over improvements. > I think the sleep time should be computed adaptively based on the > number of buffers remaining and the remaining time. There's probably > better formulations, but that seems like an easy enough improvement > and considerably better than now. One thing to keep in mind here is that somebody did work a few years ago to reduce the number of wake-ups per second that PostgreSQL generates when idle. Now obviously getting the checkpointing behavior correct is more important, and obviously also the system is not idle if we're checkpointing, but it's something to keep in mind. I like the idea of an adaptive sleep time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-10-22 09:52:25 -0400, Robert Haas wrote: > On Mon, Oct 19, 2015 at 6:10 AM, Andres Freund <andres@anarazel.de> wrote: > > 1) The progress passed to CheckpointWriteDelay() will often be wrong - > > it's calculated as num_written / num_to_write, but num_written is only > > incremented if the buffer hasn't since independently been written > > out. That's bad because it mean's we'll think we're further and > > further behind if there's independent writeout activity. > > > > Simple enough to fix, we gotta split num_written into num_written > > (for stats purposes) and num_processed (for progress). > > > > This is pretty much a bug, but I'm a slightly worried about > > backpatching a fix because it can have a rather noticeable > > behavioural impact. > > I think this is an algorithmic improvement, not a bug fix. progress, passed to CheckpointWriteDelay, is defined as:* 'progress' is an estimate of how much of the work has been done,as a* fraction between 0.0 meaning none, and 1.0 meaning all done. passing a potentially wildly inaccurate value (like close to 0, because most buffers have since been displaced) seems more like a bug than an algorithmic improvement... I can't see how that could have been intentional. But I'm now wild on backpatching these, so it's probably a moot distinction. > > I think the sleep time should be computed adaptively based on the > > number of buffers remaining and the remaining time. There's probably > > better formulations, but that seems like an easy enough improvement > > and considerably better than now. > > One thing to keep in mind here is that somebody did work a few years > ago to reduce the number of wake-ups per second that PostgreSQL > generates when idle. As far as I can see checkpointing hasn't been touched in the course of that work. For power usage it's probably not bad to complete checkpoints faster than necessary and then sleep for longer :/. So it might be good to have an upper limit to the sleep time. > I like the idea of an adaptive sleep time. Good. I do too ;) Greetings, Andres Freund