Thread: commit dfda6ebaec67 versus wal_keep_segments
This commit introduced a problem with wal_keep_segments:
commit dfda6ebaec6763090fb78b458a979b558c50b39b
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Sun Jun 24 18:06:38 2012 +0300
Don't waste the last segment of each 4GB logical log file.
in a side window do: watch "ls -lrt /tmp/data/pg_xlog"
dfda6ebaec/bin/initdb -D /tmp/data
dfda6ebaec/bin/pg_ctl -D /tmp/data -l logfile restart -o "--fsync=off --wal_keep_segments=20"
createdb
pgbench -i -s10
pgbench -T3600
xlogs accumulate until there are about 26 of them. Then all of a sudden they drop down to 11 of them. Then it builds back up to around 26, and seems to stay there permanently.
At some point when it is over-pruning and recycling, it recyles the log files that are still needed for recovery, and if the database crashes at that point it will not recover because it can't find either the primary secondary checkpoint records.
Cheers,
Jeff
On Tue, Apr 2, 2013 at 10:08 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
This commit introduced a problem with wal_keep_segments:commit dfda6ebaec6763090fb78b458a979b558c50b39b
The problem seems to be that the underflow warned about is happening, because the check to guard it was checking the wrong thing. However, I don't really understand KeepLogSeg. It seems like segno, and hence recptr, don't actually serve any purpose.
Cheers,
Jeff
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2f9209f..3643be8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8264,7 +8264,7 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
XLByteToSeg(recptr, segno);
/* avoid underflow, don't go below 1 */
- if (segno <= wal_keep_segments)
+ if (*logSegNo <= wal_keep_segments)
segno = 1;
else
segno = *logSegNo - wal_keep_segments;
On 03.04.2013 18:58, Jeff Janes wrote: > On Tue, Apr 2, 2013 at 10:08 PM, Jeff Janes<jeff.janes@gmail.com> wrote: > >> This commit introduced a problem with wal_keep_segments: >> >> commit dfda6ebaec6763090fb78b458a979b558c50b39b > > The problem seems to be that the underflow warned about is happening, > because the check to guard it was checking the wrong thing. However, I > don't really understand KeepLogSeg. It seems like segno, and hence recptr, > don't actually serve any purpose. Hmm, the check is actually correct, but the assignment in the else-branch isn't. The idea of KeepLogSeg is to calculate recptr - wal_keep_segments, and assign that to *logSegNo. But only if *logSegNo is not already < than the calculated value. Does the attached look correct to you? > At some point when it is over-pruning and recycling, it recyles the log > files that are still needed for recovery, and if the database crashes at > that point it will not recover because it can't find either the primary > secondary checkpoint records. So, KeepLogSeg incorrectly sets *logSegNo to 0, and CreateCheckPoint decrements it, causing it to underflow to 2^64-1. Now RemoveOldXlogFiles feels free to remove every WAL segment. - Heikki
Attachment
On Wed, Apr 3, 2013 at 11:14 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 03.04.2013 18:58, Jeff Janes wrote:Hmm, the check is actually correct, but the assignment in the else-branch isn't. The idea of KeepLogSeg is to calculate recptr - wal_keep_segments, and assign that to *logSegNo. But only if *logSegNo is not already < than the calculated value. Does the attached look correct to you?On Tue, Apr 2, 2013 at 10:08 PM, Jeff Janes<jeff.janes@gmail.com> wrote:This commit introduced a problem with wal_keep_segments:
commit dfda6ebaec6763090fb78b458a979b558c50b39b
The problem seems to be that the underflow warned about is happening,
because the check to guard it was checking the wrong thing. However, I
don't really understand KeepLogSeg. It seems like segno, and hence recptr,
don't actually serve any purpose.
Let me describe what I think is going on. My description is "On start, recptr is the redo location of the just-completed checkpoint, and logSegNo is the redo location segment of the checkpoint before that one. We want to keep the previous-checkpoint redo location, and we also want to keep wal_keep_segments before the current-checkpoint redo location, so we take whichever is earlier."
If my understanding is now correct, then I think your patch looks correct. (Also, applying it fixed the problem I was having.)
Why do we keep wal_keep_segments before the just-finished checkpoint, rather than keeping that many before the previous checkpoint? I seems like it would be more intuitive (to the DBA) for that parameter to mean "keep this many more segments than you otherwise would". I'm not proposing we change it, I'm just curious about why it is done that way.
Thanks,
Jeff
On 03.04.2013 22:50, Jeff Janes wrote: > On Wed, Apr 3, 2013 at 11:14 AM, Heikki Linnakangas<hlinnakangas@vmware.com >> wrote: > >> On 03.04.2013 18:58, Jeff Janes wrote: >> >>> On Tue, Apr 2, 2013 at 10:08 PM, Jeff Janes<jeff.janes@gmail.com> wrote: >>> >>> This commit introduced a problem with wal_keep_segments: >>>> >>>> commit dfda6ebaec6763090fb78b458a979b**558c50b39b >>>> >>> >>> The problem seems to be that the underflow warned about is happening, >>> because the check to guard it was checking the wrong thing. However, I >>> don't really understand KeepLogSeg. It seems like segno, and hence >>> recptr, >>> don't actually serve any purpose. >>> >> >> Hmm, the check is actually correct, but the assignment in the else-branch >> isn't. The idea of KeepLogSeg is to calculate recptr - wal_keep_segments, >> and assign that to *logSegNo. But only if *logSegNo is not already< than >> the calculated value. Does the attached look correct to you? > > > Let me describe what I think is going on. My description is "On start, > recptr is the redo location of the just-completed checkpoint, and logSegNo > is the redo location segment of the checkpoint before that one. We want to > keep the previous-checkpoint redo location, and we also want to keep > wal_keep_segments before the current-checkpoint redo location, so we take > whichever is earlier." > > If my understanding is now correct, then I think your patch looks correct. > (Also, applying it fixed the problem I was having.) Ok, thanks, applied. > Why do we keep wal_keep_segments before the just-finished checkpoint, > rather than keeping that many before the previous checkpoint? I seems like > it would be more intuitive (to the DBA) for that parameter to mean "keep > this many more segments than you otherwise would". I'm not proposing we > change it, I'm just curious about why it is done that way. It feels more intuitive to me the way it is. wal_keep_log_segments means "make sure there are always this many old WAL segments available in the server, regardless of any other settings". If you have a standby, it means that you don't need a new base backup as long as you don't fall behind the master by more than wal_keep_segments segments. On 03.04.2013 21:33, Alvaro Herrera wrote: > "by by" Fixed, thanks. - Heikki