On Tue, Aug 5, 2014 at 3:15 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Tue, Aug 5, 2014 at 12:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
>
> That's true, but my proposal to do things that way reflects the fact
> that text is a type oddly tied to the platform. I don't think it will
> come up again (note that in the 4 byte Datum case, we still use sort
> support to some degree on other platforms with patch 2 applied). It
> seemed logical to impose the obligation to deal with that on
> varlena.c.
Per your other email, here's the patch again; hopefully I've got the
right stuff in the file this time.
On this point, I'm all for confining knowledge of things to a
particular module to that module. However, in this particular case, I
don't believe that there's anything specific to varlena.c in
bttext_inject_shim(); it's a cut down version of the combination of
functions that appear in other modules, and there's nothing to
encourage someone who modifies one of those functions to also update
varlena.c. Indeed, people developing on non-Windows platforms won't
even be compiling that function, so it would be easy for most of us to
miss the need for an update. So I argue that my approach is keeping
this knowledge more localized.
I'm also not sure it won't come up again. There are certainly other
text-like datatypes out there that might want to optimize sorts; e.g.
citext.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
