Re: WIP: Covering + unique indexes.

26.02.2017 06:09, Amit Kapila:
> On Thu, Feb 16, 2017 at 6:43 PM, Anastasia Lubennikova
> <a(dot)lubennikova(at)postgrespro(dot)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