Re: WIP: Covering + unique indexes.

04.01.2016 11:49, David Rowley:
> On 2 December 2015 at 01:53, Anastasia Lubennikova 
> <a.lubennikova@postgrespro.ru <mailto:a.lubennikova@postgrespro.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