pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedIndexTupleDSize macro seems redundant
15+ messages / 7 participants
[nested] [flat]
Commits from this thread:
2018-03-01 d79e7e92bf Remove redundant IndexTupleDSize macro.
* IndexTupleDSize macro seems redundant
@ 2017-11-20 15:31 Ildar Musin <i.musin@postgrespro.ru>
0 siblings, 1 reply; 15+ messages in thread
From: Ildar Musin @ 2017-11-20 15:31 UTC (permalink / raw)
To: pgsql-hackers
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.patch (text/x-patch, 5.5K) download
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))
* Re: IndexTupleDSize macro seems redundant
@ 2017-11-21 14:26 Amit Kapila <amit.kapila16@gmail.com>
parent: Ildar Musin <i.musin@postgrespro.ru>
0 siblings, 1 reply; 15+ messages in thread
From: Amit Kapila @ 2017-11-21 14:26 UTC (permalink / raw)
To: Ildar Musin <i.musin@postgrespro.ru>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2017-11-30 21:48 Robert Haas <robertmhaas@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 15+ messages in thread
From: Robert Haas @ 2017-11-30 21:48 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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.patch (application/octet-stream, 6.2K) download
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))
* Re: IndexTupleDSize macro seems redundant
@ 2017-11-30 21:50 Peter Geoghegan <pg@bowt.ie>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 15+ messages in thread
From: Peter Geoghegan @ 2017-11-30 21:50 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 17:22 Stephen Frost <sfrost@snowman.net>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 15+ messages in thread
From: Stephen Frost @ 2018-01-11 17:22 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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: download all (3 files, .tar.gz)
remove-indextupledsize-v3.patch (text/x-diff, 14.7K) download
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.patch (text/x-diff, 689B) download
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;
}
}
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 17:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2018-01-11 17:47 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 18:17 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 15+ messages in thread
From: Stephen Frost @ 2018-01-11 18:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 18:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 2 replies; 15+ messages in thread
From: Tom Lane @ 2018-01-11 18:26 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 18:35 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 15+ messages in thread
From: Andres Freund @ 2018-01-11 18:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Stephen Frost <sfrost@snowman.net>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 18:45 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 15+ messages in thread
From: Robert Haas @ 2018-01-11 18:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Stephen Frost <sfrost@snowman.net>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 20:32 Stephen Frost <sfrost@snowman.net>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 2 replies; 15+ messages in thread
From: Stephen Frost @ 2018-01-11 20:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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.patch (text/x-diff, 14.6K) download
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))
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-11 23:19 Tom Lane <tgl@sss.pgh.pa.us>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 15+ messages in thread
From: Tom Lane @ 2018-01-11 23:19 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-12 02:17 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Stephen Frost @ 2018-01-12 02:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-01-12 02:59 Robert Haas <robertmhaas@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 15+ messages in thread
From: Robert Haas @ 2018-01-12 02:59 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: IndexTupleDSize macro seems redundant
@ 2018-03-01 00:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 0 replies; 15+ messages in thread
From: Tom Lane @ 2018-03-01 00:26 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Ildar Musin <i.musin@postgrespro.ru>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2018-03-01 00:26 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-11-20 15:31 IndexTupleDSize macro seems redundant Ildar Musin <i.musin@postgrespro.ru>
2017-11-21 14:26 ` Amit Kapila <amit.kapila16@gmail.com>
2017-11-30 21:48 ` Robert Haas <robertmhaas@gmail.com>
2017-11-30 21:50 ` Peter Geoghegan <pg@bowt.ie>
2018-01-11 17:22 ` Stephen Frost <sfrost@snowman.net>
2018-01-11 17:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-01-11 18:17 ` Stephen Frost <sfrost@snowman.net>
2018-01-11 18:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-01-11 18:35 ` Andres Freund <andres@anarazel.de>
2018-01-11 18:45 ` Robert Haas <robertmhaas@gmail.com>
2018-01-11 20:32 ` Stephen Frost <sfrost@snowman.net>
2018-01-11 23:19 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-01-12 02:17 ` Stephen Frost <sfrost@snowman.net>
2018-01-12 02:59 ` Robert Haas <robertmhaas@gmail.com>
2018-03-01 00:26 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox