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.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to