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

26.02.2017 06:09, Amit Kapila:

On Thu, Feb 16, 2017 at 6:43 PM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:

14.02.2017 15:46, Amit Kapila:

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.

Thank you for this suggestion. I've just wrote the code looking at examples
around,
but optincluding and optcincluding clauses seem to be redundant.
I've cleaned up the code.

I think you have cleaned only in gram.y as I could see the references
to 'including' in other parts of code.  For ex, see below code:
@@ -2667,6 +2667,7 @@ _copyConstraint(const Constraint *from) COPY_NODE_FIELD(raw_expr); COPY_STRING_FIELD(cooked_expr); COPY_NODE_FIELD(keys);
+ COPY_NODE_FIELD(including); COPY_NODE_FIELD(exclusions); COPY_NODE_FIELD(options); COPY_STRING_FIELD(indexname);
@@ -3187,6 +3188,7 @@ _copyIndexStmt(const IndexStmt *from) COPY_STRING_FIELD(accessMethod); COPY_STRING_FIELD(tableSpace); COPY_NODE_FIELD(indexParams);
+ COPY_NODE_FIELD(indexIncludingParams);

There is a lot of variables like 'including*' in the patch.
Frankly, I don't see a reason to rename them. It's clear that they
refers to included attributes, whatever we call them "include", "included" or "including".

@@ -425,6 +425,13 @@ ConstructTupleDescriptor(Relation heapRelation, /*
+ * Code below is concerned to the opclasses which are not used
+ * with the included columns.
+ */
+ if (i >= indexInfo->ii_NumIndexKeyAttrs)
+ continue;
+

There seems to be code below the above check which is not directly
related to opclasses, so not sure if you have missed that or is there
any other reason to ignore that.  I am referring to following code in
the same function after the above check:
/*
* If a key type different from the heap value is specified, update
*
the type-related fields in the index tupdesc.
*/
if (OidIsValid(keyType) &&
keyType != to->atttypid)

Good point,
I skip some steps that should be executed for all attributes.
It is harmless though, since for btree (and other access methods, except hash) amkeytype is always invalid.
But I agree that the code can be clarified.

New patch with minor changes is attached.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company