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
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

-#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:

Reply via email to