From ce96ce60ca2293f75f36c3661e4657a3c79ffd61 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 28 Mar 2017 12:52:55 -0300 Subject: [PATCH] Remove direct uses of ItemPointer.{ip_blkid,ip_posid} There are no functional changes here; this simply encapsulates knowledge of the ItemPointerData struct so that a future patch can change things without more breakage. All direct users of ip_blkid and ip_posid are changed to use existing macros ItemPointerGetBlockNumber and ItemPointerGetOffsetNumber respectively. For callers where that's inappropriate (because they Assert that the itempointer is is valid-looking), add ItemPointerGetBlockNumberNoCheck and ItemPointerGetOffsetNumberNoCheck, which lack the assertion but are otherwise identical. Author: Pavan Deolasee Discussion: https://postgr.es/m/CABOikdNnFon4cJiL=h1mZH3bgUeU+sWHuU4Yr8AB=j3A2p1GiA@mail.gmail.com --- contrib/pageinspect/btreefuncs.c | 4 +-- contrib/pgstattuple/pgstattuple.c | 2 +- src/backend/access/gin/ginget.c | 29 +++++++++++-------- src/backend/access/gin/ginpostinglist.c | 14 ++++----- .../replication/logical/reorderbuffer.c | 4 +-- src/backend/storage/page/itemptr.c | 15 +++++----- src/backend/utils/adt/tid.c | 17 ++++------- src/include/access/gin_private.h | 6 ++-- src/include/access/ginblock.h | 11 +++++-- src/include/access/htup_details.h | 2 +- src/include/access/nbtree.h | 5 ++-- src/include/storage/itemptr.h | 28 ++++++++++++++---- 12 files changed, 79 insertions(+), 58 deletions(-) diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c index 6f35e288fd..3d69aa9d37 100644 --- a/contrib/pageinspect/btreefuncs.c +++ b/contrib/pageinspect/btreefuncs.c @@ -363,8 +363,8 @@ bt_page_items(PG_FUNCTION_ARGS) j = 0; values[j++] = psprintf("%d", uargs->offset); values[j++] = psprintf("(%u,%u)", - BlockIdGetBlockNumber(&(itup->t_tid.ip_blkid)), - itup->t_tid.ip_posid); + ItemPointerGetBlockNumberNoCheck(&itup->t_tid), + ItemPointerGetOffsetNumberNoCheck(&itup->t_tid)); values[j++] = psprintf("%d", (int) IndexTupleSize(itup)); values[j++] = psprintf("%c", IndexTupleHasNulls(itup) ? 't' : 'f'); values[j++] = psprintf("%c", IndexTupleHasVarwidths(itup) ? 't' : 'f'); diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c index 1e0de5d660..44f90cd0d3 100644 --- a/contrib/pgstattuple/pgstattuple.c +++ b/contrib/pgstattuple/pgstattuple.c @@ -356,7 +356,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo) * heap_getnext may find no tuples on a given page, so we cannot * simply examine the pages returned by the heap scan. */ - tupblock = BlockIdGetBlockNumber(&tuple->t_self.ip_blkid); + tupblock = ItemPointerGetBlockNumber(&tuple->t_self); while (block <= tupblock) { diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c index 87cd9eaa9f..610d386ff8 100644 --- a/src/backend/access/gin/ginget.c +++ b/src/backend/access/gin/ginget.c @@ -626,8 +626,9 @@ entryLoadMoreItems(GinState *ginstate, GinScanEntry entry, } else { - entry->btree.itemptr = advancePast; - entry->btree.itemptr.ip_posid++; + ItemPointerSet(&entry->btree.itemptr, + GinItemPointerGetBlockNumber(&advancePast), + OffsetNumberNext(GinItemPointerGetOffsetNumber(&advancePast))); } entry->btree.fullScan = false; stack = ginFindLeafPage(&entry->btree, true, snapshot); @@ -979,15 +980,17 @@ keyGetItem(GinState *ginstate, MemoryContext tempCtx, GinScanKey key, if (GinItemPointerGetBlockNumber(&advancePast) < GinItemPointerGetBlockNumber(&minItem)) { - advancePast.ip_blkid = minItem.ip_blkid; - advancePast.ip_posid = 0; + ItemPointerSet(&advancePast, + GinItemPointerGetBlockNumber(&minItem), + InvalidOffsetNumber); } } else { - Assert(minItem.ip_posid > 0); - advancePast = minItem; - advancePast.ip_posid--; + Assert(GinItemPointerGetOffsetNumber(&minItem) > 0); + ItemPointerSet(&advancePast, + GinItemPointerGetBlockNumber(&minItem), + OffsetNumberPrev(GinItemPointerGetOffsetNumber(&minItem))); } /* @@ -1245,15 +1248,17 @@ scanGetItem(IndexScanDesc scan, ItemPointerData advancePast, if (GinItemPointerGetBlockNumber(&advancePast) < GinItemPointerGetBlockNumber(&key->curItem)) { - advancePast.ip_blkid = key->curItem.ip_blkid; - advancePast.ip_posid = 0; + ItemPointerSet(&advancePast, + GinItemPointerGetBlockNumber(&key->curItem), + InvalidOffsetNumber); } } else { - Assert(key->curItem.ip_posid > 0); - advancePast = key->curItem; - advancePast.ip_posid--; + Assert(GinItemPointerGetOffsetNumber(&key->curItem) > 0); + ItemPointerSet(&advancePast, + GinItemPointerGetBlockNumber(&key->curItem), + OffsetNumberPrev(GinItemPointerGetOffsetNumber(&key->curItem))); } /* diff --git a/src/backend/access/gin/ginpostinglist.c b/src/backend/access/gin/ginpostinglist.c index 598069d06f..8d2d31ac72 100644 --- a/src/backend/access/gin/ginpostinglist.c +++ b/src/backend/access/gin/ginpostinglist.c @@ -79,13 +79,11 @@ itemptr_to_uint64(const ItemPointer iptr) uint64 val; Assert(ItemPointerIsValid(iptr)); - Assert(iptr->ip_posid < (1 << MaxHeapTuplesPerPageBits)); + Assert(GinItemPointerGetOffsetNumber(iptr) < (1 << MaxHeapTuplesPerPageBits)); - val = iptr->ip_blkid.bi_hi; - val <<= 16; - val |= iptr->ip_blkid.bi_lo; + val = GinItemPointerGetBlockNumber(iptr); val <<= MaxHeapTuplesPerPageBits; - val |= iptr->ip_posid; + val |= GinItemPointerGetOffsetNumber(iptr); return val; } @@ -93,11 +91,9 @@ itemptr_to_uint64(const ItemPointer iptr) static inline void uint64_to_itemptr(uint64 val, ItemPointer iptr) { - iptr->ip_posid = val & ((1 << MaxHeapTuplesPerPageBits) - 1); + GinItemPointerSetOffsetNumber(iptr, val & ((1 << MaxHeapTuplesPerPageBits) - 1)); val = val >> MaxHeapTuplesPerPageBits; - iptr->ip_blkid.bi_lo = val & 0xFFFF; - val = val >> 16; - iptr->ip_blkid.bi_hi = val & 0xFFFF; + GinItemPointerSetBlockNumber(iptr, val); Assert(ItemPointerIsValid(iptr)); } diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index b437799c5f..12ebadca81 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -3013,8 +3013,8 @@ DisplayMapping(HTAB *tuplecid_data) ent->key.relnode.dbNode, ent->key.relnode.spcNode, ent->key.relnode.relNode, - BlockIdGetBlockNumber(&ent->key.tid.ip_blkid), - ent->key.tid.ip_posid, + ItemPointerGetBlockNumber(&ent->key.tid), + ItemPointerGetOffsetNumber(&ent->key.tid), ent->cmin, ent->cmax ); diff --git a/src/backend/storage/page/itemptr.c b/src/backend/storage/page/itemptr.c index 703cbb9c39..b872928f2f 100644 --- a/src/backend/storage/page/itemptr.c +++ b/src/backend/storage/page/itemptr.c @@ -52,20 +52,21 @@ int32 ItemPointerCompare(ItemPointer arg1, ItemPointer arg2) { /* - * Don't use ItemPointerGetBlockNumber or ItemPointerGetOffsetNumber here, - * because they assert ip_posid != 0 which might not be true for a - * user-supplied TID. + * Use ItemPointerGet{Offset,Block}NumberNoCheck to avoid asserting + * ip_posid != 0, which may not be true for a user-supplied TID. */ - BlockNumber b1 = BlockIdGetBlockNumber(&(arg1->ip_blkid)); - BlockNumber b2 = BlockIdGetBlockNumber(&(arg2->ip_blkid)); + BlockNumber b1 = ItemPointerGetBlockNumberNoCheck(arg1); + BlockNumber b2 = ItemPointerGetBlockNumberNoCheck(arg2); if (b1 < b2) return -1; else if (b1 > b2) return 1; - else if (arg1->ip_posid < arg2->ip_posid) + else if (ItemPointerGetOffsetNumberNoCheck(arg1) < + ItemPointerGetOffsetNumberNoCheck(arg2)) return -1; - else if (arg1->ip_posid > arg2->ip_posid) + else if (ItemPointerGetOffsetNumberNoCheck(arg1) > + ItemPointerGetOffsetNumberNoCheck(arg2)) return 1; else return 0; diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c index 49a5a157b9..8453b65e78 100644 --- a/src/backend/utils/adt/tid.c +++ b/src/backend/utils/adt/tid.c @@ -109,8 +109,8 @@ tidout(PG_FUNCTION_ARGS) OffsetNumber offsetNumber; char buf[32]; - blockNumber = BlockIdGetBlockNumber(&(itemPtr->ip_blkid)); - offsetNumber = itemPtr->ip_posid; + blockNumber = ItemPointerGetBlockNumberNoCheck(itemPtr); + offsetNumber = ItemPointerGetOffsetNumberNoCheck(itemPtr); /* Perhaps someday we should output this as a record. */ snprintf(buf, sizeof(buf), "(%u,%u)", blockNumber, offsetNumber); @@ -146,18 +146,13 @@ Datum tidsend(PG_FUNCTION_ARGS) { ItemPointer itemPtr = PG_GETARG_ITEMPOINTER(0); - BlockId blockId; - BlockNumber blockNumber; - OffsetNumber offsetNumber; StringInfoData buf; - blockId = &(itemPtr->ip_blkid); - blockNumber = BlockIdGetBlockNumber(blockId); - offsetNumber = itemPtr->ip_posid; - pq_begintypsend(&buf); - pq_sendint(&buf, blockNumber, sizeof(blockNumber)); - pq_sendint(&buf, offsetNumber, sizeof(offsetNumber)); + pq_sendint(&buf, ItemPointerGetBlockNumberNoCheck(itemPtr), + sizeof(BlockNumber)); + pq_sendint(&buf, ItemPointerGetOffsetNumberNoCheck(itemPtr), + sizeof(OffsetNumber)); PG_RETURN_BYTEA_P(pq_endtypsend(&buf)); } diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h index 824cc1c9d8..f2e9c4ddec 100644 --- a/src/include/access/gin_private.h +++ b/src/include/access/gin_private.h @@ -460,8 +460,8 @@ extern ItemPointer ginMergeItemPointers(ItemPointerData *a, uint32 na, static inline int ginCompareItemPointers(ItemPointer a, ItemPointer b) { - uint64 ia = (uint64) a->ip_blkid.bi_hi << 32 | (uint64) a->ip_blkid.bi_lo << 16 | a->ip_posid; - uint64 ib = (uint64) b->ip_blkid.bi_hi << 32 | (uint64) b->ip_blkid.bi_lo << 16 | b->ip_posid; + uint64 ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a); + uint64 ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b); if (ia == ib) return 0; @@ -471,6 +471,6 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b) return -1; } -extern int ginTraverseLock(Buffer buffer, bool searchMode); +extern int ginTraverseLock(Buffer buffer, bool searchMode); #endif /* GIN_PRIVATE_H */ diff --git a/src/include/access/ginblock.h b/src/include/access/ginblock.h index a3fb0560dd..438912c6a0 100644 --- a/src/include/access/ginblock.h +++ b/src/include/access/ginblock.h @@ -132,10 +132,17 @@ typedef struct GinMetaPageData * to avoid Asserts, since sometimes the ip_posid isn't "valid" */ #define GinItemPointerGetBlockNumber(pointer) \ - BlockIdGetBlockNumber(&(pointer)->ip_blkid) + (ItemPointerGetBlockNumberNoCheck(pointer)) #define GinItemPointerGetOffsetNumber(pointer) \ - ((pointer)->ip_posid) + (ItemPointerGetOffsetNumberNoCheck(pointer)) + +#define GinItemPointerSetBlockNumber(pointer, blkno) \ + (ItemPointerSetBlockNumber((pointer), (blkno))) + +#define GinItemPointerSetOffsetNumber(pointer, offnum) \ + (ItemPointerSetOffsetNumber((pointer), (offnum))) + /* * Special-case item pointer values needed by the GIN search logic. diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h index a6c7e319a6..7b6285df13 100644 --- a/src/include/access/htup_details.h +++ b/src/include/access/htup_details.h @@ -422,7 +422,7 @@ do { \ #define HeapTupleHeaderIsSpeculative(tup) \ ( \ - (tup)->t_ctid.ip_posid == SpecTokenOffsetNumber \ + (ItemPointerGetOffsetNumberNoCheck(&(tup)->t_ctid) == SpecTokenOffsetNumber) \ ) #define HeapTupleHeaderGetSpeculativeToken(tup) \ diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index 6289ffa9bd..f9304dbe1d 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -151,9 +151,8 @@ typedef struct BTMetaPageData * within a level). - vadim 04/09/97 */ #define BTTidSame(i1, i2) \ - ( (i1).ip_blkid.bi_hi == (i2).ip_blkid.bi_hi && \ - (i1).ip_blkid.bi_lo == (i2).ip_blkid.bi_lo && \ - (i1).ip_posid == (i2).ip_posid ) + ((ItemPointerGetBlockNumber(&(i1)) == ItemPointerGetBlockNumber(&(i2))) && \ + (ItemPointerGetOffsetNumber(&(i1)) == ItemPointerGetOffsetNumber(&(i2)))) #define BTEntrySame(i1, i2) \ BTTidSame((i1)->t_tid, (i2)->t_tid) diff --git a/src/include/storage/itemptr.h b/src/include/storage/itemptr.h index 576aaa890b..c21d2adfcb 100644 --- a/src/include/storage/itemptr.h +++ b/src/include/storage/itemptr.h @@ -60,23 +60,41 @@ typedef ItemPointerData *ItemPointer; ((bool) (PointerIsValid(pointer) && ((pointer)->ip_posid != 0))) /* - * ItemPointerGetBlockNumber + * ItemPointerGetBlockNumberNoCheck * Returns the block number of a disk item pointer. */ -#define ItemPointerGetBlockNumber(pointer) \ +#define ItemPointerGetBlockNumberNoCheck(pointer) \ ( \ - AssertMacro(ItemPointerIsValid(pointer)), \ BlockIdGetBlockNumber(&(pointer)->ip_blkid) \ ) /* - * ItemPointerGetOffsetNumber + * ItemPointerGetBlockNumber + * As above, but verifies that the item pointer looks valid. + */ +#define ItemPointerGetBlockNumber(pointer) \ +( \ + AssertMacro(ItemPointerIsValid(pointer)), \ + ItemPointerGetBlockNumberNoCheck(pointer) \ +) + +/* + * ItemPointerGetOffsetNumberNoCheck * Returns the offset number of a disk item pointer. */ +#define ItemPointerGetOffsetNumberNoCheck(pointer) \ +( \ + (pointer)->ip_posid \ +) + +/* + * ItemPointerGetOffsetNumber + * As above, but verifies that the item pointer looks valid. + */ #define ItemPointerGetOffsetNumber(pointer) \ ( \ AssertMacro(ItemPointerIsValid(pointer)), \ - (pointer)->ip_posid \ + ItemPointerGetOffsetNumberNoCheck(pointer) \ ) /*