Hi,
I have reviewed this now and I think this is a useful addition even
though these indexes are less common. Consistent behavior is worth a lot
in my mind and this patch is reasonably small.
The patch no longer applies due to 1) oid collisions and 2) a trivial
conflict. When I fixed those two the test suite passed.
I tested this patch by building indexes with all the typess and got nice
measurable speedups.
Logically I think the patch makes sense and the code seems to be
correct, but I have some comments on it.
- You use two names a lot "string" vs "varstr". What is the difference
between those? Is there any reason for not using varstr consistently?
- You have a lot of renaming as has been mentioned previously in this
thread. I do not care strongly for it either way, but it did make it
harder to spot what the patch changed. If it was my own project I would
have considered splitting the patch into two parts, one renaming
everything and another adding the new feature, but the PostgreSQL seem
to often prefer having one commit per feature. Do as you wish here.
- I think the comment about NUL bytes in varstr_abbrev_convert makes it
seem like the handling is much more complicated than it actually is. I
did not understand it after a couple of readings and had to read the
code understand what it was talking about.
Nice work, I like your sorting patches.
Andreas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
