IndexTupleDSize macro seems redundant

Started by Ildar Musinabout 8 years ago15 messages

i.musin@postgrespro.ru

about 8 years ago

1 attachment(s)

Hi all,

While I was looking through the indexes code I got confused by couple of
macros - IndexTupleSize() and IndexTupleDSize() - which seem to do the
same thing with only difference that the first one takes pointer as an
argument while the second one takes struct. And in most cases
IndexTupleDSize() is used with dereferencing of index tuple where
IndexTupleSize() would suit perfectly. Is there a particular reason to
have them both? I've made a patch that removes IndexTupleDSize macro.
All the tests have passed.

--
Ildar Musin
i.musin@postgrespro.ru

Attachments:

IndexTupleDSize.patchtext/x-patch; name=IndexTupleDSize.patchDownload

diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index f19f6fd..a133143 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -558,7 +558,7 @@ hash_xlog_move_page_contents(XLogReaderState *record)
 				Size		itemsz;
 				OffsetNumber l;
 
-				itemsz = IndexTupleDSize(*itup);
+				itemsz = IndexTupleSize(itup);
 				itemsz = MAXALIGN(itemsz);
 
 				data += itemsz;
@@ -686,7 +686,7 @@ hash_xlog_squeeze_page(XLogReaderState *record)
 				Size		itemsz;
 				OffsetNumber l;
 
-				itemsz = IndexTupleDSize(*itup);
+				itemsz = IndexTupleSize(itup);
 				itemsz = MAXALIGN(itemsz);
 
 				data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index dc08db9..b90d417 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -55,7 +55,7 @@ _hash_doinsert(Relation rel, IndexTuple itup, Relation heapRel)
 	hashkey = _hash_get_indextuple_hashkey(itup);
 
 	/* compute item size too */
