I just realized that my patch that turned XLogRecPtr into a uint64
changed the on-disk format of GiST indexes, because the NSN field in the
page header is an XLogRecPtr. Oops. Fortunately that's easy to fix. I
avoided the same issue with LSNs by continuing to use the old two-field
struct in the on-disk format, we just need to do the same for the NSN
field in GiST. Patch attached.
This got me thinking: although the problem is immediately visible when
you try to run queries on an upgraded gist index - you get incorrect
results - the test procedure explained in TESTING file and performed by
tests.h won't catch this.
We're never going to have full coverage of all possible
incompatibilities, but it would be nice to have at least a simple test
suite that creates an index using each indexam, and checks that they
still work after upgrade.
- Heikki
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 95d33c8..dcaa6f3 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -240,7 +240,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
{
/* save old rightlink and NSN */
oldrlink = GistPageGetOpaque(page)->rightlink;
- oldnsn = GistPageGetOpaque(page)->nsn;
+ oldnsn = GistPageGetNSN(page);
dist->buffer = buffer;
dist->block.blkno = BufferGetBlockNumber(buffer);
@@ -364,7 +364,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* F_FOLLOW_RIGHT flags ensure that scans will follow the
* rightlinks until the downlinks are inserted.
*/
- GistPageGetOpaque(ptr->page)->nsn = oldnsn;
+ GistPageSetNSN(ptr->page, oldnsn);
}
START_CRIT_SECTION();
@@ -473,7 +473,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
{
Page leftpg = BufferGetPage(leftchildbuf);
- GistPageGetOpaque(leftpg)->nsn = recptr;
+ GistPageSetNSN(leftpg, recptr);
GistClearFollowRight(leftpg);
PageSetLSN(leftpg, recptr);
@@ -561,7 +561,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
}
if (stack->blkno != GIST_ROOT_BLKNO &&
- stack->parent->lsn < GistPageGetOpaque(stack->page)->nsn)
+ stack->parent->lsn < GistPageGetNSN(stack->page))
{
/*
* Concurrent split detected. There's no guarantee that the
@@ -707,8 +707,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
*/
}
else if (GistFollowRight(stack->page) ||
- stack->parent->lsn <
- GistPageGetOpaque(stack->page)->nsn)
+ stack->parent->lsn < GistPageGetNSN(stack->page))
{
/*
* The page was split while we momentarily unlocked the
@@ -793,7 +792,7 @@ gistFindPath(Relation r, BlockNumber child, OffsetNumber *downlinkoffnum)
if (GistFollowRight(page))
elog(ERROR, "concurrent GiST page split was incomplete");
- if (top->parent && top->parent->lsn < GistPageGetOpaque(page)->nsn &&
+ if (top->parent && top->parent->lsn < PageXLogRecPtrGet(GistPageGetOpaque(page)->nsn) &&
GistPageGetOpaque(page)->rightlink != InvalidBlockNumber /* sanity check */ )
{
/*
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index 79996a2..3300fec 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -263,7 +263,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, double *myDistances,
*/
if (!XLogRecPtrIsInvalid(pageItem->data.parentlsn) &&
(GistFollowRight(page) ||
- pageItem->data.parentlsn < opaque->nsn) &&
+ pageItem->data.parentlsn < GistPageGetNSN(page)) &&
opaque->rightlink != InvalidBlockNumber /* sanity check */ )
{
/* There was a page split, follow right link to add pages */
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index dcad36e..b5be676 100644
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -114,7 +114,7 @@ pushStackIfSplited(Page page, GistBDItem *stack)
GISTPageOpaque opaque = GistPageGetOpaque(page);
if (stack->blkno != GIST_ROOT_BLKNO && !XLogRecPtrIsInvalid(stack->parentlsn) &&
- (GistFollowRight(page) || stack->parentlsn < opaque->nsn) &&
+ (GistFollowRight(page) || stack->parentlsn < GistPageGetNSN(page)) &&
opaque->rightlink != InvalidBlockNumber /* sanity check */ )
{
/* split page detected, install right link to the stack */
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index e3a213e..c18065a 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -66,7 +66,7 @@ gistRedoClearFollowRight(XLogRecPtr lsn, XLogRecord *record, int block_index,
*/
if (lsn >= PageGetLSN(page))
{
- GistPageGetOpaque(page)->nsn = lsn;
+ GistPageSetNSN(page, lsn);
GistClearFollowRight(page);
PageSetLSN(page, lsn);
@@ -271,7 +271,7 @@ gistRedoPageSplitRecord(XLogRecPtr lsn, XLogRecord *record)
if (newpage->header->blkno == GIST_ROOT_BLKNO)
{
GistPageGetOpaque(page)->rightlink = InvalidBlockNumber;
- GistPageGetOpaque(page)->nsn = xldata->orignsn;
+ GistPageSetNSN(page, xldata->orignsn);
GistClearFollowRight(page);
}
else
@@ -280,7 +280,7 @@ gistRedoPageSplitRecord(XLogRecPtr lsn, XLogRecord *record)
GistPageGetOpaque(page)->rightlink = xlrec.page[i + 1].header->blkno;
else
GistPageGetOpaque(page)->rightlink = xldata->origrlink;
- GistPageGetOpaque(page)->nsn = xldata->orignsn;
+ GistPageSetNSN(page, xldata->orignsn);
if (i < xlrec.data->npage - 1 && !isrootsplit &&
xldata->markfollowright)
GistMarkFollowRight(page);
diff --git a/src/include/access/gist.h b/src/include/access/gist.h
index 69e4ec0..c46b4fe 100644
--- a/src/include/access/gist.h
+++ b/src/include/access/gist.h
@@ -64,10 +64,11 @@
#define F_FOLLOW_RIGHT (1 << 3) /* page to the right has no downlink */
typedef XLogRecPtr GistNSN;
+typedef PageXLogRecPtr PageGistNSN;
typedef struct GISTPageOpaqueData
{
- GistNSN nsn; /* this value must change on page split */
+ PageGistNSN nsn; /* this value must change on page split */
BlockNumber rightlink; /* next page if any */
uint16 flags; /* see bit definitions above */
uint16 gist_page_id; /* for identification of GiST indexes */
@@ -137,6 +138,9 @@ typedef struct GISTENTRY
#define GistMarkFollowRight(page) ( GistPageGetOpaque(page)->flags |= F_FOLLOW_RIGHT)
#define GistClearFollowRight(page) ( GistPageGetOpaque(page)->flags &= ~F_FOLLOW_RIGHT)
+#define GistPageGetNSN(page) ( PageXLogRecPtrGet(GistPageGetOpaque(page)->nsn))
+#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)->nsn, val))
+
/*
* Vector of GISTENTRY structs; user-defined methods union and picksplit
* take it as one of their arguments
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 7758407..088945c 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -83,6 +83,24 @@ typedef uint16 LocationIndex;
/*
+ * For historical reasons, the 64-bit LSN value is stored as two 32-bit
+ * values.
+ */
+typedef struct
+{
+ uint32 xlogid; /* high bits */
+ uint32 xrecoff; /* low bits */
+} PageXLogRecPtr;
+
+/*
+ * Additional macros for access to page headers
+ */
+#define PageXLogRecPtrGet(val) \
+ ((uint64) (val).xlogid << 32 | (val).xrecoff)
+#define PageXLogRecPtrSet(ptr, lsn) \
+ ((ptr).xlogid = (uint32) ((lsn) >> 32), (ptr).xrecoff = (uint32) (lsn))
+
+/*
* disk page organization
*
* space management information generic to any page
@@ -120,13 +138,6 @@ typedef uint16 LocationIndex;
* are 15 bits.
*/
-/* for historical reasons, the LSN is stored as two 32-bit values. */
-typedef struct
-{
- uint32 xlogid; /* high bits */
- uint32 xrecoff; /* low bits */
-} PageXLogRecPtr;
-
typedef struct PageHeaderData
{
/* XXX LSN is member of *any* block, not only page-organized ones */
@@ -319,13 +330,13 @@ typedef PageHeaderData *PageHeader;
/ sizeof(ItemIdData)))
/*
- * Additional macros for access to page headers
+ * Additional macros for access to page headers. (Beware multiple evaluation
+ * of the arguments!)
*/
#define PageGetLSN(page) \
- ((uint64) ((PageHeader) (page))->pd_lsn.xlogid << 32 | ((PageHeader) (page))->pd_lsn.xrecoff)
+ PageXLogRecPtrGet(((PageHeader) (page))->pd_lsn)
#define PageSetLSN(page, lsn) \
- (((PageHeader) (page))->pd_lsn.xlogid = (uint32) ((lsn) >> 32), \
- ((PageHeader) (page))->pd_lsn.xrecoff = (uint32) (lsn))
+ PageXLogRecPtrSet(((PageHeader) (page))->pd_lsn, lsn)
/* NOTE: only the 16 least significant bits are stored */
#define PageGetTLI(page) \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
