Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  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

Re: IndexTupleDSize macro seems redundant

[Robert Haas]

On Thu, Jan 11, 2018 at 9:17 PM, Stephen Frost  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

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  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


signature.asc
Description: PGP signature

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  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

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Greetings Tom, Robert, Ildar, all,

* Stephen Frost (sfr...@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
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 

Re: IndexTupleDSize macro seems redundant

[Robert Haas]

On Thu, Jan 11, 2018 at 1:26 PM, Tom Lane  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

Re: IndexTupleDSize macro seems redundant

[Andres Freund]

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

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@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

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  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


signature.asc
Description: PGP signature

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  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

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila  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);
Itemitem = PageGetItem(pageHeader, itemId);

((HeapTupleHeader) item)->t_ctid = tuple->t_self;
}

So I've attached a seperate patch for that, too.

I'll leave the patch status in 'Needs review' since there's more
changes, but hopefully someone can take a look and we can move this
along, seems like a pretty small and reasonable improvement.

Thanks!

Stephen
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
new file mode 100644
index b38208e..ab5aaff
*** a/src/backend/access/hash/hash_xlog.c
--- b/src/backend/access/hash/hash_xlog.c
*** hash_xlog_move_page_contents(XLogReaderS
*** 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
*** hash_xlog_squeeze_page(XLogReaderState *
*** 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
new file mode 100644
index f668dcf..f121286
*** a/src/backend/access/hash/hashinsert.c
--- b/src/backend/access/hash/hashinsert.c
*** _hash_doinsert(Relation rel, IndexTuple
*** 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleDSize(*itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
--- 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleSize(itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
*** restart_insert:
*** 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
--- 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
*** _hash_pgaddmultitup(Relation rel, Buffer
*** 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleDSize(*itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
--- 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleSize(itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
diff --git a/src/backend/access/hash/hashovfl.c 

Re: IndexTupleDSize macro seems redundant

[Peter Geoghegan]

On Thu, Nov 30, 2017 at 1:48 PM, Robert Haas  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

Re: IndexTupleDSize macro seems redundant

[Amit Kapila]

On Mon, Nov 20, 2017 at 9:01 PM, Ildar Musin  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