Cleanup: avoid direct use of ip_posid/ip_blkid
Hello All,
I would like to propose the attached patch which removes all direct
references to ip_posid and ip_blkid members of ItemPointerData struct and
instead use ItemPointerGetOffsetNumber and ItemPointerGetBlockNumber macros
respectively to access these members.
My motivation to do this is to try to free-up some bits from ip_posid
field, given that it can only be maximum 13-bits long. But even without
that, it seems like a good cleanup to me.
One reason why these macros are not always used is because they typically
do assert-validation to ensure ip_posid has a valid value. There are a few
places in code, especially in GIN and also when we are dealing with
user-supplied TIDs when we might get a TID with invalid ip_posid. I've
handled that by defining and using separate macros which skip the
validation. This doesn't seem any worse than what we are already doing.
make check-world with --enable-tap-tests passes.
Comments/suggestions?
Thanks,
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
remove_ip_posid_blkid_ref_v3.patchapplication/octet-stream; name=remove_ip_posid_blkid_ref_v3.patchDownload
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index d50ec3a..2ec265e 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 06a1992..e65040d 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -353,7 +353,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 60f005c..b4e9fec 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 598069d..8d2d31a 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 7dc97fa..152be87 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3048,8 +3048,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 703cbb9..28ac885 100644
--- a/src/backend/storage/page/itemptr.c
+++ b/src/backend/storage/page/itemptr.c
@@ -54,18 +54,21 @@ 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.
+ * user-supplied TID. Instead we use ItemPointerGetBlockNumberNoCheck and
+ * ItemPointerGetOffsetNumberNoCheck which do not do any validation.
*/
- 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 a3b372f..735c006 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,14 +146,12 @@ 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;
+ blockNumber = ItemPointerGetBlockNumberNoCheck(itemPtr);
+ offsetNumber = ItemPointerGetOffsetNumberNoCheck(itemPtr);
pq_begintypsend(&buf);
pq_sendint(&buf, blockNumber, sizeof(blockNumber));
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 34e7339..2fd4479 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;
diff --git a/src/include/access/ginblock.h b/src/include/access/ginblock.h
index a3fb056..438912c 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 a6c7e31..7b6285d 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 6289ffa..f9304db 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 576aaa8..60d0070 100644
--- a/src/include/storage/itemptr.h
+++ b/src/include/storage/itemptr.h
@@ -69,6 +69,12 @@ typedef ItemPointerData *ItemPointer;
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
)
+/* Same as ItemPointerGetBlockNumber but without any assert-checks */
+#define ItemPointerGetBlockNumberNoCheck(pointer) \
+( \
+ BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
+)
+
/*
* ItemPointerGetOffsetNumber
* Returns the offset number of a disk item pointer.
@@ -79,6 +85,12 @@ typedef ItemPointerData *ItemPointer;
(pointer)->ip_posid \
)
+/* Same as ItemPointerGetOffsetNumber but without any assert-checks */
+#define ItemPointerGetOffsetNumberNoCheck(pointer) \
+( \
+ (pointer)->ip_posid \
+)
+
/*
* ItemPointerSet
* Sets a disk item pointer to the specified block and offset.
On 2/22/17 08:38, Pavan Deolasee wrote:
One reason why these macros are not always used is because they
typically do assert-validation to ensure ip_posid has a valid value.
There are a few places in code, especially in GIN and also when we are
dealing with user-supplied TIDs when we might get a TID with invalid
ip_posid. I've handled that by defining and using separate macros which
skip the validation. This doesn't seem any worse than what we are
already doing.
I wonder why we allow that. Shouldn't the tid type reject input that
has ip_posid == 0?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 2, 2017 at 8:25 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
I wonder why we allow that. Shouldn't the tid type reject input that
has ip_posid == 0?
InvalidOffsetNumber (that is, 0) is something that I wouldn't like to
bet doesn't make it out to disk at some point. I know that we use 1 as
a meaningless placeholder value for internal B-Tree pages. P_HIKEY is
where I get 1 from, which might as well be any other value in
practice, I think -- we only need an item pointer to point to a block
from an internal page. SpecTokenOffsetNumber can certainly make it to
disk, and that is invalid in some sense, even if it isn't actually set
to InvalidOffsetNumber. So, seems pretty risky to me.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 2, 2017 at 9:55 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 2/22/17 08:38, Pavan Deolasee wrote:
One reason why these macros are not always used is because they
typically do assert-validation to ensure ip_posid has a valid value.
There are a few places in code, especially in GIN and also when we are
dealing with user-supplied TIDs when we might get a TID with invalid
ip_posid. I've handled that by defining and using separate macros which
skip the validation. This doesn't seem any worse than what we are
already doing.I wonder why we allow that. Shouldn't the tid type reject input that
has ip_posid == 0?
Yes, I think it seems sensible to disallow InvalidOffsetNumber (or >
MaxOffsetNumber) in user-supplied value. But there are places in GIN and
with INSERT ON CONFLICT where we seem to use special values in ip_posid to
mean different things. So we might still need some way to accept invalid
values there.
Thanks,
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services