Load Distributed Checkpoints, take 3

Heikki Linnakangas <heikki@enterprisedb.com> writes:

I added a spinlock to protect the signaling fields between bgwriter and
backends. The current non-locking approach gets really difficult as the
patch adds two new flags, and both are more important than the existing
ckpt_time_warn flag.

That may be, but you could minimize the cost and notational ugliness by
not using the spinlock where you don't have to. Put the sig_atomic_t
fields back the way they were, and many of the uses of the spinlock go
away. All you really need it for is the places where the additional
flags are set or read.

In fact, I think there's a small race condition in CVS HEAD:

Yeah, probably --- the original no-locking design didn't have any side
flags. The reason you need the lock is for a backend to be sure that
a newly-started checkpoint is using its requested flags. But the
detection of checkpoint termination is still the same.

Some other comments:

I tend to agree with whoever said upthread that the combination of GUC
variables proposed here is confusing and ugly. It'd make more sense to
have min and max checkpoint rates in KB/s, with the max checkpoint rate
only honored when we are predicting we'll finish before the next
checkpoint time.

The flow of control and responsibility between xlog.c, bgwriter.c and
bufmgr.c seems to have reached the breaking point of unintelligibility.
Can you think of a refactoring that would simplify it? We might have
to resign ourselves to this much messiness, but I'd rather not...

regards, tom lane

Heikki Linnakangas <heikki@enterprisedb.com> wrote:

Here's an updated WIP patch for load distributed checkpoints.
Since last patch, I did some clean up and refactoring, and added a bunch
of comments, and user documentation.

The only thing I don't understand is the naming of 'checkpoint_smoothing'.
Can users imagine the unit of 'smoothing' is a fraction?

You explain the paremeter with the word 'fraction'.
Why don't you simply name it 'checkpoint_fraction' ?
| Specifies the target length of checkpoints, as a fraction of
| the checkpoint interval. The default is 0.3.

Sorry if I'm missing discussions abount the naming.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:

The only thing I don't understand is the naming of 'checkpoint_smoothing'.
Can users imagine the unit of 'smoothing' is a fraction?

You explain the paremeter with the word 'fraction'.
Why don't you simply name it 'checkpoint_fraction' ?
| Specifies the target length of checkpoints, as a fraction of
| the checkpoint interval. The default is 0.3.

I chose checkpoint_smoothing because it tells you what the parameter is
for. If you want more smoothing, tune it up, and if you want less, tune
it down. checkpoint_fraction makes you wonder what you can do with it
and why you would change it.

Sorry if I'm missing discussions abount the naming.

No, I chose _smoothing on my own. People didn't like
checkpoint_write_percent either (including).

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

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

I added a spinlock to protect the signaling fields between bgwriter and
backends. The current non-locking approach gets really difficult as the
patch adds two new flags, and both are more important than the existing
ckpt_time_warn flag.

That may be, but you could minimize the cost and notational ugliness by
not using the spinlock where you don't have to. Put the sig_atomic_t
fields back the way they were, and many of the uses of the spinlock go
away. All you really need it for is the places where the additional
flags are set or read.

I find it easier to understand if it's used whenever any of the fields
are accessed. You don't need to read and write ckpt_failed and
ckpt_started/ckpt_done in specific order anymore, for example.

Some other comments:

I tend to agree with whoever said upthread that the combination of GUC
variables proposed here is confusing and ugly. It'd make more sense to
have min and max checkpoint rates in KB/s, with the max checkpoint rate
only honored when we are predicting we'll finish before the next
checkpoint time.

Really? I thought everyone is happy with the current combination, and
that it was just the old trio of parameters controlling the write, nap
and sync phases that was ugly. One particularly nice thing about
defining the duration as a fraction of checkpoint interval is that we
can come up with a reasonable default value that doesn't depend on your
hardware.

How would a min and max rate work?

Anyone else have an opinion on the parameters?

The flow of control and responsibility between xlog.c, bgwriter.c and
bufmgr.c seems to have reached the breaking point of unintelligibility.
Can you think of a refactoring that would simplify it? We might have
to resign ourselves to this much messiness, but I'd rather not...

