Re: Covering GiST indexes

Thank you very much for reviewing the patch!

> 28 янв. 2019 г., в 12:15, Andreas Karlsson <andreas(at)proxel(dot)se> написал(а):
>
> = Code
>
> * Have some minor style issues like that there should be spaces around || (in gistcanreturn()) and ? and : (in gistFormTuple()).
Fixed.
>
> * I do not see any need for adding the new parameter to gistFormTuple. As far as I can tell isTruncated is always equal to !isleaf.
You are right. I've removed isTruncated parameter.
>
> * The comment below from gistFormTuple() does not look correct. No allocation is taking place.
>
> /*
> * Allocate each included attribute.
> */
Fixed.
>
> * Why do you set a supportCollation for the included columns? As far as I can tell the collation is only used by the various GiST support functions. Also in theory we should be able to skip initializing these array entires, but it is probably for the best that we do.
Removed supportCollation.
>
> * I think you should check for the attno first in gistcanreturn() to take advantage of the short circuiting.
Done.
>
> * I am no fan of the tupdesc vs truncTupdesc separation and think that it is a potential hazard, but I do not have any better suggestion right now.
B-tree is copying tupdesc every time they truncate tuple. We need tuple truncation a little more often: when we are doing page split, we have to form all page tuples, truncated.
Initially, I've optimized only this case, but this led to prepared tupledesc for truncated tuples.
>
> * There is no test case for exclusion constraints, and I feel since that is one of the more important uses we should probably have at least one such test case.

Actually, I did not understand this test case. Can you elaborate more on this? How included attributes should participate in exclude index? What for?

> = Minor comments
>
> * I think that "the" should have been kept in the text below.
>
> - Currently, only the B-tree index access method supports this feature.
> + Currently, B-tree and GiST index access methods supports this feature.
>
Fixed.
> * I am not a native speaker but I think it should be "use the INCLUDE clause" in the diff below, and I think it also should be "without any GiST operator class".
>
> + A GiST index can be covering, i.e. use <literal>INCLUDE</literal> clause.
> + Included columns can have data types without GiST operator class. Included
> + attributes will be stored uncompressed.
>
Fixed.
> * I think you should write something like "Included attributes always support index-only scans." rather than "Included attributes can be returned." in the comment for gistcanreturn().
>
Fixed, but slightly reworded.

> * Why the random noise in the diff below? I think using "(c3) INCLUDE (c4)" for both gist and rtree results in a cleaner patch.
I've used columns with and without opclass in INCLUDE. This led to these seemingly random changes.

PFA v6. Thanks for reviewing!

Best regards, Andrey Borodin.