Message-ID: <cad7055f-0d76-cc31-71d5-f8b600ebb116@gmail.com>
Date: Thu, 30 Mar 2023 08:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist
 index with buffering
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>, Pavel Borisov <pashkin.elfe@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org, kyzevan23@mail.ru
References: <16329-7a6aa9b6fa1118a1@postgresql.org>
 <ca1f896b-e5d1-53a7-5bc3-9451ab9aca22@gmail.com>
 <e91e45fd-a614-bb87-e9fc-c556c2b95fe5@gmail.com>
 <3dda6945-c147-5afc-7720-38ec6848dafa@gmail.com>
 <10261.1588705157@sss.pgh.pa.us>
 <CALT9ZEEJ1pqAnkcO4H7AWOBhLN0KJDa9A=pDn9NC7pbqkwEAeQ@mail.gmail.com>
 <3990135.1680104273@sss.pgh.pa.us> <4095379.1680128498@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <4095379.1680128498@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/cad7055f-0d76-cc31-71d5-f8b600ebb116%40gmail.com>
Precedence: bulk

30.03.2023 01:21, Tom Lane wrote:
>    
> The question is whether it's worth the extra test cycles forevermore.
> On my machine, the time for the gist test script goes from ~400ms to
> ~600ms.  That's still less than some of the concurrent scripts (brin,
> privileges, and join_hash all take more than that for me), so maybe
> it's fine.  But it seems like a lot for a change that doesn't move the
> raw code coverage results by even one line.
>
> Thoughts?

I would say that providing the behavior coverage is the next thing to do after
the code coverage reached 100%.  I can easily imagine bugs hiding in a fully
covered code (#17858 is the recent example).

As to that concrete place, I would incline to improve the test while it stays
in line with other tests, but I lack a whole picture of the gist testing.
It doesn't seem to me that the regress/sql/gist.sql is too comprehensive.
I've just looked at the scan-build's results and found 9 warnings related to
gist. A quick glance at the code confirms that there are indeed
interesting cases, which are not exercised for now (e.g., call
gistRelocateBuildBuffersOnSplit() with
IndexRelationGetNumberOfKeyAttributes(r) > 1).
So if I could prove that 2-3 of them are really bugs during the next week,
maybe then assess that test change alongside with others (if any).
(Maybe we'll want to build that index over different columns, for example.)

Best regards,
Alexander