The problem we're trying to solve is doing a checkpoint while running
the normal bgwriter activity at the same time. The normal way to do two
things simultaneously is to have two different processes (or threads). I
thought about having a separate checkpoint process, but I gave up on
that thought because the communication needed between backends, bgwriter
and the checkpointer seems like a mess. The checkpointer would need the
pendingOpsTable so that it can do the fsyncs, and it would also need to
receive the forget-messages to that table. We could move that table
entirely to the checkpointer, but since bgwriter is presumably doing
most of the writes, there would be a lot of chatter between bgwriter and
the checkpointer.

The current approach is like co-operative multitasking. BufferSyncs
yields control to bgwriter every now and then.

The division of labor between xlog.c and other modules is not that bad,
IMO. CreateCheckPoint is the main entry point to create a checkpoint,
and it calls other modules to do their stuff, including bufmgr.c.

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

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

In fact, I think there's a small race condition in CVS HEAD:

Yeah, probably --- the original no-locking design didn't have any side
flags. The reason you need the lock is for a backend to be sure that
a newly-started checkpoint is using its requested flags. But the
detection of checkpoint termination is still the same.

Actually, the race condition I outlined isn't related to the flags. It's
possible because RequestCheckpoint doesn't guarantee that a checkpoint
is performed when there's been no WAL activity since last one.

I did use a new force-flag to fix it, but I'm sure there is other ways.

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

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

I tend to agree with whoever said upthread that the combination of GUC
variables proposed here is confusing and ugly. It'd make more sense to
have min and max checkpoint rates in KB/s, with the max checkpoint rate
only honored when we are predicting we'll finish before the next
checkpoint time.

Really? I thought everyone is happy with the current combination, and
that it was just the old trio of parameters controlling the write, nap
and sync phases that was ugly. One particularly nice thing about
defining the duration as a fraction of checkpoint interval is that we
can come up with a reasonable default value that doesn't depend on your
hardware.

That argument would hold some water if you weren't introducing a
hardware-dependent min rate in the same patch. Do we need the min rate
at all? If so, why can't it be in the same units as the max (ie, a
fraction of checkpoint)?

How would a min and max rate work?

Pretty much the same as the code does now, no? You either delay, or not.

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

I tend to agree with whoever said upthread that the combination of GUC
variables proposed here is confusing and ugly. It'd make more sense to
have min and max checkpoint rates in KB/s, with the max checkpoint rate
only honored when we are predicting we'll finish before the next
checkpoint time.

Really? I thought everyone is happy with the current combination, and
that it was just the old trio of parameters controlling the write, nap
and sync phases that was ugly. One particularly nice thing about
defining the duration as a fraction of checkpoint interval is that we
can come up with a reasonable default value that doesn't depend on your
hardware.

That argument would hold some water if you weren't introducing a
hardware-dependent min rate in the same patch. Do we need the min rate
at all? If so, why can't it be in the same units as the max (ie, a
fraction of checkpoint)?

How would a min and max rate work?

Pretty much the same as the code does now, no? You either delay, or not.

I don't think you understand how the settings work. Did you read the
documentation? If you did, it's apparently not adequate.

The main tuning knob is checkpoint_smoothing, which is defined as a
fraction of the checkpoint interval (both checkpoint_timeout and
checkpoint_segments are taken into account). Normally, the write phase
of a checkpoint takes exactly that much time.

So the length of a checkpoint stays the same regardless of how many
dirty buffers there is (assuming you don't exceed the bandwidth of your
hardware), but the I/O rate varies.

There's another possible strategy: keep the I/O rate constant, but vary
the length of the checkpoint. checkpoint_rate allows you to do that.

I'm envisioning we set the defaults so that checkpoint_smoothing is the
effective control in a relatively busy system, and checkpoint_rate
ensures that we don't unnecessarily prolong checkpoints on an idle system.

