Robert Haas <robertmhaas@gmail.com> writes:
> On further review, this is definitely the way to go: it's a
> straight-up win. The isnull array is never more than one element in
> length, so testing the single element is quite trivial. The
> attached, revised patch provides a modest but useful speedup for both
> hash and btree index builds.
> Anyone see any reason NOT to do this? So far it looks like a
> slam-dunk from here.
If index_form_tuple leaks any memory in the sort context, this would be
not so much a slam-dunk win as a complete disaster. I'm not sure that
no-leak is a safe assumption, especially in cases where we do toast
compression of the index datums. (In fact, I'm pretty sure that the
reason it was coded like this originally was exactly to avoid that
assumption.)
Assuming that you inspect the code and determine it's safe, the patch
had better decorate index_form_tuple with dire warnings about not leaking
memory; because even if it's safe today, somebody might break it tomorrow.
On a micro-optimization level, it might be worth passing the TID as
ItemPointer not ItemPointerData (ie, pass a pointer until we get to
the point of actually inserting the TID into the index tuple).
I'm not sure that copying odd-size structs should be assumed to be
efficient.
regards, tom lane
