IndexTupleDSize macro seems redundant

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(dot)musin(at)postgrespro(dot)ru

On Mon, Nov 20, 2017 at 9:01 PM, Ildar Musin <i(dot)musin(at)postgrespro(dot)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

On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

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

Robert, all,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

Stephen Frost <sfrost(at)snowman(dot)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

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)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

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

On Thu, Jan 11, 2018 at 1:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Greetings Tom, Robert, Ildar, all,

* Stephen Frost (sfrost(at)snowman(dot)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

Stephen Frost <sfrost(at)snowman(dot)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

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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

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

Stephen Frost <sfrost(at)snowman(dot)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