Now how would you replace checkpoint_smoothing with a max I/O rate? The
only real alternative way of defining it is directly as a time and/or
segments, similar to checkpoint_timeout and checkpoint_segments, but
then we'd really need two knobs instead of one.

Though maybe we could just hard-code it to 0.8, for example, and tell
people to tune checkpoint_rate if they want more aggressive checkpoints.

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

Heikki Linnakangas <heikki@enterprisedb.com> writes:

I don't think you understand how the settings work. Did you read the
documentation? If you did, it's apparently not adequate.

I did read the documentation, and I'm not complaining that I don't
understand it. I'm complaining that I don't like the presented API
because it's self-inconsistent. You've got two parameters that are in
effect upper and lower bounds for the checkpoint write rate, but they
are named inconsistently and not even measured in the same kind of unit.
Nor do I agree that the inconsistency buys any ease of use.

The main tuning knob is checkpoint_smoothing, which is defined as a
fraction of the checkpoint interval (both checkpoint_timeout and
checkpoint_segments are taken into account). Normally, the write phase
of a checkpoint takes exactly that much time.

So the question is, why in the heck would anyone want the behavior that
"checkpoints take exactly X time"?? The useful part of this whole patch
is to cap the write rate at something that doesn't interfere too much
with foreground queries. I don't see why people wouldn't prefer
"checkpoints can take any amount of time up to the checkpoint interval,
but we do our best not to exceed Y writes/second".

Basically I don't see what useful values checkpoint_smoothing would have
other than 0 and 1. You might as well make it a bool.

There's another possible strategy: keep the I/O rate constant, but vary
the length of the checkpoint. checkpoint_rate allows you to do that.

But only from the lower side.

Now how would you replace checkpoint_smoothing with a max I/O rate?

I don't see why you think that's hard. It looks to me like the
components of the decision are the same numbers in any case: you have to
estimate your progress towards checkpoint completion, your available
time till next checkpoint, and your write rate. Then you either delay
or not.

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

The main tuning knob is checkpoint_smoothing, which is defined as a
fraction of the checkpoint interval (both checkpoint_timeout and
checkpoint_segments are taken into account). Normally, the write phase
of a checkpoint takes exactly that much time

So the question is, why in the heck would anyone want the behavior that
"checkpoints take exactly X time"?? The useful part of this whole patch
is to cap the write rate at something that doesn't interfere too much
with foreground queries. I don't see why people wouldn't prefer
"checkpoints can take any amount of time up to the checkpoint interval,
but we do our best not to exceed Y writes/second".

Because it's easier to tune. You don't need to know how much checkpoint
I/O you can tolerate. The system will use just enough I/O bandwidth to
meet the deadline, but not more than that.

Basically I don't see what useful values checkpoint_smoothing would have
other than 0 and 1. You might as well make it a bool.

Well that's one option. It feels like a good thing to be able to control
how much headroom you have until the next checkpoint, but maybe we
can just hardcode it close to 1. It's also good to avoid spreading the
checkpoints unnecessarily, to keep recovery times lower, but you can
control that with the min rate setting as well.

There's another possible strategy: keep the I/O rate constant, but vary
the length of the checkpoint. checkpoint_rate allows you to do that.

But only from the lower side.

Now how would you replace checkpoint_smoothing with a max I/O rate?

I don't see why you think that's hard. It looks to me like the
components of the decision are the same numbers in any case: you have to
estimate your progress towards checkpoint completion, your available
time till next checkpoint, and your write rate. Then you either delay
or not.

Let me put it this way: If you define a min and a max I/O rate, when
would the max I/O rate limit take effect? If there's few dirty buffers
in the pool, so that you'll finish the checkpoint in time before the
next one is due writing at the min rate, that's what you'll use. If
there's more, you'll need to write fast enough that you'll finish the
checkpoint in time, regardless of the max rate. Or would you let the
next checkpoint slip and keep writing at the max rate? That seems like a
footgun if someone sets it to a too low value.

Or are you thinking that we have just one setting: checkpoint_rate? You
describe it as a maximum, but I've been thinking of it as a minimum
because you *will* exceed it if the next checkpoint is due soon.

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

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

