Re: [PATCH] Covering SPGiST index

>
> I've started to review this, and I've got to say that I really disagree
> with this choice:
>
> + * If there are INCLUDE columns, they are stored after a key value, each
> + * starting from its own typalign boundary. Unlike IndexTuple, first
> INCLUDE
> + * value does not need to start from MAXALIGN boundary, so SPGiST uses
> private
> + * routines to access them.
>
> This seems to require far more new code than it could possibly be worth,
> because most of the time anything you could save here is just going
> to disappear into end-of-tuple alignment space anyway -- recall that
> the overall index tuple length is going to be MAXALIGN'd no matter what.
> I think you should yank this out and try to rely on standard tuple
> construction/deconstruction code instead.
>
I'd say that much of the SELECT performance gain of SP-GiST over GiST is
due to its lightweight pages, each containing more tuples so we can have
less page fetches. And this is the main goal of having lightweight tuples.
PFA my performance measurements for box+cidr selects, with gist and spgist
indexes built on box key-column and cidr (optionally) include column.

The way that seems acceptable to me is to add (optional) nulls mask into
the end of existing style SpGistLeafTuple header and use indextuple
routines to attach attributes after it. In this case, we can reduce the
amount of code at the cost of adding one extra MAXALIGN size to the overall
tuple size on 32-bit arch as now tuple header size of 12 bit already fits 3
MAXALIGNS (on 64 bit the header now is shorter than 2 maxaligns (12 bytes
of 16) and nulls mask will be free of cost). If you mean this I try to make
changes soon. What do you think of it?

I also find it unacceptable that you stuck a tuple descriptor pointer into
> the rd_amcache structure. The relcache only supports that being a flat
> blob of memory. I see that you tried to hack around that by having
> spgGetCache reconstruct a new tupdesc every time through, but (a) that's
> actually worse than having no cache at all, and (b) spgGetCache doesn't
> really know much about the longevity of the memory context it's called in.
> This could easily lead to dangling tuple pointers, serious memory bloat
> from repeated tupdesc construction, or quite possibly both. Safer would
> be to build the tupdesc during initSpGistState(), or maybe just make it
> on-demand. In view of the previous point, I'm also wondering if there's
> any way to get the relcache's regular rd_att tupdesc to be useful here,
> so we don't have to build one during scans at all.
>
> (I wondered for a bit about whether you could keep a long-lived private
> tupdesc in the relcache's rd_indexcxt context. But it looks like
> relcache.c sometimes resets rd_amcache without also clearing the
> rd_indexcxt, so that would probably lead to leakage.)
>
I will consider this for sure, thanks.