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

On Sat, Aug 2, 2014 at 6:58 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Sat, Aug 2, 2014 at 2:45 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> I'll post something over the weekend.
>
> Attached is a cumulative pair of patches generated using
> git-format-patch. I refer anyone who wants to know how the two parts
> fit together to the commit messages of each patch. In passing, I have
> added a reference to the MIT License as outlined by Noah.

OK, I have taken a look at patch 1. You write:

+ * As a general principle, operator classes with a cataloged sort support
+ * function are expected to provide sane sort support state, including a
+ * function pointer comparator. Rather than revising that principle, just
+ * setup a shim for the WIN32 UTF-8 and non-"C" collation special case here.

...but I'm wondering what underlies that decision. I would
understand the decision to go that way if it simplified things
elsewhere, but in fact it seems that's what underlies the addition of
ssup_operator to SortSupportData, which in turn requires a number of
changes elsewhere. The upshot of those changes is that it makes it
possible to write bttext_inject_shim, but AFAICS that's just
recapitulating what get_sort_function_for_ordering_op and
PrepareSortSupportFromOrderingOp are already doing. Any material
change to either of those functions will have to be reflected in
bttext_inject_shim; and if some opclass other than text wants to
provide a sortsupport shim that supplies a comparator only sometimes,
it will need its own copy of the logic.

So I think it's better to just change the sortsupport contract so that
filling in the comparator is optional. Patch for that attached.
Objections?

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