Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Barring objections, I'm going to apply the indexam API changes part,
> since that simplifies the code in question regardless of the rest of the
> work. I'm pretty happy now with the indexfilter patch as well, but want
> to do some more testing on that before committing. Some more eyeballs
> would be appreciated as well.
The first patch definitely needs another editorial pass, eg you have
complicated the behavior of heap_hot_search_buffer's all_dead parameter
without adjusting its documentation. The patch as-submitted is really
quite hard to review, though. It's hard to convince myself that all the
logic you removed from index_getnext is all accounted for elsewhere.
Could you run through the reasoning on that?
I think the second patch needs a fair amount of work. The fact that
_bt_getindextuple can fail to get the right tuple seems quite bogus;
what I think it means is that you've attached the functionality in the
wrong place. nbtree certainly had its hands on the right tuple at some
point, and what you should have done was saved the tuple aside at that
point. I feel it's important that an indexam either be able to give
back tuples or not; this "maybe I can" semantics will prevent us from
doing many interesting things with the capability. (More about that
in an upcoming message.)
ExecStoreIndexTuple seems rather confused as well, as it's applying what
apparently is a heap tupdesc to an index tuple. That might accidentally
fail to fail at the moment, but I think it would be better to keep the
two concepts well separated. Moreover attr-by-attr extraction is not
what you want, for efficiency reasons. Use index_deform_tuple(), now
that that's there. (The internal implementation of that is no better,
but now that there is actually a performance reason to improve it, we
could do that.)
I concur with the objection to "regurgitate" terminology, it seems a
bit yucky.
I haven't had time to go through the planner part in any detail...
regards, tom lane
