From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: All-zero page in GIN index causes assertion failure
Date: Mon, 20 Jul 2015 23:23:41 +0300
Message-ID: <55AD58CD.2000607@iki.fi> (raw)
In-Reply-To: <20150720161124.GR2301@postgresql.org>
References: <55ACADCD.6020206@iki.fi>
<55AD0266.4050300@iki.fi>
<20150720161124.GR2301@postgresql.org>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 07/20/2015 07:11 PM, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
>
>> Looking closer, heap vacuum does a similar thing, but there are two
>> mitigating factors that make it safe in practice, I think:
>>
>> 1. An empty heap page is all-zeroes except for the small page header in the
>> beginning of the page. For a torn page to matter, the page would need to be
>> torn in the header, but we rely elsewhere (control file) anyway that a
>> 512-byte sector update is atomic, so that shouldn't happen. Note that this
>> hinges on the fact that there is no special area on heap pages, so you
>> cannot rely on this for index pages.
>>
>> 2. The redo of the first insert/update on a heap page will always
>> re-initialize the page, even when full-page-writes are disabled. This is the
>> XLOG_HEAP_INIT_PAGE optimization. So it's not just an optimization, it's
>> required for correctness.
>
> Hmm, I guess this is a case of an optimization hiding a bug :-( I mean,
> if we didn't have that, we would have noticed this a lot sooner, I
> imagine.
Yeah, probably.
I came up with the attached, for GIN and SP-GiST. Instead of WAL-logging
the page initialization in SP-GiST vacuum, I changed it so that it
simply leaves the page as all-zeroes, and adds it to the FSM. The code
that grabs a target page from the FSM handles that, and initializes the
page anyway, so that was simpler. This made the SP-GiST is-deleted flag
obsolete, it's no longer set, although the code still checks for it for
backwards-compatibility. (even that may actually be unnecessary, as a
page that's marked as deleted must also be empty, and wherever we check
for the deleted-flag, we also check for PageIsEmpty()))
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[application/x-patch] fix-gin-spgist-vacuum-1.patch (2.9K, ../55AD58CD.2000607@iki.fi/2-fix-gin-spgist-vacuum-1.patch)
download | inline diff:diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.cindex eba572b..1315762 100644--- a/src/backend/access/gin/ginvacuum.c+++ b/src/backend/access/gin/ginvacuum.c@@ -710,7 +710,7 @@ ginvacuumcleanup(PG_FUNCTION_ARGS)
LockBuffer(buffer, GIN_SHARE);
page = (Page) BufferGetPage(buffer);
- if (GinPageIsDeleted(page))+ if (PageIsNew(page) || GinPageIsDeleted(page))
{
Assert(blkno != GIN_ROOT_BLKNO);
RecordFreeIndexPage(index, blkno);
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.cindex dc69d1e..36e93ad 100644--- a/src/backend/access/spgist/spgvacuum.c+++ b/src/backend/access/spgist/spgvacuum.c@@ -621,14 +621,10 @@ spgvacuumpage(spgBulkDeleteState *bds, BlockNumber blkno)
{
/*
* We found an all-zero page, which could happen if the database
- * crashed just after extending the file. Initialize and recycle it.+ * crashed just after extending the file. Recycle it.
*/
- SpGistInitBuffer(buffer, 0);- SpGistPageSetDeleted(page);- /* We don't bother to WAL-log this action; easy to redo */- MarkBufferDirty(buffer);
}
- else if (SpGistPageIsDeleted(page))+ else if (PageIsEmpty(page))
{
/* nothing to do */
}
@@ -659,25 +655,18 @@ spgvacuumpage(spgBulkDeleteState *bds, BlockNumber blkno)
*/
if (!SpGistBlockIsRoot(blkno))
{
- /* If page is now empty, mark it deleted */- if (PageIsEmpty(page) && !SpGistPageIsDeleted(page))- {- SpGistPageSetDeleted(page);- /* We don't bother to WAL-log this action; easy to redo */- MarkBufferDirty(buffer);- }-- if (SpGistPageIsDeleted(page))+ if (PageIsEmpty(page))
{
RecordFreeIndexPage(index, blkno);
bds->stats->pages_deleted++;
}
else
+ {+ SpGistSetLastUsedPage(index, buffer);
bds->lastFilledBlock = blkno;
+ }
}
- SpGistSetLastUsedPage(index, buffer);-
UnlockReleaseBuffer(buffer);
}
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.hindex 413f71e..48dadd5 100644--- a/src/include/access/spgist_private.h+++ b/src/include/access/spgist_private.h@@ -48,14 +48,14 @@ typedef SpGistPageOpaqueData *SpGistPageOpaque;
/* Flag bits in page special space */
#define SPGIST_META (1<<0)
-#define SPGIST_DELETED (1<<1)+#define SPGIST_DELETED (1<<1) /* never set, but keep for backwards+ * compatibility */
#define SPGIST_LEAF (1<<2)
#define SPGIST_NULLS (1<<3)
#define SpGistPageGetOpaque(page) ((SpGistPageOpaque) PageGetSpecialPointer(page))
#define SpGistPageIsMeta(page) (SpGistPageGetOpaque(page)->flags & SPGIST_META)
#define SpGistPageIsDeleted(page) (SpGistPageGetOpaque(page)->flags & SPGIST_DELETED)
-#define SpGistPageSetDeleted(page) (SpGistPageGetOpaque(page)->flags |= SPGIST_DELETED)
#define SpGistPageIsLeaf(page) (SpGistPageGetOpaque(page)->flags & SPGIST_LEAF)
#define SpGistPageStoresNulls(page) (SpGistPageGetOpaque(page)->flags & SPGIST_NULLS)
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: hlinnaka@iki.fi, alvherre@2ndquadrant.com
Subject: Re: All-zero page in GIN index causes assertion failure
In-Reply-To: <55AD58CD.2000607@iki.fi>
* 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