On Fri, Dec 18, 2015 at 2:22 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Fri, Dec 18, 2015 at 9:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> Attached revision updates both the main commit (the optimization), and
>>> the backpatch commit (updated the contract).
>>
>> - /* abbreviation is possible here only for by-reference types */
>> + /*
>> + * Abbreviation is possible here only for by-reference types.
>> Note that a
>> + * pass-by-value representation for abbreviated values is forbidden, but
>> + * that's a distinct, generic restriction imposed by the SortSupport
>> + * contract.
>>
>> I think that you have not written what you meant to write here. I
>> think what you mean is "Note that a pass-by-REFERENCE representation
>> for abbreviated values is forbidden...".
>
> You're right. Sorry about that.
PFA my proposal for comment changes for 9.5 and master. This is based
on your 0001, but I edited somewhat. Please let me know your
thoughts. I am not willing to go further and rearrange actual code in
9.5 at this point; it just isn't necessary.
Looking at this whole system again, I wonder if we're missing a trick
here. How about if we decree from on high that the abbreviated-key
comparator is always just the application of an integer comparison
operator? The only abbreviation function that we have right now that
wouldn't be happy with that is the one for numeric, and that looks
like it could be fixed. Then we could hard-code knowledge of this
representation in tuplesort.c in such a way that it wouldn't need to
call a comparator function at all - instead of doing
ApplySortComparator() and then maybe ApplySortAbbrevFullComparator(),
it would do something like:
if (using_abbreviation && (compare = ApplyAbbrevComparator(...)) != 0)
return compare;
I'm not sure if that would save enough cycles vs. the status quo to be
worth bothering with, but it seems like it might. You may have a
better feeling for that than I do.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company