Re: WIP: Covering + unique indexes.

On Mon, Jan 9, 2017 at 8:32 PM, Anastasia Lubennikova
<a(dot)lubennikova(at)postgrespro(dot)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 INHERITS INITIALLY INLINE_P
INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
INTERSECT INTERVAL INTO INVOKER IS 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