So the question is, why in the heck would anyone want the behavior that
"checkpoints take exactly X time"??

Because it's easier to tune. You don't need to know how much checkpoint
I/O you can tolerate. The system will use just enough I/O bandwidth to
meet the deadline, but not more than that.

Uh, not really. After consuming more caffeine and reading the patch
more carefully, I think there are several problems here:

1. checkpoint_rate is used thusly:

writes_per_nap = Min(1, checkpoint_rate / BgWriterDelay);

where writes_per_nap is the max number of dirty blocks to write before
taking a bgwriter nap. Now surely this is completely backward: if
BgWriterDelay is increased, the number of writes to allow per nap
decreases? If you think checkpoint_rate is expressed in some kind of
physical bytes/sec unit, that cannot be right; the number of blocks
per nap has to increase if the naps get longer. (BTW, the patch seems
a bit schizoid about whether checkpoint_rate is int or float.)

2. checkpoint_smoothing is used thusly:

/* scale progress according to CheckPointSmoothing */
progress *= CheckPointSmoothing;

where the progress value being scaled is the fraction so far completed
of the total number of dirty pages we expect to have to write. This
is then compared against estimates of the total fraction of the time-
between-checkpoints that has elapsed; if less, we are behind schedule
and should not nap, if more, we are ahead of schedule and may nap.
(This is a bit odd, but I guess it's all right because it's equivalent
to dividing the elapsed-time estimate by CheckPointSmoothing, which
seems a more natural way of thinking about what needs to happen.)

What's bugging me about this is that we are either going to be writing
at checkpoint_rate if ahead of schedule, or max possible rate if behind;
that's not "smoothing" to me, that's introducing some pretty bursty
behavior. ISTM that actual "smoothing" would involve adjusting
writes_per_nap up or down according to whether we are ahead or behind
schedule, so as to have a finer degree of control over the I/O rate.
(I'd also consider saving the last writes_per_nap value across
checkpoints so as to have a more nearly accurate starting value next
time.)

In any case I still concur with Takahiro-san that "smoothing" doesn't
seem the most appropriate name for the parameter. Something along the
lines of "checkpoint_completion_target" would convey more about what it
does, I think.

And checkpoint_rate really needs to be named checkpoint_min_rate, if
it's going to be a minimum. However, I question whether we need it at
all, because as the code stands, with the default BgWriterDelay you
would have to increase checkpoint_rate to 4x its proposed default before
writes_per_nap moves off its minimum of 1. This says to me that the
system's tested behavior has been so insensitive to checkpoint_rate
that we probably need not expose such a parameter at all; just hardwire
the minimum writes_per_nap at 1.

regards, tom lane

Tom Lane wrote:

1. checkpoint_rate is used thusly:

writes_per_nap = Min(1, checkpoint_rate / BgWriterDelay);

where writes_per_nap is the max number of dirty blocks to write before
taking a bgwriter nap. Now surely this is completely backward: if
BgWriterDelay is increased, the number of writes to allow per nap
decreases? If you think checkpoint_rate is expressed in some kind of
physical bytes/sec unit, that cannot be right; the number of blocks
per nap has to increase if the naps get longer.

Uh, you're right, that's just plain wrong. checkpoint_rate is in
pages/s, so that line should be

writes_per_nap = Min(1, checkpoint_rate * BgWriterDelay / 1000)

(BTW, the patch seems
a bit schizoid about whether checkpoint_rate is int or float.)

Yeah, I've gone back and forth on the data type. I wanted it to be a
float, but guc code doesn't let you specify a float in KB, so I switched
it to int.

2. checkpoint_smoothing is used thusly:

/* scale progress according to CheckPointSmoothing */
progress *= CheckPointSmoothing;

where the progress value being scaled is the fraction so far completed
of the total number of dirty pages we expect to have to write. This
is then compared against estimates of the total fraction of the time-
between-checkpoints that has elapsed; if less, we are behind schedule
and should not nap, if more, we are ahead of schedule and may nap.

