Re: IndexTupleDSize macro seems redundant

Robert, all,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > +1.  I was also once confused with these macros.  I think this is a
> > good cleanup.  On a quick look, I don't see any problem with your
> > changes.
>
> One difference between those two macros is that IndexTupleSize
> forcibly casts the argument to IndexTuple, which means that you don't
> get any type-checking when you use that one.  I suggest that in
> addition to removing IndexTupleDSize as proposed, we also remove that
> cast.  It seems that the only place which relies on that cast
> currently is btree_xlog_split.

I agree with removing the macro and the force cast that's being done,
but I would have thought to change btree_xlog_split() to declare those
variables as IndexTuple (since that's really what it is, no?) and then
cast the other way when needed, as in the attached.

I'll note that basically every other function in nbtxlog.c operates this
way too, declaring the variable as the appropriate type (not just
'Item') and then casting to that when calling PageGetItem and casting
back when calling PageAddItem().

See btree_xlog_delete_get_latestRemovedXid(),
btree_xlog_mark_page_halfdead(), and btree_xlog_unlink_page().

The only other place where Item is actually declared as a variable is in
_bt_restore_page(), and it looks like it probably makes sense to change
that to IndexTuple too.

Attached is a patch which does that.

Looking further, there's actually only one other place that uses Item as
an actual declared variable (rather than being part of a function
signature and passed in), and that's in RelationPutHeapTuple() and it
looks like it should really be changed:

    if (!token)
    {
        ItemId      itemId = PageGetItemId(pageHeader, offnum);
        Item        item = PageGetItem(pageHeader, itemId);

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

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

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

Thanks!

Stephen