Thread: [PATCH] Covering SPGiST index

> 7 авг. 2020 г., в 16:59, Pavel Borisov <pashkin.elfe@gmail.com> написал(а):
>
> As usual I very much appreciate your feedback

Thanks for the patch! Looks interesting.

On a first glance the whole concept of non-multicolumn index with included attributes seems...well, just difficult to
understand.
But I expect for SP-GiST this must be single key with multiple included attributes, right?
I couldn't find a test that checks impossibility of on 2-column SP-GiST, only few asserts about it. Is this checked
somewhereelse? 

Thanks!

Best regards, Andrey Borodin.

Hi!

> 17 авг. 2020 г., в 21:04, Pavel Borisov <pashkin.elfe@gmail.com> написал(а):
>
> Postgres Professional: http://postgrespro.com
> <v6-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch>

I'm looking into the patch. I have few notes:

1. I see that in src/backend/access/spgist/README you describe SP-GiST tuple as sequence of {Value, ItemPtr to heap,
Includedattributes}. Is it different from regular IndexTuple where tid is within TupleHeader? 

2. Instead of cluttering tuple->nextOffset with bit flags we could just change Tuple Header for leaf tuples with
coveringindexes. Interpret tuples for indexes with included attributes differently, iff it makes code cleaner. There
areso many changes with SGLT_SET_OFFSET\SGLT_GET_OFFSET that it seems viable to put some effort into research of other
waysto represent two bits for null mask and varatts. 

3. Comment "* SPGiST dead tuple: declaration for examining non-live tuples" does not precede relevant code. because
structSpGistDeadTupleData was not moved. 

Thanks!

Best regards, Andrey Borodin.

I added some extra comments and mentions in manual to make all the things clear (see v7 patch) 

 

The patch implements the proposed functionality, passes tests, and in general looks good to me.
I don't mind using a macro to differentiate tuples with and without included attributes. Any approach will require code changes. Though, I don't have a strong opinion about that.
 
A bit of nitpicking:

1) You mention backward compatibility in some comments. But, after this patch will be committed, it will be uneasy to distinct new and old phrases.  So I suggest to rephrase them.  You can either refer a specific version or just call it "compatibility with indexes without included attributes".

2) SpgLeafSize() function name seems misleading, as it actually refers to a tuple's size, not a leaf page. I suggest to rename it to SpgLeafTupleSize().

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) ?

4) There are a few typos in comments and docs:
s/colums/columns
s/include attribute/included attribute

and so on.

5) This comment in index_including.sql is outdated:
  * 7. Check various AMs. All but btree and gist must fail.

6) New test lacks SET enable_seqscan TO off;
in addition to SET enable_bitmapscan TO off;

I also wonder, why both index_including_spgist.sql and index_including.sql tests are stable without running 'vacuum analyze' before the EXPLAIN that shows Index Only Scan plan. Is autovacuum just always fast enough to fill a visibility map, or I miss something?

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> 27 авг. 2020 г., в 21:03, Pavel Borisov <pashkin.elfe@gmail.com> написал(а):
>
> see v8

For me is the only concerning point is putting nullmask and varatt bits into tuple->nextOffset.
But, probably, we can go with this.

But let's change macro a bit. When I see
SGLT_SET_OFFSET(leafTuple->nextOffset, InvalidOffsetNumber);
I expect that leafTuple->nextOffset is function argument by value and will not be changed.
For example see ItemPointerSetOffsetNumber() - it's not exposing ip_posid.

Also, I'd propose instead of
>*(leafChainDatums + i * natts) and leafChainIsnulls + i * natts
using something like
>int some_index = i * natts;
>leafChainDatumsp[some_index] and &leafChainIsnulls[some_index]
But, probably, it's a matter of taste...

Also I'm not sure would it be helpful to use instead of
>isnull[0] and leafDatum[0]
more complex
>#define SpgKeyIndex 0
>isnull[SpgKeyIndex] and leafDatum[SpgKeyIndex]
There is so many [0] in the patch...

Thanks!

Best regards, Andrey Borodin.

> 31 авг. 2020 г., в 16:57, Pavel Borisov <pashkin.elfe@gmail.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.

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> [ v10-0001-Covering-SP-GiST-index-support-for-INCLUDE-colum.patch ]

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 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.)

            regards, tom lane

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> 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.

            regards, tom lane

--

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> I've noticed CI error due to the fact that MSVC doesn't allow arrays of
> flexible size arrays and made a fix for the issue.
> Also did some minor refinement in tuple creation.
> PFA v12 of a patch.

The cfbot's still unhappy --- looks like you omitted a file from the
patch?

            regards, tom lane

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> In a v14 I forgot to add the test. PFA v15

I've committed this with a lot of mostly-cosmetic changes.
The not-so-cosmetic bits had to do with confusion between
the input data type and the leaf type, which isn't really
your fault because it was there before :-(.

One note is that I dropped the added regression test script
(index_including_spgist.sql) entirely, because I couldn't
see that it did anything that justified a permanent expenditure
of test cycles.  It looks like you made that by doing s/gist/spgist/g
on index_including_gist.sql, which might be all right except that
that script was designed to test GiST-specific implementation concerns
that aren't too relevant to SP-GiST.  AFAICT, removing that script had
exactly zero effect on the test coverage shown by gcov.  There are
certainly bits of spgist that are depressingly under-covered, but I'm
afraid we need custom-designed test cases to get at them.

(wanders away wondering if the isolationtester could be used to test
the concurrency-sensitive parts of spgvacuum.c ...)

            regards, tom lane