(This is a bit odd, but I guess it's all right because it's equivalent
to dividing the elapsed-time estimate by CheckPointSmoothing, which
seems a more natural way of thinking about what needs to happen.)

Yeah, it's a bit unnatural. I did it that way so we don't have to divide
all three of the estimates: cached_elapsed, progress_in_time and
progress_in_xlog. Maybe it's not worth micro-optimizing that.

What's bugging me about this is that we are either going to be writing
at checkpoint_rate if ahead of schedule, or max possible rate if behind;
that's not "smoothing" to me, that's introducing some pretty bursty
behavior. ISTM that actual "smoothing" would involve adjusting
writes_per_nap up or down according to whether we are ahead or behind
schedule, so as to have a finer degree of control over the I/O rate.
(I'd also consider saving the last writes_per_nap value across
checkpoints so as to have a more nearly accurate starting value next
time.)

That sounds a lot more complex. The burstiness at that level shouldn't
matter much. The OS is still going to cache the writes, and should even
out the bursts.

Assuming time/xlogs elapse at a steady rate, we will write some multiple
of writes_per_nap pages between each sleep. With a small writes_per_nap,
writing just writes_per_nap isn't enough to catch up after we fall
behind, so we'll write more than that between each sleep. That means
that on each iteration, we'll write either N*writes_per_nap, or
(N+1)*writes_per_nap. At worst, that means either writes_per_nap or
2*writes_per_nap pages on each iteration. That's not too bad, I think.

In any case I still concur with Takahiro-san that "smoothing" doesn't
seem the most appropriate name for the parameter. Something along the
lines of "checkpoint_completion_target" would convey more about what it
does, I think.

Ok, I'm not wedded to smoothing.

And checkpoint_rate really needs to be named checkpoint_min_rate, if
it's going to be a minimum. However, I question whether we need it at
all, because as the code stands, with the default BgWriterDelay you
would have to increase checkpoint_rate to 4x its proposed default before
writes_per_nap moves off its minimum of 1.

Hmm. With bgwriter_delay of 200 ms, and checkpoint_min_rate of 512 KB/s,
using the non-broken formula above, we get:

(512*1024/8192) * 200 / 1000 = 12.8, truncated to 12.

So I think that's fine. (looking at the patch, I see that the default in
guc.c is actually 100 pages / s, contrary to documentation; needs to be
fixed)

This says to me that the
system's tested behavior has been so insensitive to checkpoint_rate
that we probably need not expose such a parameter at all; just hardwire
the minimum writes_per_nap at 1.

I've set checkpoint_rate to a small value in my tests on purpose to
control the feature with the other parameter. That must be why I haven't
noticed the bogus calculation of it before.

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

Tom Lane wrote:

And checkpoint_rate really needs to be named checkpoint_min_rate, if
it's going to be a minimum. However, I question whether we need it at
all, because as the code stands, with the default BgWriterDelay you
would have to increase checkpoint_rate to 4x its proposed default before
writes_per_nap moves off its minimum of 1. This says to me that the
system's tested behavior has been so insensitive to checkpoint_rate
that we probably need not expose such a parameter at all; just hardwire
the minimum writes_per_nap at 1.

I agree on starting with as simple a GUC as possible and expand it as
there is need in the field. 99% of people are never going to test this
value as much as you are.

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

+ If your life is a hard drive, Christ can be your backup. +

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

(BTW, the patch seems
a bit schizoid about whether checkpoint_rate is int or float.)

Yeah, I've gone back and forth on the data type. I wanted it to be a
float, but guc code doesn't let you specify a float in KB, so I switched
it to int.

I seriously question trying to claim that it's blocks at all, seeing
that the *actual* units are pages per unit time. Pretending that it's
a memory unit does more to obscure the truth than document it.

What's bugging me about this is that we are either going to be writing
at checkpoint_rate if ahead of schedule, or max possible rate if behind;
that's not "smoothing" to me, that's introducing some pretty bursty
behavior.

That sounds a lot more complex. The burstiness at that level shouldn't
matter much. The OS is still going to cache the writes, and should even
out the bursts.

