Re: [PATCH] Covering SPGiST index

> 31 авг. 2020 г., в 16:57, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> написал(а):
>
> I agree with all of your proposals and integrated them into v9.

I have a wild idea of renaming nextOffset in SpGistLeafTupleData.
Or at least documenting in comments that this field is more than just an offset.

This looks like assert rather than real runtime check in spgLeafTupleSize()

+ if (state->includeTupdesc->natts + 1 >= INDEX_MAX_KEYS)
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_COLUMNS),
+ errmsg("number of index columns (%d) exceeds limit (%d)",
+ state->includeTupdesc->natts, INDEX_MAX_KEYS)));
Even if you go with check, number of columns is state->includeTupdesc->natts + 1.

Also I'd refactor this
+ /* Form descriptor for INCLUDE columns if any */
+ if (IndexRelationGetNumberOfAttributes(index) > 1)
+ {
+ int i;
+
+ cache->includeTupdesc = CreateTemplateTupleDesc(
+ IndexRelationGetNumberOfAttributes(index) - 1);

+ for (i = 0; i < IndexRelationGetNumberOfAttributes(index) - 1; i++)
+ {
+ TupleDescInitEntry(cache->includeTupdesc, i + 1, NULL,
+ TupleDescAttr(index->rd_att, i + 1)->atttypid,
+ -1, 0);
+ }
+ }
+ else
+ cache->includeTupdesc = NULL;
into something like
cache->includeTupdesc = NULL;
for (i = 0; i < IndexRelationGetNumberOfAttributes(index) - 1; i++)
{
if (cache->includeTupdesc == NULL)
// init tuple description
// init entry
}
But, probably it's only a matter of taste.

Beside this, I think patch is ready for committer. If Anastasia has no objections, let's flip CF entry state.

Thanks!

Best regards, Andrey Borodin.