On Mon, Jun 16, 2014 at 8:10 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Robert Haas <robertmh...@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.)

Yes, that would be bad.  The reason I think it's probably OK is that
index_form_tuple seems to have had, from ancient times (cf.
f67e79045), code of non-trivial to free any memory that's allocated as
a result of de-TOASTing, which we presumably would not bother to do if
we were counting on memory context resets to clean things up.  It
calls toast_compress_datum(), which is similarly careful.  The most
complicated code path appears to be the VARATT_IS_EXTERNAL() case,
where we call heap_tuple_fetch_attr(), which calls
toast_fetch_datum().  I'm kind of wondering whether that can really
happen, though, because I didn't think indexes could use TOAST
storage.  Even if it does happen I don't see any obvious reason why it
should leak, but it'd be harder to be certain that it doesn't.

> 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.

Yeah, true.  Checking existing precedent, it looks like we usually
pass ItemPointer rather than ItemPointerData, so it's probably a good
idea to do this that way too for reasons of style if nothing else.  I
kind of wonder whether it's really more efficient to pass an 8-byte
pointer to a 6-byte structure than to just pass the structure itself,
but it might be.

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to