With the changes you're proposing here, the burstiness would be quite
severe. OTOH, if writes_per_nap is always 1, then bufmgr is going to
recheck the delay situation after every page, so what you have actually
tested is as granular as it could get.

And checkpoint_rate really needs to be named checkpoint_min_rate, if
it's going to be a minimum. However, I question whether we need it at
all,

Hmm. With bgwriter_delay of 200 ms, and checkpoint_min_rate of 512 KB/s,
using the non-broken formula above, we get:

(512*1024/8192) * 200 / 1000 = 12.8, truncated to 12.

So I think that's fine.

"Fine?" That's 12x the value you have actually tested. That's enough
of a change to invalidate all your performance testing IMHO. I still
think you've not demonstrated a need to expose this parameter.

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

(BTW, the patch seems
a bit schizoid about whether checkpoint_rate is int or float.)

Yeah, I've gone back and forth on the data type. I wanted it to be a
float, but guc code doesn't let you specify a float in KB, so I switched
it to int.

I seriously question trying to claim that it's blocks at all, seeing
that the *actual* units are pages per unit time. Pretending that it's
a memory unit does more to obscure the truth than document it.

Hmm. What I wanted to avoid is that the I/O rate you get then depends on
your bgwriter_delay, so you if you change that you need to change
checkpoint_min_rate as well.

Now we already have that issue with bgwriter_all/lru_maxpages, and I
don't like it there either. If you think it's better to let the user
define it directly as pages/bgwriter_delay, fine.

And checkpoint_rate really needs to be named checkpoint_min_rate, if
it's going to be a minimum. However, I question whether we need it at
all,

Hmm. With bgwriter_delay of 200 ms, and checkpoint_min_rate of 512 KB/s,
using the non-broken formula above, we get:

(512*1024/8192) * 200 / 1000 = 12.8, truncated to 12.

So I think that's fine.

"Fine?" That's 12x the value you have actually tested. That's enough
of a change to invalidate all your performance testing IMHO.

I'll reschedule the tests to be sure, after we settle on how we want to
control this feature.

I still think you've not demonstrated a need to expose this parameter.

Greg Smith wanted to explicitly control the I/O rate, and let the
checkpoint duration vary. I personally think that fixing the checkpoint
duration is better because it's easier to tune.

But if we only do that, you might end up with ridiculously long
checkpoints when there's not many dirty pages. If we want to avoid that,
we need some way of telling what's a safe minimum rate to write at,
because that can vary greatly depending on your hardware.

But maybe we don't care about prolonging checkpoints, and don't really
need any GUCs at all. We could then just hardcode writes_per_nap to some
low value, and target duration close to 1.0. You would have a checkpoint
running practically all the time, and you would use
checkpoint_timeout/checkpoint_segments to control how long it takes. I'm
a bit worried about jumping to such a completely different regime,
though. For example, pg_start_backup needs to create a new checkpoint,
so it would need to wait on average 1.5 * checkpoint_timeout/segments,
and recovery would need to process on average 1.5 as much WAL as before.
Though with LDC, you should get away with shorter checkpoint intervals
than before, because the checkpoints aren't as invasive.

If we do that, we should remove bgwriter_all_* settings. They wouldn't
do much because we would have checkpoint running all the time, writing
out dirty pages.

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

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Tom Lane wrote:

I still think you've not demonstrated a need to expose this parameter.

Greg Smith wanted to explicitly control the I/O rate, and let the
checkpoint duration vary. I personally think that fixing the checkpoint
duration is better because it's easier to tune.

But if we only do that, you might end up with ridiculously long
checkpoints when there's not many dirty pages. If we want to avoid that,
we need some way of telling what's a safe minimum rate to write at,
because that can vary greatly depending on your hardware.

As long as the minimum rate is at least 1/bgdelay, I don't think this is
a big problem.

