Re: B-Tree support function number 3 (strxfrm() optimization)

On Thu, Sep 11, 2014 at 8:34 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Tue, Sep 9, 2014 at 2:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I like that I don't have to care about every combination, and can
>>> treat abbreviation abortion as the special case with the extra step,
>>> in line with how I think of the optimization conceptually. Does that
>>> make sense?
>>
>> No.  comparetup_heap() is hip-deep in this optimization as it stands,
>> and what I proposed - if done correctly - isn't going to make that
>> significantly worse.  In fact, it really ought to make things better:
>> you should be able to set things up so that ssup->comparator is always
>> the test that should be applied first, regardless of whether we're
>> aborted or not-aborted or not doing this in the first place; and then
>> ssup->tiebreak_comparator, if not NULL, can be applied after that.
>
> I'm not following here. Isn't that at least equally true of what I've
> proposed? Sure, I'm checking "if (!state->abbrevAborted)" first, but
> that's irrelevant to the non-abbreviated case. It has nothing to
> abort. Also, AFAICT we cannot abort and still call ssup->comparator()
> indifferently, since sorttuple.datum1 fields are perhaps abbreviated
> keys in half of all cases (i.e. pre-abort tuples), and uninitialized
> garbage the other half of the time (i.e. post-abort tuples).

You can if you engineer ssup->comparator() to contain the right
pointer at the right time.  Also, shouldn't you go back and fix up
those abbreviated keys to point to datum1 again if you abort?

> Where is the heap_getattr() stuff supposed to happen for the first
> attribute to get an authoritative comparison in the event of aborting
> (if we're calling ssup->comparator() on datum1 indifferently)? We
> decide that we're going to use abbreviated keys within datum1 fields
> up-front. When we abort, we cannot use datum1 fields at all (which
> doesn't appear to matter for text -- the datum1 optimization has
> historically only benefited pass-by-value types).

You always pass datum1 to a function.  The code doesn't need to care
about whether that function is expecting abbreviated or
non-abbreviated datums unless that function returns equality.  Then it
needs to think about calling a backup comparator if there is one.

> Is doing all this worth the small saving in memory? Personally, I
> don't think that it is, but I defer to you.

I don't care about the memory; I care about the code complexity and
the ease of understanding that code.  I am confident that it can be
done better than the patch does it today.

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