Robert, all, * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > +1. I was also once confused with these macros. I think this is a > > good cleanup. On a quick look, I don't see any problem with your > > changes. > > One difference between those two macros is that IndexTupleSize > forcibly casts the argument to IndexTuple, which means that you don't > get any type-checking when you use that one. I suggest that in > addition to removing IndexTupleDSize as proposed, we also remove that > cast. It seems that the only place which relies on that cast > currently is btree_xlog_split.
I agree with removing the macro and the force cast that's being done,
but I would have thought to change btree_xlog_split() to declare those
variables as IndexTuple (since that's really what it is, no?) and then
cast the other way when needed, as in the attached.
I'll note that basically every other function in nbtxlog.c operates this
way too, declaring the variable as the appropriate type (not just
'Item') and then casting to that when calling PageGetItem and casting
back when calling PageAddItem().
See btree_xlog_delete_get_latestRemovedXid(),
btree_xlog_mark_page_halfdead(), and btree_xlog_unlink_page().
The only other place where Item is actually declared as a variable is in
_bt_restore_page(), and it looks like it probably makes sense to change
that to IndexTuple too.
Attached is a patch which does that.
Looking further, there's actually only one other place that uses Item as
an actual declared variable (rather than being part of a function
signature and passed in), and that's in RelationPutHeapTuple() and it
looks like it should really be changed:
if (!token)
{
ItemId itemId = PageGetItemId(pageHeader, offnum);
Item item = PageGetItem(pageHeader, itemId);
((HeapTupleHeader) item)->t_ctid = tuple->t_self;
}
So I've attached a seperate patch for that, too.
I'll leave the patch status in 'Needs review' since there's more
changes, but hopefully someone can take a look and we can move this
along, seems like a pretty small and reasonable improvement.
Thanks!
Stephen
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
new file mode 100644
index b38208e..ab5aaff
*** a/src/backend/access/hash/hash_xlog.c
--- b/src/backend/access/hash/hash_xlog.c
*************** hash_xlog_move_page_contents(XLogReaderS
*** 558,564 ****
Size itemsz;
OffsetNumber l;
! itemsz = IndexTupleDSize(*itup);
itemsz = MAXALIGN(itemsz);
data += itemsz;
--- 558,564 ----
Size itemsz;
OffsetNumber l;
! itemsz = IndexTupleSize(itup);
itemsz = MAXALIGN(itemsz);
data += itemsz;
*************** hash_xlog_squeeze_page(XLogReaderState *
*** 686,692 ****
Size itemsz;
OffsetNumber l;
! itemsz = IndexTupleDSize(*itup);
itemsz = MAXALIGN(itemsz);
data += itemsz;
--- 686,692 ----
Size itemsz;
OffsetNumber l;
! itemsz = IndexTupleSize(itup);
itemsz = MAXALIGN(itemsz);
data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
new file mode 100644
index f668dcf..f121286
*** a/src/backend/access/hash/hashinsert.c
--- b/src/backend/access/hash/hashinsert.c
*************** _hash_doinsert(Relation rel, IndexTuple
*** 55,61 ****
hashkey = _hash_get_indextuple_hashkey(itup);
/* compute item size too */
! itemsz = IndexTupleDSize(*itup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
--- 55,61 ----
hashkey = _hash_get_indextuple_hashkey(itup);
/* compute item size too */
! itemsz = IndexTupleSize(itup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
*************** restart_insert:
*** 222,228 ****
XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
--- 222,228 ----
XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
*************** _hash_pgaddmultitup(Relation rel, Buffer
*** 309,315 ****
{
Size itemsize;
! itemsize = IndexTupleDSize(*itups[i]);
itemsize = MAXALIGN(itemsize);
/* Find where to insert the tuple (preserving page's hashkey ordering) */
--- 309,315 ----
{
Size itemsize;
! itemsize = IndexTupleSize(itups[i]);
itemsize = MAXALIGN(itemsize);
/* Find where to insert the tuple (preserving page's hashkey ordering) */
diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
new file mode 100644
index c9de128..d92ecd7
*** a/src/backend/access/hash/hashovfl.c
--- b/src/backend/access/hash/hashovfl.c
*************** readpage:
*** 890,896 ****
itup = (IndexTuple) PageGetItem(rpage,
PageGetItemId(rpage, roffnum));
! itemsz = IndexTupleDSize(*itup);
itemsz = MAXALIGN(itemsz);
/*
--- 890,896 ----
itup = (IndexTuple) PageGetItem(rpage,
PageGetItemId(rpage, roffnum));
! itemsz = IndexTupleSize(itup);
itemsz = MAXALIGN(itemsz);
/*
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
new file mode 100644
index e3c8721..3859e3b
*** a/src/backend/access/hash/hashpage.c
--- b/src/backend/access/hash/hashpage.c
*************** _hash_splitbucket(Relation rel,
*** 1173,1179 ****
* the current page in the new bucket, we must allocate a new
* overflow page and place the tuple on that page instead.
*/
! itemsz = IndexTupleDSize(*new_itup);
itemsz = MAXALIGN(itemsz);
if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_size + itemsz))
--- 1173,1179 ----
* the current page in the new bucket, we must allocate a new
* overflow page and place the tuple on that page instead.
*/
! itemsz = IndexTupleSize(new_itup);
itemsz = MAXALIGN(itemsz);
if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_size + itemsz))
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
new file mode 100644
index 51059c0..2fe9867
*** a/src/backend/access/nbtree/nbtinsert.c
--- b/src/backend/access/nbtree/nbtinsert.c
*************** _bt_findinsertloc(Relation rel,
*** 558,564 ****
lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
! itemsz = IndexTupleDSize(*newtup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
--- 558,564 ----
lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
! itemsz = IndexTupleSize(newtup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
*************** _bt_insertonpg(Relation rel,
*** 755,761 ****
elog(ERROR, "cannot insert to incompletely split page %u",
BufferGetBlockNumber(buf));
! itemsz = IndexTupleDSize(*itup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
--- 755,761 ----
elog(ERROR, "cannot insert to incompletely split page %u",
BufferGetBlockNumber(buf));
! itemsz = IndexTupleSize(itup);
itemsz = MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we
* need to be consistent */
*************** _bt_insertonpg(Relation rel,
*** 914,920 ****
sizeof(IndexTupleData));
}
else
! XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
recptr = XLogInsert(RM_BTREE_ID, xlinfo);
--- 914,920 ----
sizeof(IndexTupleData));
}
else
! XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
recptr = XLogInsert(RM_BTREE_ID, xlinfo);
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
new file mode 100644
index f6159db..4a909ea
*** a/src/backend/access/nbtree/nbtsort.c
--- b/src/backend/access/nbtree/nbtsort.c
*************** _bt_buildadd(BTWriteState *wstate, BTPag
*** 468,474 ****
last_off = state->btps_lastoff;
pgspc = PageGetFreeSpace(npage);
! itupsz = IndexTupleDSize(*itup);
itupsz = MAXALIGN(itupsz);
/*
--- 468,474 ----
last_off = state->btps_lastoff;
pgspc = PageGetFreeSpace(npage);
! itupsz = IndexTupleSize(itup);
itupsz = MAXALIGN(itupsz);
/*
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
new file mode 100644
index bed1dd2..153d079
*** a/src/backend/access/nbtree/nbtxlog.c
--- b/src/backend/access/nbtree/nbtxlog.c
*************** _bt_restore_page(Page page, char *from,
*** 38,44 ****
IndexTupleData itupdata;
Size itemsz;
char *end = from + len;
! Item items[MaxIndexTuplesPerPage];
uint16 itemsizes[MaxIndexTuplesPerPage];
int i;
int nitems;
--- 38,44 ----
IndexTupleData itupdata;
Size itemsz;
char *end = from + len;
! IndexTuple items[MaxIndexTuplesPerPage];
uint16 itemsizes[MaxIndexTuplesPerPage];
int i;
int nitems;
*************** _bt_restore_page(Page page, char *from,
*** 53,62 ****
{
/* Need to copy tuple header due to alignment considerations */
memcpy(&itupdata, from, sizeof(IndexTupleData));
! itemsz = IndexTupleDSize(itupdata);
itemsz = MAXALIGN(itemsz);
! items[i] = (Item) from;
itemsizes[i] = itemsz;
i++;
--- 53,62 ----
{
/* Need to copy tuple header due to alignment considerations */
memcpy(&itupdata, from, sizeof(IndexTupleData));
! itemsz = IndexTupleSize(&itupdata);
itemsz = MAXALIGN(itemsz);
! items[i] = (IndexTuple) from;
itemsizes[i] = itemsz;
i++;
*************** _bt_restore_page(Page page, char *from,
*** 66,72 ****
for (i = nitems - 1; i >= 0; i--)
{
! if (PageAddItem(page, items[i], itemsizes[i], nitems - i,
false, false) == InvalidOffsetNumber)
elog(PANIC, "_bt_restore_page: cannot add item to page");
from += itemsz;
--- 66,72 ----
for (i = nitems - 1; i >= 0; i--)
{
! if (PageAddItem(page, (Item) items[i], itemsizes[i], nitems - i,
false, false) == InvalidOffsetNumber)
elog(PANIC, "_bt_restore_page: cannot add item to page");
from += itemsz;
*************** btree_xlog_split(bool onleft, XLogReader
*** 205,211 ****
BTPageOpaque ropaque;
char *datapos;
Size datalen;
! Item left_hikey = NULL;
Size left_hikeysz = 0;
BlockNumber leftsib;
BlockNumber rightsib;
--- 205,211 ----
BTPageOpaque ropaque;
char *datapos;
Size datalen;
! IndexTuple left_hikey = NULL;
Size left_hikeysz = 0;
BlockNumber leftsib;
BlockNumber rightsib;
*************** btree_xlog_split(bool onleft, XLogReader
*** 248,254 ****
{
ItemId hiItemId = PageGetItemId(rpage, P_FIRSTDATAKEY(ropaque));
! left_hikey = PageGetItem(rpage, hiItemId);
left_hikeysz = ItemIdGetLength(hiItemId);
}
--- 248,254 ----
{
ItemId hiItemId = PageGetItemId(rpage, P_FIRSTDATAKEY(ropaque));
! left_hikey = (IndexTuple) PageGetItem(rpage, hiItemId);
left_hikeysz = ItemIdGetLength(hiItemId);
}
*************** btree_xlog_split(bool onleft, XLogReader
*** 272,278 ****
Page lpage = (Page) BufferGetPage(lbuf);
BTPageOpaque lopaque = (BTPageOpaque) PageGetSpecialPointer(lpage);
OffsetNumber off;
! Item newitem = NULL;
Size newitemsz = 0;
Page newlpage;
OffsetNumber leftoff;
--- 272,278 ----
Page lpage = (Page) BufferGetPage(lbuf);
BTPageOpaque lopaque = (BTPageOpaque) PageGetSpecialPointer(lpage);
OffsetNumber off;
! IndexTuple newitem = NULL;
Size newitemsz = 0;
Page newlpage;
OffsetNumber leftoff;
*************** btree_xlog_split(bool onleft, XLogReader
*** 281,287 ****
if (onleft)
{
! newitem = (Item) datapos;
newitemsz = MAXALIGN(IndexTupleSize(newitem));
datapos += newitemsz;
datalen -= newitemsz;
--- 281,287 ----
if (onleft)
{
! newitem = (IndexTuple) datapos;
newitemsz = MAXALIGN(IndexTupleSize(newitem));
datapos += newitemsz;
datalen -= newitemsz;
*************** btree_xlog_split(bool onleft, XLogReader
*** 290,296 ****
/* Extract left hikey and its size (assuming 16-bit alignment) */
if (!isleaf)
{
! left_hikey = (Item) datapos;
left_hikeysz = MAXALIGN(IndexTupleSize(left_hikey));
datapos += left_hikeysz;
datalen -= left_hikeysz;
--- 290,296 ----
/* Extract left hikey and its size (assuming 16-bit alignment) */
if (!isleaf)
{
! left_hikey = (IndexTuple) datapos;
left_hikeysz = MAXALIGN(IndexTupleSize(left_hikey));
datapos += left_hikeysz;
datalen -= left_hikeysz;
*************** btree_xlog_split(bool onleft, XLogReader
*** 301,307 ****
/* Set high key */
leftoff = P_HIKEY;
! if (PageAddItem(newlpage, left_hikey, left_hikeysz,
P_HIKEY, false, false) == InvalidOffsetNumber)
elog(PANIC, "failed to add high key to left page after split");
leftoff = OffsetNumberNext(leftoff);
--- 301,307 ----
/* Set high key */
leftoff = P_HIKEY;
! if (PageAddItem(newlpage, (Item) left_hikey, left_hikeysz,
P_HIKEY, false, false) == InvalidOffsetNumber)
elog(PANIC, "failed to add high key to left page after split");
leftoff = OffsetNumberNext(leftoff);
*************** btree_xlog_split(bool onleft, XLogReader
*** 310,321 ****
{
ItemId itemid;
Size itemsz;
! Item item;
/* add the new item if it was inserted on left page */
if (onleft && off == xlrec->newitemoff)
{
! if (PageAddItem(newlpage, newitem, newitemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add new item to left page after split");
leftoff = OffsetNumberNext(leftoff);
--- 310,321 ----
{
ItemId itemid;
Size itemsz;
! IndexTuple item;
/* add the new item if it was inserted on left page */
if (onleft && off == xlrec->newitemoff)
{
! if (PageAddItem(newlpage, (Item) newitem, newitemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add new item to left page after split");
leftoff = OffsetNumberNext(leftoff);
*************** btree_xlog_split(bool onleft, XLogReader
*** 323,330 ****
itemid = PageGetItemId(lpage, off);
itemsz = ItemIdGetLength(itemid);
! item = PageGetItem(lpage, itemid);
! if (PageAddItem(newlpage, item, itemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add old item to left page after split");
leftoff = OffsetNumberNext(leftoff);
--- 323,330 ----
itemid = PageGetItemId(lpage, off);
itemsz = ItemIdGetLength(itemid);
! item = (IndexTuple) PageGetItem(lpage, itemid);
! if (PageAddItem(newlpage, (Item) item, itemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add old item to left page after split");
leftoff = OffsetNumberNext(leftoff);
*************** btree_xlog_split(bool onleft, XLogReader
*** 333,339 ****
/* cope with possibility that newitem goes at the end */
if (onleft && off == xlrec->newitemoff)
{
! if (PageAddItem(newlpage, newitem, newitemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add new item to left page after split");
leftoff = OffsetNumberNext(leftoff);
--- 333,339 ----
/* cope with possibility that newitem goes at the end */
if (onleft && off == xlrec->newitemoff)
{
! if (PageAddItem(newlpage, (Item) newitem, newitemsz, leftoff,
false, false) == InvalidOffsetNumber)
elog(ERROR, "failed to add new item to left page after split");
leftoff = OffsetNumberNext(leftoff);
diff --git a/src/include/access/itup.h b/src/include/access/itup.h
new file mode 100644
index 0ffa91d..9be3442
*** a/src/include/access/itup.h
--- b/src/include/access/itup.h
*************** typedef IndexAttributeBitMapData * Index
*** 67,74 ****
#define INDEX_VAR_MASK 0x4000
#define INDEX_NULL_MASK 0x8000
! #define IndexTupleSize(itup) ((Size) (((IndexTuple) (itup))->t_info & INDEX_SIZE_MASK))
! #define IndexTupleDSize(itup) ((Size) ((itup).t_info & INDEX_SIZE_MASK))
#define IndexTupleHasNulls(itup) ((((IndexTuple) (itup))->t_info & INDEX_NULL_MASK))
#define IndexTupleHasVarwidths(itup) ((((IndexTuple) (itup))->t_info & INDEX_VAR_MASK))
--- 67,73 ----
#define INDEX_VAR_MASK 0x4000
#define INDEX_NULL_MASK 0x8000
! #define IndexTupleSize(itup) ((Size) ((itup)->t_info & INDEX_SIZE_MASK))
#define IndexTupleHasNulls(itup) ((((IndexTuple) (itup))->t_info & INDEX_NULL_MASK))
#define IndexTupleHasVarwidths(itup) ((((IndexTuple) (itup))->t_info & INDEX_VAR_MASK))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
new file mode 100644
index 0d7bc68..179f78e
*** a/src/backend/access/heap/hio.c
--- b/src/backend/access/heap/hio.c
*************** RelationPutHeapTuple(Relation relation,
*** 66,75 ****
*/
if (!token)
{
! ItemId itemId = PageGetItemId(pageHeader, offnum);
! Item item = PageGetItem(pageHeader, itemId);
! ((HeapTupleHeader) item)->t_ctid = tuple->t_self;
}
}
--- 66,75 ----
*/
if (!token)
{
! ItemId itemId = PageGetItemId(pageHeader, offnum);
! HeapTupleHeader item = (HeapTupleHeader) PageGetItem(pageHeader, itemId);
! item->t_ctid = tuple->t_self;
}
}
signature.asc
Description: PGP signature