But maybe we don't care about prolonging checkpoints, and don't really
need any GUCs at all. We could then just hardcode writes_per_nap to some
low value, and target duration close to 1.0. You would have a checkpoint
running practically all the time, and you would use
checkpoint_timeout/checkpoint_segments to control how long it takes. I'm
a bit worried about jumping to such a completely different regime,
though. For example, pg_start_backup needs to create a new checkpoint,
so it would need to wait on average 1.5 * checkpoint_timeout/segments,

Maybe I misread the patch, but I thought that if someone requested an
immediate checkpoint, the checkpoint-in-progress would effectively flip
to immediate mode. So that could be handled by offering an immediate vs
extended checkpoint option in pg_start_backup. I'm not sure it's a
problem though, since as previously noted you probably want
pg_start_backup to be noninvasive. Also, one could do a manual
CHECKPOINT command then immediately pg_start_backup if one wanted
as-fast-as-possible (CHECKPOINT requests immediate checkpoint, right?)

and recovery would need to process on average 1.5 as much WAL as before.
Though with LDC, you should get away with shorter checkpoint intervals
than before, because the checkpoints aren't as invasive.

No, you still want a pretty long checkpoint interval, because of the
increase in WAL traffic due to more page images being dumped when the
interval is short.

If we do that, we should remove bgwriter_all_* settings. They wouldn't
do much because we would have checkpoint running all the time, writing
out dirty pages.

Yeah, I'm not sure that we've thought through the interactions with the
existing bgwriter behavior.

regards, tom lane

Tom Lane wrote:

Maybe I misread the patch, but I thought that if someone requested an
immediate checkpoint, the checkpoint-in-progress would effectively flip
to immediate mode. So that could be handled by offering an immediate vs
extended checkpoint option in pg_start_backup. I'm not sure it's a
problem though, since as previously noted you probably want
pg_start_backup to be noninvasive. Also, one could do a manual
CHECKPOINT command then immediately pg_start_backup if one wanted
as-fast-as-possible (CHECKPOINT requests immediate checkpoint, right?)

Yeah, that's possible.

and recovery would need to process on average 1.5 as much WAL as before.
Though with LDC, you should get away with shorter checkpoint intervals
than before, because the checkpoints aren't as invasive.

No, you still want a pretty long checkpoint interval, because of the
increase in WAL traffic due to more page images being dumped when the
interval is short.

If we do that, we should remove bgwriter_all_* settings. They wouldn't
do much because we would have checkpoint running all the time, writing
out dirty pages.

Yeah, I'm not sure that we've thought through the interactions with the
existing bgwriter behavior.

I searched the archives a bit for the discussions when the current
bgwriter settings were born, and found this thread:

http://archives.postgresql.org/pgsql-hackers/2004-12/msg00784.php

The idea of Load Distributed Checkpoints certainly isn't new :).

Ok, if we approach this from the idea that there will be *no* GUC
variables at all to control this, and we remove the bgwriter_all_*
settings as well, does anyone see a reason why that would be bad? Here's
the ones mentioned this far:

1. we need to keep 2x as much WAL segments around as before.

2. pg_start_backup will need to wait for a long time.

3. Recovery will take longer, because the distance last committed redo
ptr will lag behind more.

1. and 3. can be alleviated by using a smaller
checkpoint_timeout/segments though as you pointed out that leads to
higher WAL traffic. 2. is not a big deal, and we can add an 'immediate'
parameter to pg_start_backup if necessary.

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

On Fri, 22 Jun 2007, Tom Lane wrote:

Yeah, I'm not sure that we've thought through the interactions with the
existing bgwriter behavior.

The entire background writer mess needs a rewrite, and the best way to
handle that is going to shift considerably with LDC applied.

As the person who was complaining about corner cases I'm not in a position
to talk more explicitly about, I can at least summarize my opinion of how
I feel everyone should be thinking about this patch and you can take what
you want from that. In the context of getting an 8.3 release finalized, I
think you should be building a LDC implementation that accomplishes the
main goal of spreading the checkpoints out, which is clearly working, and
erring on the side of more knobs in cases where it's unsure if they're
needed or not. It's clear what my position is which non-obvious knobs I
think are important for some people.

