From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru> |
Cc: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Andrey Borodin <amborodin(at)acm(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Anastasia Lubennikova <lubennikovaav(at)gmail(dot)com>, Brad DeJong <Brad(dot)Dejong(at)infor(dot)com> |
Subject: | Re: WIP: Covering + unique indexes. |
Date: | 2017-02-14 12:46:06 |
Message-ID: | CAA4eK1+THMD_G6tb0tpXSy_YKyYFcZCXFvpEOWQai3RqVGfgrQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2017-02-14 12:53:23 | Re: pg_waldump's inclusion of backend headers is a mess |
Previous Message | Alexander Korotkov | 2017-02-14 12:00:16 | Re: Should we cacheline align PGXACT? |