Thank you very much for reviewing the patch! > 28 янв. 2019 г., в 12:15, Andreas Karlsson <andr...@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 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.
0001-Covering-GiST-v6.patch
Description: Binary data
