Re: Covering GiST indexes - Mailing list pgsql-hackers

Thank you very much for reviewing the patch!

> 28 янв. 2019 г., в 12:15, Andreas Karlsson <andreas@proxel.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
equalto !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
variousGiST support functions. Also in theory we should be able to skip initializing these array entires, but it is
probablyfor 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
anybetter suggestion right now. 
B-tree is copying tupdesc every time they truncate tuple. We need tuple truncation a little more often: when we are
doingpage 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
probablyhave at least one such test case. 

Actually, I did not understand this test case. Can you elaborate more on this? How included attributes should
participatein 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
shouldbe "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
"Includedattributes 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
cleanerpatch. 
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.