From: Tom Lane <tgl@sss.pgh.pa.us>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Cc: kyzevan23@mail.ru
Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
Date: Wed, 29 Mar 2023 18:21:38 -0400
Message-ID: <4095379.1680128498@sss.pgh.pa.us> (raw)
In-Reply-To: <3990135.1680104273@sss.pgh.pa.us>
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>
I wrote:
> My apologies for having let this slip through the cracks. I think
> I'd wanted to understand why the committed version of the GiST
> index tests doesn't expose the problem, and I never got time to
> study that. I still haven't tracked it down, but the proposed patch
> seems clearly safe so I've gone ahead and pushed it.
I couldn't quite let this go, and after some testing I see the issue.
The code coverage report correctly shows that we reach almost all of
gistProcessEmptyingQueue in the regression tests, but what it fails to
expose is that the MemoryContextReset call is only reached in calls
from gistEmptyAllBuffers, which occur only at the end of the index
build. We never get there from gistBufferingBuildInsert, which is
the troublesome path. I haven't grokked gistProcessEmptyingQueue
fully, but if the comment in gistBufferingBuildInsert is correct:
/* If we filled up (half of a) buffer, process buffer emptying. */
then this is just a matter of not having enough data volume --- as
indeed Egor's test case proves, if you break it down a little: the
failure doesn't happen in the index builds with 10k or 20k tuples,
you need 30k. The same was true in Alexander's original report.
So we could fix the lack of test coverage with something about
like this:
inline patch 1: src/test/regress/sql/gist.sql (+5/-0 lines) download inline-1.patchdiff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sqlindex 6f1fc65..a42d3ee 100644
*** 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.
Thoughts?
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, pashkin.elfe@gmail.com, 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
In-Reply-To: <4095379.1680128498@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox