From d03e6be539c51d98a6ec0bc90a063edc16b1b202 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 15 Jul 2019 17:51:03 -0700 Subject: [PATCH v2] Fix nbtree metapage cache upgrade bug. It's not okay to update a cached copy of an nbtree index's metapage metadata at a point where it isn't also being updated on disk. Commit 0a64b45152b seems to have gotten this wrong. This is almost a straight revert of commit 0a64b45152b, except the changes to contrib/pageinspect remain, and the failing assertions are updated to deal with the possibility that an index might be pg_upgrade'd from an earlier version. Discussion: https://postgr.es/m/CAH2-Wzmj6pz98qZ6+Ro-=tHvyBJ6q0yxHV8QLOr6O0mE20Nw9Q@mail.gmail.com --- src/backend/access/nbtree/nbtpage.c | 70 ++++++++--------------------- src/include/access/nbtree.h | 2 +- 2 files changed, 20 insertions(+), 52 deletions(-) diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 50455db9af..5cf7a3f6d9 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -33,7 +33,6 @@ #include "storage/predicate.h" #include "utils/snapmgr.h" -static void _bt_cachemetadata(Relation rel, BTMetaPageData *input); static BTMetaPageData *_bt_getmeta(Relation rel, Buffer metabuf); static bool _bt_mark_page_halfdead(Relation rel, Buffer buf, BTStack stack); static bool _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, @@ -109,45 +108,6 @@ _bt_upgrademetapage(Page page) ((char *) metad + sizeof(BTMetaPageData)) - (char *) page; } -/* - * Cache metadata from input meta page to rel->rd_amcache. - */ -static void -_bt_cachemetadata(Relation rel, BTMetaPageData *input) -{ - BTMetaPageData *cached_metad; - - /* We assume rel->rd_amcache was already freed by caller */ - Assert(rel->rd_amcache == NULL); - rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt, - sizeof(BTMetaPageData)); - - /* Meta page should be of supported version */ - Assert(input->btm_version >= BTREE_MIN_VERSION && - input->btm_version <= BTREE_VERSION); - - cached_metad = (BTMetaPageData *) rel->rd_amcache; - if (input->btm_version >= BTREE_NOVAC_VERSION) - { - /* Version with compatible meta-data, no need to upgrade */ - memcpy(cached_metad, input, sizeof(BTMetaPageData)); - } - else - { - /* - * Upgrade meta-data: copy available information from meta-page and - * fill new fields with default values. - * - * Note that we cannot upgrade to version 4+ without a REINDEX, since - * extensive on-disk changes are required. - */ - memcpy(cached_metad, input, offsetof(BTMetaPageData, btm_oldest_btpo_xact)); - cached_metad->btm_version = BTREE_NOVAC_VERSION; - cached_metad->btm_oldest_btpo_xact = InvalidTransactionId; - cached_metad->btm_last_cleanup_num_heap_tuples = -1.0; - } -} - /* * Get metadata from share-locked buffer containing metapage, while performing * standard sanity checks. Sanity checks here must match _bt_getroot(). @@ -483,7 +443,9 @@ _bt_getroot(Relation rel, int access) /* * Cache the metapage data for next time */ - _bt_cachemetadata(rel, metad); + rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt, + sizeof(BTMetaPageData)); + memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData)); /* * We are done with the metapage; arrange to release it via first @@ -650,6 +612,7 @@ _bt_getrootheight(Relation rel) */ if (metad->btm_root == P_NONE) { + Assert(metad->btm_version >= BTREE_MIN_VERSION); _bt_relbuf(rel, metabuf); return 0; } @@ -657,16 +620,18 @@ _bt_getrootheight(Relation rel) /* * Cache the metapage data for next time */ - _bt_cachemetadata(rel, metad); - /* We shouldn't have cached it if any of these fail */ - Assert(metad->btm_magic == BTREE_MAGIC); - Assert(metad->btm_version >= BTREE_NOVAC_VERSION); - Assert(metad->btm_fastroot != P_NONE); + rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt, + sizeof(BTMetaPageData)); + memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData)); _bt_relbuf(rel, metabuf); } /* Get cached page */ metad = (BTMetaPageData *) rel->rd_amcache; + /* We shouldn't have cached it if any of these fail */ + Assert(metad->btm_magic == BTREE_MAGIC); + Assert(metad->btm_version >= BTREE_MIN_VERSION); + Assert(metad->btm_fastroot != P_NONE); return metad->btm_fastlevel; } @@ -701,6 +666,7 @@ _bt_heapkeyspace(Relation rel) { uint32 btm_version = metad->btm_version; + Assert(metad->btm_version >= BTREE_MIN_VERSION); _bt_relbuf(rel, metabuf); return btm_version > BTREE_NOVAC_VERSION; } @@ -708,16 +674,18 @@ _bt_heapkeyspace(Relation rel) /* * Cache the metapage data for next time */ - _bt_cachemetadata(rel, metad); - /* We shouldn't have cached it if any of these fail */ - Assert(metad->btm_magic == BTREE_MAGIC); - Assert(metad->btm_version >= BTREE_NOVAC_VERSION); - Assert(metad->btm_fastroot != P_NONE); + rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt, + sizeof(BTMetaPageData)); + memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData)); _bt_relbuf(rel, metabuf); } /* Get cached page */ metad = (BTMetaPageData *) rel->rd_amcache; + /* We shouldn't have cached it if any of these fail */ + Assert(metad->btm_magic == BTREE_MAGIC); + Assert(metad->btm_version >= BTREE_MIN_VERSION); + Assert(metad->btm_fastroot != P_NONE); return metad->btm_version > BTREE_NOVAC_VERSION; } diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index a3583f225b..1b312311ea 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -97,7 +97,7 @@ typedef BTPageOpaqueData *BTPageOpaque; typedef struct BTMetaPageData { uint32 btm_magic; /* should contain BTREE_MAGIC */ - uint32 btm_version; /* should contain BTREE_VERSION */ + uint32 btm_version; /* nbtree version (always <= BTREE_VERSION) */ BlockNumber btm_root; /* current root location */ uint32 btm_level; /* tree level of the root page */ BlockNumber btm_fastroot; /* current "fast" root location */ -- 2.17.1