Re: [HACKERS] WIP: Covering + unique indexes.

On Mon, Jan 9, 2017 at 8:32 PM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
> Updated version of the patch is attached. Besides code itself, it contains
> new regression test,
> documentation updates and a paragraph in nbtree/README.
>

The latest patch doesn't apply cleanly.

Few assorted comments:
1.
@@ -4806,16 +4810,25 @@ RelationGetIndexAttrBitmap(Relation relation,
IndexAttrBitmapKind attrKind)
{
..
+ /*
+ * Since we have covering indexes with non-key columns,
+ * we must handle them accurately here. non-key columns
+ * must be added into indexattrs, since they are in index,
+ * and HOT-update shouldn't miss them.
+ * Obviously, non-key columns couldn't be referenced by
+ * foreign key or identity key. Hence we do not include
+ * them into uindexattrs and idindexattrs bitmaps.
+ */ if (attrnum != 0) { indexattrs = bms_add_member(indexattrs,   attrnum -
FirstLowInvalidHeapAttributeNumber);

- if (isKey)
+ if (isKey && i < indexInfo->ii_NumIndexKeyAttrs) uindexattrs = bms_add_member(uindexattrs,   attrnum -
FirstLowInvalidHeapAttributeNumber);

- if (isIDKey)
+ if (isIDKey && i < indexInfo->ii_NumIndexKeyAttrs) idindexattrs = bms_add_member(idindexattrs,   attrnum -
FirstLowInvalidHeapAttributeNumber);
..
}

Can included columns be part of primary key?  If not, then won't you
need a check similar to above for Primary keys?


2.
+ int indnkeyattrs; /* number of index key attributes*/
+ int indnattrs; /* total number of index attributes*/
+ Oid   *indkeys; /* In spite of the name 'indkeys' this field
+ * contains both key and nonkey
attributes*/

Before the end of the comment, one space is needed.

3.
}
- /* * For UNIQUE and PR
IMARY KEY, we just have a list of column names. *

Looks like spurious line removal.

4.
+ IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE INCLUDING INCREMENT INDEX INDEXES INHERIT
INHERITSINITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO
INVOKERIS ISNULL ISOLATION
 
@@ -3431,17 +3433,18 @@ ConstraintElem: n->initially_valid = !n->skip_validation; $$ = (Node *)n; }
- | UNIQUE '(' columnList ')' opt_definition OptConsTableSpace
+ | UNIQUE '(' columnList ')' opt_c_including opt_definition OptConsTableSpace

If we want to use INCLUDE in syntax, then it might be better to keep
the naming reflect the same.  For ex. instead of opt_c_including we
should use opt_c_include.

5.
+opt_c_including: INCLUDE optcincluding { $$ = $2; }
+ | /* EMPTY */ { $$
= NIL; }
+ ;
+
+optcincluding : '(' columnList ')' { $$ = $2; }
+ ;
+

It seems optcincluding is redundant, why can't we directly specify
along with INCLUDE?  If there was some other use of optcincluding or
if there is a complicated definition of the same then it would have
made sense to define it separately.  We have a lot of similar usage in
gram.y, refer opt_in_database.

6.
+optincluding : '(' index_including_params ')' { $$ = $2; }
+ ;
+opt_including: INCLUDE optincluding { $$ = $2; }
+ | /* EMPTY */ { $$ = NIL; }
+ ;

Here the ordering of above clauses seems to be another way.  Also, the
naming of both seems to be confusing. I think either we can eliminate
*optincluding* by following suggestion similar to the previous point
or name them somewhat clearly (like opt_include_clause and
opt_include_params/opt_include_list).

7. Can you include doc fixes suggested by Erik Rijkers [1]?  I have
checked them and they seem to be better than what is there in the
patch.


[1] - https://www.postgresql.org/message-id/3863bca17face15c6acd507e0173a6dc%40xs4all.nl

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com