From: Alexander Lakhin <exclusion@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
Date: Mon, 11 May 2020 23:00:01 +0300
Message-ID: <93ca4540-0016-8217-2b31-535613499987@gmail.com> (raw)
In-Reply-To: <10261.1588705157@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>
Hello Tom,
05.05.2020 21:59, Tom Lane wrote:
>> Could you please let me know if the fix is incorrect (or not elaborated
>> enough to be included in the upcoming releases) or this issue is false
>> positive?
> I took a look at this. I see the hazard, but I'm not sure that
> I like your proposed quick-fix. Essentially, the problem is that
> gistBuildCallback is supposing that the tuple it creates will survive
> the execution of its subroutines, which in fact it won't always.
> But that means that at some point the tuple pointer it's passed down to
> those subroutines becomes a dangling pointer. What guarantee would we
> have that the subroutines don't access the tuple after that point?
Thanks for your review!
Yes, I'm not excited by this fix too, so I'll try to find another not so
quick way to fix it.
> Or, if we do just hack it as you suggest, there had better be a couple
> of fat comments in gistBufferingBuildInsert warning about the hazards.
>
> I was somewhat dismayed to realize from looking at the code coverage
> report that we have exactly zero test coverage of the gist buffering
> build code paths today. That's just awful; how did the feature get
> committed with no test coverage? Your proposed test additions raise the
> coverage in gistbuild.c and gistbuildbuffers.c to something respectable,
> at least. But it looks like there are still some potentially-delicate
> paths that aren't tested, notably the "have to stop buffering" business.
> Can we do better at reasonable cost, or are those paths just never
> reached without huge data volume? (Could we make them more reachable
> by turning down maintenance_work_mem or some other setting?)
Please look at the improved test that makes the code coverage for
gistbuildbuffers.c almost 100%.
Best regards,
Alexander
Attachments:gist-buffering-test.patch (text/x-patch, 3.3K) downloaddiff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.outindex 90edb4061d4..881623c7552 100644--- a/src/test/regress/expected/gist.out+++ b/src/test/regress/expected/gist.out@@ -3,7 +3,7 @@
--
-- There are other tests to test different GiST opclasses. This is for
-- testing GiST code itself. Vacuuming in particular.
-create table gist_point_tbl(id int4, p point);+create table gist_point_tbl(id int4, p point, t text);
create index gist_pointidx on gist_point_tbl using gist(p);
-- Verify the fillfactor and buffering options
create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering = on, fillfactor=50);
@@ -21,10 +21,15 @@ create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=101
ERROR: value 101 out of bounds for option "fillfactor"
DETAIL: Valid values are between "10" and "100".
-- Insert enough data to create a tree that's a couple of levels deep.
-insert into gist_point_tbl (id, p)-select g, point(g*10, g*10) from generate_series(1, 10000) g;+insert into gist_point_tbl (id, p, t)+select g, point(g*10, g*10), repeat('x', 250) from generate_series(1, 20000) g;+insert into gist_point_tbl (id, p, t)+select g, point(g*10, g*10), repeat('x', 250) from generate_series(1, 20000) g;
insert into gist_point_tbl (id, p)
select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g;
+-- Test buffering index build with data+create index gist_pointidx6 on gist_point_tbl using gist(p) include(t) with (buffering = on, fillfactor=50);+drop index gist_pointidx6;
-- To test vacuum, delete some entries from all over the index.
delete from gist_point_tbl where id % 2 = 1;
-- And also delete some concentration of values.
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sqlindex b9d398ea941..4e39cf176dc 100644--- a/src/test/regress/sql/gist.sql+++ b/src/test/regress/sql/gist.sql@@ -4,7 +4,7 @@
-- There are other tests to test different GiST opclasses. This is for
-- testing GiST code itself. Vacuuming in particular.
-create table gist_point_tbl(id int4, p point);+create table gist_point_tbl(id int4, p point, t text);
create index gist_pointidx on gist_point_tbl using gist(p);
-- Verify the fillfactor and buffering options
@@ -19,12 +19,19 @@ create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=9);
create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=101);
-- Insert enough data to create a tree that's a couple of levels deep.
-insert into gist_point_tbl (id, p)-select g, point(g*10, g*10) from generate_series(1, 10000) g;+insert into gist_point_tbl (id, p, t)+select g, point(g*10, g*10), repeat('x', 250) from generate_series(1, 20000) g;++insert into gist_point_tbl (id, p, t)+select g, point(g*10, g*10), repeat('x', 250) from generate_series(1, 20000) g;
insert into gist_point_tbl (id, p)
select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g;
+-- Test buffering index build with data+create index gist_pointidx6 on gist_point_tbl using gist(p) include(t) with (buffering = on, fillfactor=50);+drop index gist_pointidx6;+
-- To test vacuum, delete some entries from all over the index.
delete from gist_point_tbl where id % 2 = 1;
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: exclusion@gmail.com, tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
In-Reply-To: <93ca4540-0016-8217-2b31-535613499987@gmail.com>
* 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