From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Subject: Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
Date: Sat, 16 Feb 2019 20:09:13 -0800
Message-ID: <20190217040913.GA1305059@rfd.leadboat.com> (raw)
In-Reply-To: <20190214072623.GA1139206@rfd.leadboat.com>
References: <20190202083822.GC32531@gust.leadboat.com>
<20190214072623.GA1139206@rfd.leadboat.com>
On Wed, Feb 13, 2019 at 11:26:23PM -0800, Noah Misch wrote:
> On further study, I was able to reproduce data loss
> Fixes are available:
>
> a. Fix the rounding in SimpleLruTruncate(). (The patch I posted upthread is
> wrong; I will correct it in a separate message.)
Here's a corrected version. I now delete a segment only if both its first
page and its last page are considered to precede the cutoff; see the new
comment at SlruMayDeleteSegment().
Attachments:slru-truncate-modulo-v2.patch (text/plain, 4.3K) download
commit 09393a1 (HEAD)
Author: Noah Misch <noah@leadboat.com>
AuthorDate: Sat Feb 16 20:02:51 2019 -0800
Commit: Noah Misch <noah@leadboat.com>
CommitDate: Sat Feb 16 20:02:51 2019 -0800
Don't round SimpleLruTruncate() cutoffPage values.
Every core SLRU wraps around. The rounding did not account for that; in
rare cases, it permitted deletion of the most recently-populated page of
SLRU data. This closes a rare opportunity for data loss, which
manifested as "could not access status of transaction" errors. If a
user's physical replication primary logged ": apparent wraparound"
messages, the user should rebuild that primary's standbys regardless of
symptoms. At less risk is a cluster having emitted "not accepting
commands" errors or "must be vacuumed" warnings at some point. One can
test a cluster for this data loss by running VACUUM FREEZE in every
database. Back-patch to 9.4 (all supported versions).
Discussion: https://postgr.es/m/20190202083822.GC32531@gust.leadboat.com
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.cindex 3623352..71e29b9 100644--- a/src/backend/access/transam/slru.c+++ b/src/backend/access/transam/slru.c@@ -1171,11 +1171,6 @@ SimpleLruTruncate(SlruCtl ctl, int cutoffPage)
int slotno;
/*
- * The cutoff point is the start of the segment containing cutoffPage.- */- cutoffPage -= cutoffPage % SLRU_PAGES_PER_SEGMENT;-- /*
* Scan shared memory and remove any pages preceding the cutoff page, to
* ensure we won't rewrite them later. (Since this is normally called in
* or just after a checkpoint, any dirty pages should have been flushed
@@ -1221,8 +1216,11 @@ restart:;
* Hmm, we have (or may have) I/O operations acting on the page, so
* we've got to wait for them to finish and then start again. This is
* the same logic as in SlruSelectLRUPage. (XXX if page is dirty,
- * wouldn't it be OK to just discard it without writing it? For now,- * keep the logic the same as it was.)+ * wouldn't it be OK to just discard it without writing it?+ * SlruMayDeleteSegment() uses a stricter qualification, so we might+ * not delete this page in the end; even if we don't delete it, we+ * won't have cause to read its data again. For now, keep the logic+ * the same as it was.)
*/
if (shared->page_status[slotno] == SLRU_PAGE_VALID)
SlruInternalWritePage(ctl, slotno, NULL);
@@ -1313,18 +1311,40 @@ restart:
}
/*
+ * Determine whether a segment is okay to delete.+ *+ * segpage is the first page of the segment, and cutoffPage is the oldest (in+ * PagePrecedes order) page in the SLRU containing still-useful data. Since+ * every core PagePrecedes callback implements "wrap around", check the+ * segment's first and last pages:+ *+ * first<cutoff && last<cutoff: yes+ * first<cutoff && last>=cutoff: no; cutoff falls inside this segment+ * first>=cutoff && last<cutoff: no; wrap point falls inside this segment+ * first>=cutoff && last>=cutoff: no; every page of this segment is too young+ */+static bool+SlruMayDeleteSegment(SlruCtl ctl, int segpage, int cutoffPage)+{+ int seg_last_page = segpage + SLRU_PAGES_PER_SEGMENT - 1;++ Assert(segpage % SLRU_PAGES_PER_SEGMENT == 0);++ return (ctl->PagePrecedes(segpage, cutoffPage) &&+ ctl->PagePrecedes(seg_last_page, cutoffPage));+}++/*
* SlruScanDirectory callback
- * This callback reports true if there's any segment prior to the one- * containing the page passed as "data".+ * This callback reports true if there's any segment wholly prior to the+ * one containing the page passed as "data".
*/
bool
SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int segpage, void *data)
{
int cutoffPage = *(int *) data;
- cutoffPage -= cutoffPage % SLRU_PAGES_PER_SEGMENT;-- if (ctl->PagePrecedes(segpage, cutoffPage))+ if (SlruMayDeleteSegment(ctl, segpage, cutoffPage))
return true; /* found one; don't iterate any more */
return false; /* keep going */
@@ -1339,7 +1359,7 @@ SlruScanDirCbDeleteCutoff(SlruCtl ctl, char *filename, int segpage, void *data)
{
int cutoffPage = *(int *) data;
- if (ctl->PagePrecedes(segpage, cutoffPage))+ if (SlruMayDeleteSegment(ctl, segpage, cutoffPage))
SlruInternalDeleteSegment(ctl, filename);
return false; /* keep going */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com
Subject: Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
In-Reply-To: <20190217040913.GA1305059@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox