Re: [HACKERS] WIP: Covering + unique indexes. - Mailing list pgsql-hackers

14.02.2017 15:46, Amit Kapila:
> 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.
Fixed.
> 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?
No, they cannot be a part of any constraint, so I fixed a check.

> 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.
Both are fixed.
> 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).

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.

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

Yes, I've included them in the last version of the patch.
> [1] - https://www.postgresql.org/message-id/3863bca17face15c6acd507e0173a6dc%40xs4all.nl

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers