Date: Wed, 29 Mar 2023 21:42:45 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	pgsql-bugs@lists.postgresql.org, kyzevan23@mail.ru
Subject: Re: BUG #16329: Valgrind detects an invalid read when building a
 gist index with buffering
Message-ID: <20230330044245.GA449998@rfd.leadboat.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4095379.1680128498@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20230330044245.GA449998%40rfd.leadboat.com>
Precedence: bulk

On Wed, Mar 29, 2023 at 06:21:38PM -0400, Tom Lane wrote:
> *** a/src/test/regress/sql/gist.sql
> --- b/src/test/regress/sql/gist.sql
> *************** select p from gist_tbl order by circle(p
> *** 170,175 ****
> --- 170,180 ----
>   select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
>   
>   -- Force an index build using buffering.
> + insert into gist_tbl
> + select box(point(0.05*i, 0.05*i), point(0.05*i, 0.05*i)),
> +        point(0.05*i, 0.05*i),
> +        circle(point(0.05*i, 0.05*i), 1.0)
> + from generate_series(10001,30000) as i;
>   create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p)
>     with (buffering=on, fillfactor=50);
>   
> 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.

I say +200ms is fine.  This thread is a reminder of the limits of raw code
coverage as a measure of tests.  (I expect this will add tens of minutes to
the soft-float buildfarm animal runs, but they deserve it.)