Re: [PATCH] Covering SPGiST index

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>
Cc: David Steele <david(at)pgmasters(dot)net>, "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: 2021-04-05 22:52:43
Message-ID: 931852.1617663163@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-04-05 23:29:27 Re: New IndexAM API controlling index vacuum strategies
Previous Message Peter Geoghegan 2021-04-05 22:39:10 Re: New IndexAM API controlling index vacuum strategies