-	itemsz = IndexTupleDSize(*itup);
+	itemsz = IndexTupleSize(itup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -222,7 +222,7 @@ restart_insert:
 		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
 
 		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
-		XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
+		XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
 
 		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
 
@@ -309,7 +309,7 @@ _hash_pgaddmultitup(Relation rel, Buffer buf, IndexTuple *itups,
 	{
 		Size		itemsize;
 
-		itemsize = IndexTupleDSize(*itups[i]);
+		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
index c206e70..b5ec0f6 100644
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -890,7 +890,7 @@ readpage:
 
 			itup = (IndexTuple) PageGetItem(rpage,
 											PageGetItemId(rpage, roffnum));
-			itemsz = IndexTupleDSize(*itup);
+			itemsz = IndexTupleSize(itup);
 			itemsz = MAXALIGN(itemsz);
 
 			/*
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index a50e35d..51100e0 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -1173,7 +1173,7 @@ _hash_splitbucket(Relation rel,
 				 * 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 = 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
index 310589d..b39090a 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -558,7 +558,7 @@ _bt_findinsertloc(Relation rel,
 
 	lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
 
-	itemsz = IndexTupleDSize(*newtup);
+	itemsz = IndexTupleSize(newtup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -755,7 +755,7 @@ _bt_insertonpg(Relation rel,
 		elog(ERROR, "cannot insert to incompletely split page %u",
 			 BufferGetBlockNumber(buf));
 
-	itemsz = IndexTupleDSize(*itup);
+	itemsz = IndexTupleSize(itup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -914,7 +914,7 @@ _bt_insertonpg(Relation rel,
 									sizeof(IndexTupleData));
 			}
 			else
-				XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
+				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
index bf6c03c..b71529f 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -468,7 +468,7 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state, IndexTuple itup)
 	last_off = state->btps_lastoff;
 
 	pgspc = PageGetFreeSpace(npage);
-	itupsz = IndexTupleDSize(*itup);
+	itupsz = IndexTupleSize(itup);
 	itupsz = MAXALIGN(itupsz);
 
 	/*
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index 7250b4f..c18a0d2 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -53,7 +53,7 @@ _bt_restore_page(Page page, char *from, int len)
 	{
 		/* Need to copy tuple header due to alignment considerations */
 		memcpy(&itupdata, from, sizeof(IndexTupleData));
-		itemsz = IndexTupleDSize(itupdata);
+		itemsz = IndexTupleSize(&itupdata);
 		itemsz = MAXALIGN(itemsz);
 
 		items[i] = (Item) from;
diff --git a/src/include/access/itup.h b/src/include/access/itup.h
index c178ae9..37b6137 100644
--- a/src/include/access/itup.h
+++ b/src/include/access/itup.h
@@ -68,7 +68,6 @@ typedef IndexAttributeBitMapData * IndexAttributeBitMap;
 #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))

Amit Kapila

amit.kapila16@gmail.com

about 8 years ago

In reply to: Ildar Musin (#1)

Re: IndexTupleDSize macro seems redundant

On Mon, Nov 20, 2017 at 9:01 PM, Ildar Musin <i.musin@postgrespro.ru> wrote:

Hi all,

While I was looking through the indexes code I got confused by couple of
macros - IndexTupleSize() and IndexTupleDSize() - which seem to do the same
thing with only difference that the first one takes pointer as an argument
while the second one takes struct. And in most cases IndexTupleDSize() is
used with dereferencing of index tuple where IndexTupleSize() would suit
perfectly. Is there a particular reason to have them both? I've made a patch
that removes IndexTupleDSize macro. All the tests have passed.

+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.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Amit Kapila (#2)

1 attachment(s)

Re: IndexTupleDSize macro seems redundant

On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit.kapila16@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.

Therefore I propose the attached patch instead.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

remove-indextupledsize-v2.patchapplication/octet-stream; name=remove-indextupledsize-v2.patchDownload

diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index f19f6fdfaf..a13314338a 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -558,7 +558,7 @@ hash_xlog_move_page_contents(XLogReaderState *record)
 				Size		itemsz;
 				OffsetNumber l;
 
-				itemsz = IndexTupleDSize(*itup);
+				itemsz = IndexTupleSize(itup);
 				itemsz = MAXALIGN(itemsz);
 
 				data += itemsz;
@@ -686,7 +686,7 @@ hash_xlog_squeeze_page(XLogReaderState *record)
 				Size		itemsz;
 				OffsetNumber l;
 
-				itemsz = IndexTupleDSize(*itup);
+				itemsz = IndexTupleSize(itup);
 				itemsz = MAXALIGN(itemsz);
 
 				data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index dc08db97db..b90d4174fa 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -55,7 +55,7 @@ _hash_doinsert(Relation rel, IndexTuple itup, Relation heapRel)
 	hashkey = _hash_get_indextuple_hashkey(itup);
 
 	/* compute item size too */
-	itemsz = IndexTupleDSize(*itup);
+	itemsz = IndexTupleSize(itup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -222,7 +222,7 @@ restart_insert:
 		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
 
 		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
-		XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
+		XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
 
 		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
 
@@ -309,7 +309,7 @@ _hash_pgaddmultitup(Relation rel, Buffer buf, IndexTuple *itups,
 	{
 		Size		itemsize;
 
-		itemsize = IndexTupleDSize(*itups[i]);
+		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
index c206e704d4..b5ec0f69d6 100644
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -890,7 +890,7 @@ readpage:
 
 			itup = (IndexTuple) PageGetItem(rpage,
 											PageGetItemId(rpage, roffnum));
-			itemsz = IndexTupleDSize(*itup);
+			itemsz = IndexTupleSize(itup);
 			itemsz = MAXALIGN(itemsz);
 
 			/*
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index a50e35dfcb..51100e059b 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -1173,7 +1173,7 @@ _hash_splitbucket(Relation rel,
 				 * 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 = 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
index 310589da4e..b39090aff1 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -558,7 +558,7 @@ _bt_findinsertloc(Relation rel,
 
 	lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
 
-	itemsz = IndexTupleDSize(*newtup);
+	itemsz = IndexTupleSize(newtup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -755,7 +755,7 @@ _bt_insertonpg(Relation rel,
 		elog(ERROR, "cannot insert to incompletely split page %u",
 			 BufferGetBlockNumber(buf));
 
-	itemsz = IndexTupleDSize(*itup);
+	itemsz = IndexTupleSize(itup);
 	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
 								 * need to be consistent */
 
@@ -914,7 +914,7 @@ _bt_insertonpg(Relation rel,
 									sizeof(IndexTupleData));
 			}
 			else
-				XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
+				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
index bf6c03c7b2..b71529f124 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -468,7 +468,7 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state, IndexTuple itup)
 	last_off = state->btps_lastoff;
 
 	pgspc = PageGetFreeSpace(npage);
-	itupsz = IndexTupleDSize(*itup);
+	itupsz = IndexTupleSize(itup);
 	itupsz = MAXALIGN(itupsz);
 
 	/*
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index 7250b4f0b8..afd9f824b4 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -53,7 +53,7 @@ _bt_restore_page(Page page, char *from, int len)
 	{
 		/* Need to copy tuple header due to alignment considerations */
 		memcpy(&itupdata, from, sizeof(IndexTupleData));
-		itemsz = IndexTupleDSize(itupdata);
+		itemsz = IndexTupleSize(&itupdata);
 		itemsz = MAXALIGN(itemsz);
 
 		items[i] = (Item) from;
@@ -282,7 +282,7 @@ btree_xlog_split(bool onleft, XLogReaderState *record)
 		if (onleft)
 		{
 			newitem = (Item) datapos;
-			newitemsz = MAXALIGN(IndexTupleSize(newitem));
+			newitemsz = MAXALIGN(IndexTupleSize((IndexTuple) newitem));
 			datapos += newitemsz;
 			datalen -= newitemsz;
 		}
@@ -291,7 +291,7 @@ btree_xlog_split(bool onleft, XLogReaderState *record)
 		if (!isleaf)
 		{
 			left_hikey = (Item) datapos;
-			left_hikeysz = MAXALIGN(IndexTupleSize(left_hikey));
+			left_hikeysz = MAXALIGN(IndexTupleSize((IndexTuple) left_hikey));
 			datapos += left_hikeysz;
 			datalen -= left_hikeysz;
 		}
diff --git a/src/include/access/itup.h b/src/include/access/itup.h
index c178ae91a9..4b53d32ee5 100644
--- a/src/include/access/itup.h
+++ b/src/include/access/itup.h
@@ -67,8 +67,7 @@ typedef IndexAttributeBitMapData * IndexAttributeBitMap;
 #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 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))

Peter Geoghegan

pg@bowt.ie

about 8 years ago

In reply to: Robert Haas (#3)

Re: IndexTupleDSize macro seems redundant

On Thu, Nov 30, 2017 at 1:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

Therefore I propose the attached patch instead.

+1 to both points. I specifically recall being annoyed by both issues,
more than once.

--
Peter Geoghegan

Stephen Frost

sfrost@snowman.net

about 8 years ago

In reply to: Robert Haas (#3)

2 attachment(s)

Re: IndexTupleDSize macro seems redundant

Robert, all,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit.kapila16@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

Attachments:

remove-indextupledsize-v3.patchtext/x-diff; charset=us-asciiDownload

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))

remove-Item-as-variable_v1.patchtext/x-diff; charset=us-asciiDownload

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;
  	}
  }

Tom Lane

tgl@sss.pgh.pa.us

about 8 years ago

In reply to: Stephen Frost (#5)

Re: IndexTupleDSize macro seems redundant

Stephen Frost <sfrost@snowman.net> writes:

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.

I'm on board with Stephen's changes, except in _bt_restore_page.
The issue there is that the "from" pointer isn't necessarily adequately
aligned to be considered an IndexTuple pointer; that's why we're doing
the memcpy dance to get a length out of it. "Item" doesn't connote
anything about alignment (it's the same as Pointer, ie char*). Even
though we don't do anything with items[i] except pass it as an Item
to PageAddItem, the proposed change could result in breakage, because
the compiler could take it as license to assume that "from" is aligned,
and perhaps change what it generates for the memcpy.

I think that in the other places where Stephen wants to change Item
to something else, the alignment expectation actually does hold,
so we're OK if we want to do it in those places.

regards, tom lane

Stephen Frost

sfrost@snowman.net

about 8 years ago

In reply to: Tom Lane (#6)

Re: IndexTupleDSize macro seems redundant

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

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.

I'm on board with Stephen's changes, except in _bt_restore_page.
The issue there is that the "from" pointer isn't necessarily adequately
aligned to be considered an IndexTuple pointer; that's why we're doing
the memcpy dance to get a length out of it. "Item" doesn't connote
anything about alignment (it's the same as Pointer, ie char*). Even
though we don't do anything with items[i] except pass it as an Item
to PageAddItem, the proposed change could result in breakage, because
the compiler could take it as license to assume that "from" is aligned,
and perhaps change what it generates for the memcpy.

I certainly hadn't been thinking about that. I didn't see any
issues in my testing (where I created a table with a btree index and
insert'd a bunch of records into and then killed the server, forcing WAL
replay and then checked that the index appeared to be valid using order
by queries; perhaps I should have tried amcheck, but doesn't sound like
this is something that would have been guaranteed to break anyway).

That said, since it's not aligned, regardless of the what craziness the
compiler might try to pull, we probably shouldn't go casting it
to something that later hackers might think will be aligned, but we
should add a comment to clarify that it's not aligned and that we can't
act like it is.

I think that in the other places where Stephen wants to change Item
to something else, the alignment expectation actually does hold,
so we're OK if we want to do it in those places.

Yes, everywhere else it's a pointer returned from PageGetItem() or
XLogRecGetBlockData() (which always returns a MAXALIGNed pointer).

Thanks!

Stephen

Tom Lane

tgl@sss.pgh.pa.us

about 8 years ago

In reply to: Stephen Frost (#7)

Re: IndexTupleDSize macro seems redundant

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

I'm on board with Stephen's changes, except in _bt_restore_page.
The issue there is that the "from" pointer isn't necessarily adequately
aligned to be considered an IndexTuple pointer; that's why we're doing
the memcpy dance to get a length out of it.

I certainly hadn't been thinking about that. I didn't see any
issues in my testing (where I created a table with a btree index and
insert'd a bunch of records into and then killed the server, forcing WAL
replay and then checked that the index appeared to be valid using order
by queries; perhaps I should have tried amcheck, but doesn't sound like
this is something that would have been guaranteed to break anyway).

You wouldn't see a problem, unless you tested on alignment-picky
hardware, ie, not Intel.

I wonder whether there is a way to get alignment traps on Intel-type
hardware. It's getting less and less likely that most hackers are
developing on anything else, so that we don't see gotchas of this
type until code hits the buildfarm (and even then, only if the case
is actually exercised in regression testing).

regards, tom lane

Andres Freund

andres@anarazel.de

about 8 years ago

In reply to: Tom Lane (#8)

Re: IndexTupleDSize macro seems redundant

On 2018-01-11 13:26:27 -0500, Tom Lane wrote:

I wonder whether there is a way to get alignment traps on Intel-type
hardware. It's getting less and less likely that most hackers are
developing on anything else, so that we don't see gotchas of this
type until code hits the buildfarm (and even then, only if the case
is actually exercised in regression testing).

It's possible, but a lot of code out there, including things like glibc,
assumes that unaligned accesses are fine. Therefore it's very painful to
use.

Greetings,

Andres Freund

#10

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Tom Lane (#8)

Re: IndexTupleDSize macro seems redundant

On Thu, Jan 11, 2018 at 1:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I certainly hadn't been thinking about that. I didn't see any
issues in my testing (where I created a table with a btree index and
insert'd a bunch of records into and then killed the server, forcing WAL
replay and then checked that the index appeared to be valid using order
by queries; perhaps I should have tried amcheck, but doesn't sound like
this is something that would have been guaranteed to break anyway).

You wouldn't see a problem, unless you tested on alignment-picky
hardware, ie, not Intel.

I wonder whether there is a way to get alignment traps on Intel-type
hardware. It's getting less and less likely that most hackers are
developing on anything else, so that we don't see gotchas of this
type until code hits the buildfarm (and even then, only if the case
is actually exercised in regression testing).

-fsanitize=alignment?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#11

Stephen Frost

sfrost@snowman.net

about 8 years ago

In reply to: Stephen Frost (#7)

1 attachment(s)

Re: IndexTupleDSize macro seems redundant

Greetings Tom, Robert, Ildar, all,

* Stephen Frost (sfrost@snowman.net) wrote:

That said, since it's not aligned, regardless of the what craziness the
compiler might try to pull, we probably shouldn't go casting it
to something that later hackers might think will be aligned, but we
should add a comment to clarify that it's not aligned and that we can't
act like it is.

Updated (combined) patch attached for review. I went through and looked
again to make sure there weren't any cases of making an unaligned
pointer to a struct and didn't see any, and I added some comments to
_bt_restore_page().

Thanks!

Stephen

Attachments:

remove-indextupledsize-v4.patchtext/x-diff; charset=us-asciiDownload

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/heap/hio.c b/src/backend/access/heap/hio.c
new file mode 100644
index 0d7bc68..42e75ec
*** a/src/backend/access/heap/hio.c
--- b/src/backend/access/heap/hio.c
*************** RelationPutHeapTuple(Relation relation,
*** 67,75 ****
  	if (!token)
  	{
  		ItemId		itemId = PageGetItemId(pageHeader, offnum);
! 		Item		item = PageGetItem(pageHeader, itemId);
  
! 		((HeapTupleHeader) item)->t_ctid = tuple->t_self;
  	}
  }
  
--- 67,75 ----
  	if (!token)
  	{
  		ItemId		itemId = PageGetItemId(pageHeader, offnum);
! 		HeapTupleHeader item = (HeapTupleHeader) PageGetItem(pageHeader, itemId);
  
! 		item->t_ctid = tuple->t_self;
  	}
  }
  
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..454bfe2
*** a/src/backend/access/nbtree/nbtxlog.c
--- b/src/backend/access/nbtree/nbtxlog.c
*************** _bt_restore_page(Page page, char *from,
*** 51,59 ****
  	i = 0;
  	while (from < end)
  	{
! 		/* Need to copy tuple header due to alignment considerations */
  		memcpy(&itupdata, from, sizeof(IndexTupleData));
! 		itemsz = IndexTupleDSize(itupdata);
  		itemsz = MAXALIGN(itemsz);
  
  		items[i] = (Item) from;
--- 51,65 ----
  	i = 0;
  	while (from < end)
  	{
! 		/* 
! 		 * As we step through the items, 'from' won't always be properly
! 		 * aligned, so we need to use memcpy().  Further, we use Item (which
! 		 * is just a char*) here for our items array for the same reason;
! 		 * wouldn't want the compiler or anyone thinking that an item is
! 		 * aligned when it isn't.
! 		 */
  		memcpy(&itupdata, from, sizeof(IndexTupleData));
! 		itemsz = IndexTupleSize(&itupdata);
  		itemsz = MAXALIGN(itemsz);
  
  		items[i] = (Item) from;
*************** 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;
--- 211,217 ----
  	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);
  	}
  
--- 254,260 ----
  	{
  		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;
--- 278,284 ----
  		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;
--- 287,293 ----
  
  		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;
--- 296,302 ----
  		/* 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);
--- 307,313 ----
  
  		/* 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);
--- 316,327 ----
  		{
  			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);
--- 329,336 ----
  
  			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);
--- 339,345 ----
  		/* 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))

#12

Tom Lane

tgl@sss.pgh.pa.us

about 8 years ago

In reply to: Stephen Frost (#11)

Re: IndexTupleDSize macro seems redundant

Stephen Frost <sfrost@snowman.net> writes:

Updated (combined) patch attached for review. I went through and looked
again to make sure there weren't any cases of making an unaligned
pointer to a struct and didn't see any, and I added some comments to
_bt_restore_page().

Looks OK from here.

regards, tom lane

#13

Stephen Frost

sfrost@snowman.net

about 8 years ago

In reply to: Tom Lane (#12)

Re: IndexTupleDSize macro seems redundant

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Updated (combined) patch attached for review. I went through and looked
again to make sure there weren't any cases of making an unaligned
pointer to a struct and didn't see any, and I added some comments to
_bt_restore_page().

Looks OK from here.

Great, thanks, I'll mark it as Ready For Committer then.

Robert, since you were on this thread and the patch is mostly yours
anyway, did you want to commit it? I'm happy to do so also, either way.

Thanks!

Stephen

#14

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Stephen Frost (#13)

Re: IndexTupleDSize macro seems redundant

On Thu, Jan 11, 2018 at 9:17 PM, Stephen Frost <sfrost@snowman.net> wrote:

Great, thanks, I'll mark it as Ready For Committer then.

Robert, since you were on this thread and the patch is mostly yours
anyway, did you want to commit it? I'm happy to do so also, either way.

Feel free.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#15

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Stephen Frost (#11)

Re: IndexTupleDSize macro seems redundant

Stephen Frost <sfrost@snowman.net> writes:

Updated (combined) patch attached for review. I went through and looked
again to make sure there weren't any cases of making an unaligned
pointer to a struct and didn't see any, and I added some comments to
_bt_restore_page().

This seems to have fallen through a crack, so I went ahead and pushed it.

regards, tom lane