> > 3) I didn't quite get the meaning of the assertion, that is added in a few > places: > Assert(so->state.includeTupdesc->natts); > Should it be Assert(so->state.includeTupdesc->natts > 1) ? > It is rather Assert(so->state.includeTupdesc->natts > 0) as INCLUDE tuple descriptor should not be initialized and filled in case of index without INCLUDE attributes and doesn't contain any info about key attribute which is processed by SpGist existing way separately for different SpGist tuple types i.e. leaf, prefix=inner and label tuples. So only INCLUDE attributes are counted there. This and similar Asserts are for the case includeTupdesc becomes mistakenly initialized by some future code change.
I completely agree with all the other suggestions and made corrections (see v8). Thank you very much for your review! Also there is a separate patch 0002 to add VACUUM ANALYZE to index_including test which is not necessary for covering spgist. One more point to note: in spgist_private.h I needed to shift down whole block between *"typedef struct SpGistSearchItem"* *and * *"} SpGistCache;"* to position it below tuples types declarations to insert pointer "SpGistLeafTuple leafTuple"; into struct SpGistSearchItem. This is the only change in this block and I apologize for possible inconvenience to review this change. -- Best regards, Pavel Borisov Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>
v8-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch
Description: Binary data
v1-0002-Add-VACUUM-ANALYZE-to-index-including-test.patch
Description: Binary data