Which is worse: putting in a tuning setting that it's discovered
everybody just uses the same value for, or discovering after release that
there's a use-case for that setting and by hard-coding it you've made the
DBAs for a class of applications you didn't think about miserable? In the
cases where there's good evidence so far of the right setting, just make
that the default, and the only harm is GUC bloat.

Nothing should be done that changes the existing behavior if the LDC
feature is turned off, so anything more obtrusive to the background writer
is right out. Make reducing the knobs, optimizing the default behavior,
and rewriting the background writer to better fit into its new context a
major goal of 8.4. I know I've got a whole notebook full of stuff on that
topic I've been ignoring as not to distract you guys from getting 8.3
done.

That's the low risk plan, and the design/beta/release period here is short
enough that I think going too experimental beyond that is a bad idea. To
pick an example, when I read this idea from Heikki:

You would have a checkpoint running practically all the time, and you
would use checkpoint_timeout/checkpoint_segments to control how long it
takes... If we do that, we should remove bgwriter_all_* settings

Whether or not I think this is an awesome idea, the very idea of a change
that big at this point gives me the willies. Just off the top of my head,
there's a whole class of issues involving recycling xlog segments this
would introduce I would be really unhappy with the implications of. Did
anyone else ever notice that when a new xlog segment is created, the write
to clear it out doesn't happen via direct I/O like the rest of the xlog
writes do? That write goes through the regular buffered path instead.
The checkpoint logging patch I submitted logs when this happens
specifically because that particular issue messed with some operations and
I found it important to be aware of.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

Heikki Linnakangas <heikki@enterprisedb.com> writes:

Ok, if we approach this from the idea that there will be *no* GUC
variables at all to control this, and we remove the bgwriter_all_*
settings as well, does anyone see a reason why that would be bad? Here's
the ones mentioned this far:

1. we need to keep 2x as much WAL segments around as before.

No, only 50% more. We've always kept WAL back to the
checkpoint-before-last, so the peak usage has been about 2 checkpoint
distances (plus a bit), and now it'd tend to be about 3.

2. pg_start_backup will need to wait for a long time.

3. Recovery will take longer, because the distance last committed redo
ptr will lag behind more.

True, you'd have to replay 1.5 checkpoint intervals on average instead
of 0.5 (more or less, assuming checkpoints had been short). I don't
think we're in the business of optimizing crash recovery time though.

It might be that getting rid of checkpoint_smoothing is an overreaction,
and that we should keep that parameter. IIRC Greg had worried about
being able to turn this behavior off, so we'd still need at least a
bool, and we might as well expose the fraction instead. (Still don't
like the name "smoothing" though.) I agree with removing the non-LRU
part of the bgwriter's write logic though; that should simplify matters
a bit and cut down the overall I/O volume.

regards, tom lane

Greg Smith <gsmith@gregsmith.com> writes:

As the person who was complaining about corner cases I'm not in a position
to talk more explicitly about, I can at least summarize my opinion of how
I feel everyone should be thinking about this patch and you can take what
you want from that.

Sorry, but we're not going to make decisions on the basis of evidence
you won't show us. Right at the moment the best thing to do seems to
be to enable LDC with a low minimum write rate and a high target
duration, and remove the thereby-obsoleted "all buffers" scan of the
existing bgwriter logic. If you've got specific evidence why any of
these things need to be parameterized, let's see it. Personally I think
that we have a bad track record of exposing GUC variables as a
substitute for understanding performance issues at the start, and this
approach isn't doing any favors for DBAs.

Whether or not I think this is an awesome idea, the very idea of a change
that big at this point gives me the willies. Just off the top of my head,
there's a whole class of issues involving recycling xlog segments this
would introduce I would be really unhappy with the implications of.

Really? Name one. AFAICS this should not affect xlog recycling in the
slightest (other than that we have to bump up the target number of live
segments from 2x to 3x the checkpoint distance).

Did anyone else ever notice that when a new xlog segment is created,
the write to clear it out doesn't happen via direct I/O like the rest
of the xlog writes do?

It's not supposed to matter, because that path isn't supposed to be
taken often.

regards, tom lane