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: http://www.postgresql.org/mailpref/pgsql-hackers
