On 20 January 2016 at 06:08, Anastasia Lubennikova <a.lubennik...@postgrespro.ru> wrote: > > > > 18.01.2016 01:02, David Rowley пишет: > > On 14 January 2016 at 08:24, David Rowley <david.row...@2ndquadrant.com> > wrote: >> >> I will try to review the omit_opclass_4.0.patch soon. > > > Hi, as promised, here's my review of the omit_opclass_4.0.patch patch. > > Thank you again. All mentioned points are fixed and patches are merged. > I hope it's all right now. Please check comments one more time. I rather > doubt that I wrote everything correctly.
Thanks for updating. + for the searching or ordering of records can defined in the should be: + for the searching or ordering of records can be defined in the but perhaps "defined" should be "included". The following is still quite wasteful. CopyIndexTuple() does a palloc() and memcpy(), and then you throw that away if rel->rd_index->indnatts != rel->rd_index->indnkeyatts. I think you just need to add an "else" and move the CopyIndexTuple() below the if. item = (IndexTuple) PageGetItem(lpage, itemid); right_item = CopyIndexTuple(item); + if (rel->rd_index->indnatts != rel->rd_index->indnkeyatts) + right_item = index_reform_tuple(rel, right_item, rel->rd_index->indnatts, rel->rd_index->indnkeyatts); Tom also commited http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=65c5fcd353a859da9e61bfb2b92a99f12937de3b So it looks like you'll need to update your pg_am.h changes. Looks like you'll need a new struct member in IndexAmRoutine and just populate that new member in each of the *handler functions listed in pg_am.h -#define Natts_pg_am 30 +#define Natts_pg_am 31 Can the following be changed to - (At present, only b-tree supports it.) + (At present, only b-tree supports it.) Columns included with clause + INCLUDING aren't used to enforce uniqueness. - (At present, only b-tree supports it.) + (At present, only b-tree supports it.) Columns which are present in the <literal>INCLUDING</> clause are not used to enforce uniqueness. > Also this makes me think that the name ii_KeyAttrNumbers is now out-of-date, > as it contains > the including columns too by the looks of it. Maybe it just needs to drop the > "Key" and become > "ii_AttrNumbers". It would be interesting to hear what others think of that. > > I'm also wondering if indexkeys is still a good name for the IndexOptInfo > struct member. > Including columns are not really keys, but I feel renaming that might cause a > fair bit of code churn, so I'd be interested to hear what other's have to say. > > > I agree that KeyAttrNumbers and indexkeys are a bit confusing names, but I'd > like to keep them at least in this patch. > It's may be worth doing "index structures refactoring" as a separate patch. I agree. A separate patch sounds like the best course of action, but authoring that can wait until after this is committed (I think). -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
