Re: [PATCH] Covering SPGiST index

From: Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru>, Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] Covering SPGiST index
Date: 2020-11-26 17:48:31
Message-ID: CALT9ZEEszJUwsXMWknXQ3k_YbGtQaQwiYxxEGZ-pFGRUDSXdtQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>
> > 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?
>
> Yeah, that was pretty much the same conclusion I came to. For
> INDEX_MAX_KEYS values up to 32, the nulls bitmap will fit into what's
> now padding space on 64-bit machines. For backwards compatibility,
> we'd have to be careful that the code knows there's no nulls bitmap in
> an index without included columns, so I'm not sure how messy that will
> be. But it's worth trying that way to see how it comes out.
>

I made a refactoring of the patch code according to the discussion:
1. Changed a leaf tuple format to: header - (optional) bitmask - key value
- (optional) INCLUDE values
2. Re-use existing code of heap_fill_tuple() to fill data part of a leaf
tuple
3. Splitted index_deform_tuple() into two portions: (a) bigger 'inner' one
- index_deform_anyheader_tuple() - to make processing of index-like tuples
(now IndexTuple and SpGistLeafTuple) working independent from type of tuple
header. (b) a small 'outer' index_deform_tuple() and spgDeformLeafTuple()
to make all header-specific processing and then call the inner (a)
4. Inserted a tuple descriptor into the SpGistCache chunk of memory. So
cleaning the cached chunk will also invalidate the tuple descriptor and not
make it dangling or leaked. This also allows not to build it every time
unless the cache is invalidated.
5. Corrected amroutine->amcaninclude according to new upstream fix.
6. Returned big chunks that were shifted in spgist_private.h to their
initial places where possible and made other cosmetic changes to improve
the patch.

PFA v.11 of the patch.
Do you think the proposed changes are in the right direction?

Thank you!
--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

Attachment Content-Type Size
v11-0001-Covering-SP-GiST-index-support-for-INCLUDE-colum.patch application/octet-stream 68.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2020-11-26 19:27:14 Re: [PoC] Non-volatile WAL buffer
Previous Message Fujii Masao 2020-11-26 16:51:20 Re: [patch] CLUSTER blocks scanned progress reporting