Re: WIP: Covering + unique indexes.

04.01.2016 11:49, David Rowley:
> On 2 December 2015 at 01:53, Anastasia Lubennikova
> <a(dot)lubennikova(at)postgrespro(dot)ru <mailto:a(dot)lubennikova(at)postgrespro(dot)ru>>
> wrote:
>
> Finally, completed patch "covering_unique_3.0.patch" is here.
> It includes the functionality discussed above in the thread,
> regression tests and docs update.
> I think it's quite ready for review.
>
>
> Hi Anastasia,
>
> I've maybe mentioned before that I think this is a great feature and I
> think it will be very useful to have, so I've signed up to review the
> patch, and below is the results of my first pass from reading the
> code. Apologies if some of the things seem like nitpicks, I've
> basically just listed everything I've noticed during, no matter how small.

First of all, I would like to thank you for writing such a detailed review.
All mentioned style problems, comments and typos are fixed in the patch
v4.0.
> + An access method that supports this feature sets
> <structname>pg_am</>.<structfield>amcanincluding</> true.
>
> I don't think this belongs under the "Index Uniqueness Checks" title.
> I think the "Columns included with clause INCLUDING aren't used to
> enforce uniqueness." that you've added before it is a good idea, but
> perhaps the details of amcanincluding are best explained elsewhere.
agree
>
> + This clause specifies additional columns to be appended to
> the set of index columns.
> + Included columns don't support any constraints
> <literal>(UNIQUE, PRMARY KEY, EXCLUSION CONSTRAINT)</>.
> + These columns can improve the performance of some queries
> through using advantages of index-only scan
> + (Or so called <firstterm>covering</firstterm> indexes.
> Covering index is the index that
> + covers all columns required in the query and prevents a table
> access).
> + Besides that, included attributes are not stored in index
> inner pages.
> + It allows to decrease index size and furthermore it provides
> a way to extend included
> + columns to store atttributes without suitable opclass (not
> implemented yet).
> + This clause could be applied to both unique and nonunique
> indexes.
> + It's possible to have non-unique covering index, which
> behaves as a regular
> + multi-column index with a bit smaller index-size.
> + Currently, only the B-tree access method supports this feature.
>
> "PRMARY KEY" should be "PRIMARY KEY". I ended up rewriting this
> paragraph as follows.
>
> "An optional <literal>INCLUDING</> clause allows a list of columns to
> be specified which will be included in the index, in the non-key
> portion of the index. Columns which are part of this clause cannot
> also exist in the indexed columns portion of the index, and vice
> versa. The <literal>INCLUDING</> columns exist solely to allow more
> queries to benefit from <firstterm>index only scans</> by including
> certain columns in the index, the value of which would otherwise have
> to be obtained by reading the table's heap. Having these columns in
> the <literal>INCLUDING</> clause in some cases allows
> <productname>PostgreSQL</> to skip the heap read completely. This also
> allows <literal>UNIQUE</> indexes to be defined on one set of columns,
> which can include another set of column in the <literal>INCLUDING</>
> clause, on which the uniqueness is not enforced upon. This can also be
> useful for non-unique indexes as any columns which are not required
> for the searching or ordering of records can defined in the
> <literal>INCLUDING</> clause, which can often reduce the size of the
> index."
>
> Maybe not perfect, but maybe it's an improvement?
>
>
Yes, this explanation is much better. I've just added couple of notes.

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