On Sun, Jan 31, 2016 at 10:59 PM, Andreas Karlsson <andr...@proxel.se> wrote: > 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.
Thanks for the review. I fixed the OID conflict, tweaked a few comments, and committed this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
