pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #16329: Valgrind detects an invalid read when building a gist index with buffering
20+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2023-03-29 a1904c9ce5 d2a1d4b190 b5c6776c11 2adb6adad5 2dc77adc76 8e5eef50c5 Fix dereference of dangling pointer in GiST index buffering build.
2020-10-12 371668a838 12945874eb 962ab473ec Fix GiST buffering build to work when there are included columns.
2020-10-12 78c0b6ed27 Re-allow testing of GiST buffered builds.
* BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-03-29 21:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 20+ messages in thread
From: PG Bug reporting form @ 2020-03-29 21:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 16329
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 12.2
Operating system: Ubuntu 18.04
Description:
Running the following query under valgrind:
create table gist_point_tbl(id int4, p point);
insert into gist_point_tbl (id, p)
select g, point(g*10+1, g*10+1) from generate_series(1, 30000) g;
create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
= on, fillfactor=50);
leads to a memory access error:
==00:00:00:08.755 18585== Invalid read of size 2
==00:00:00:08.756 18585== at 0x1F94CA: gistBuildCallback
(gistbuild.c:495)
==00:00:00:08.756 18585== by 0x21F556: heapam_index_build_range_scan
(heapam_handler.c:1664)
==00:00:00:08.756 18585== by 0x1F972F: table_index_build_scan
(tableam.h:1522)
==00:00:00:08.756 18585== by 0x1F972F: gistbuild (gistbuild.c:196)
==00:00:00:08.756 18585== by 0x29D1FE: index_build (index.c:2909)
==00:00:00:08.756 18585== by 0x29EAC8: index_create (index.c:1221)
==00:00:00:08.756 18585== by 0x348C60: DefineIndex (indexcmds.c:1005)
==00:00:00:08.756 18585== by 0x52B518: ProcessUtilitySlow
(utility.c:1372)
==00:00:00:08.756 18585== by 0x52AAC5: standard_ProcessUtility
(utility.c:927)
==00:00:00:08.756 18585== by 0x52AB73: ProcessUtility (utility.c:360)
==00:00:00:08.756 18585== by 0x527025: PortalRunUtility (pquery.c:1175)
==00:00:00:08.756 18585== by 0x527C6F: PortalRunMulti (pquery.c:1321)
==00:00:00:08.756 18585== by 0x5289DE: PortalRun (pquery.c:796)
==00:00:00:08.756 18585== Address 0x8a4ca1e is 398 bytes inside a block of
size 8,192 alloc'd
==00:00:00:08.756 18585== at 0x4C2FBFF: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:08.756 18585== by 0x6781E6: AllocSetContextCreateInternal
(aset.c:477)
==00:00:00:08.756 18585== by 0x3BF065: ExecInitFunctionScan
(nodeFunctionscan.c:508)
==00:00:00:08.756 18585== by 0x3ACE6F: ExecInitNode
(execProcnode.c:247)
==00:00:00:08.756 18585== by 0x3D03E1: ExecInitModifyTable
(nodeModifyTable.c:2363)
==00:00:00:08.756 18585== by 0x3ACD4C: ExecInitNode
(execProcnode.c:174)
==00:00:00:08.756 18585== by 0x3A70F3: InitPlan (execMain.c:1020)
==00:00:00:08.756 18585== by 0x3A7299: standard_ExecutorStart
(execMain.c:266)
==00:00:00:08.756 18585== by 0x3A739B: ExecutorStart (execMain.c:148)
==00:00:00:08.756 18585== by 0x527936: ProcessQuery (pquery.c:156)
==00:00:00:08.756 18585== by 0x527B34: PortalRunMulti (pquery.c:1283)
==00:00:00:08.756 18585== by 0x5289DE: PortalRun (pquery.c:796)
Reproduced at least on REL_10_STABLE..master.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-03-30 05:00 Alexander Lakhin <exclusion@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2020-03-30 05:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
30.03.2020 00:00, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 16329
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 12.2
> Operating system: Ubuntu 18.04
> Description:
>
> Running the following query under valgrind:
> create table gist_point_tbl(id int4, p point);
> insert into gist_point_tbl (id, p)
> select g, point(g*10+1, g*10+1) from generate_series(1, 30000) g;
> create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
> = on, fillfactor=50);
>
> leads to a memory access error:
> ==00:00:00:08.755 18585== Invalid read of size 2
> ==00:00:00:08.756 18585== at 0x1F94CA: gistBuildCallback
> (gistbuild.c:495)
I've found that the memory context where the itup is created:
oldCtx = MemoryContextSwitchTo(buildstate->giststate->tempCxt);
/* form an index tuple and point it at the heap tuple */
itup = gistFormTuple(buildstate->giststate, index, values, isnull,
true);
can be reset in gistBufferingBuildInsert->gistProcessEmptyingQueue:
/* Free all the memory allocated during index tuple
processing */
MemoryContextReset(buildstate->giststate->tempCxt);
Probably, the offending commit is d22a09dc.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-04-06 06:00 Alexander Lakhin <exclusion@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2020-04-06 06:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hello hackers,
30.03.2020 08:00, Alexander Lakhin wrote:
> 30.03.2020 00:00, PG Bug reporting form wrote:
>> The following bug has been logged on the website:
>>
>> Bug reference: 16329
>> Logged by: Alexander Lakhin
>> Email address: exclusion@gmail.com
>> PostgreSQL version: 12.2
>> Operating system: Ubuntu 18.04
>> Description:
>>
>> Running the following query under valgrind:
>> create table gist_point_tbl(id int4, p point);
>> insert into gist_point_tbl (id, p)
>> select g, point(g*10+1, g*10+1) from generate_series(1, 30000) g;
>> create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
>> = on, fillfactor=50);
>>
>> leads to a memory access error:
>> ==00:00:00:08.755 18585== Invalid read of size 2
>> ==00:00:00:08.756 18585== at 0x1F94CA: gistBuildCallback
>> (gistbuild.c:495)
>>
Please look at the patch that modifies the gist regression test to make
the issue visible and fixes it by avoiding access to the memory context
that can be reset in gistProcessEmptyingQueue().
Best regards,
Alexander
Attachments:
fix-gistbuild.patch (text/x-patch, 2.8K) download
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 671b5e9186..cee81d58ad 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -456,6 +456,10 @@ gistBuildCallback(Relation index,
itup = gistFormTuple(buildstate->giststate, index, values, isnull, true);
itup->t_tid = *tid;
+ /* Update tuple count and total size. */
+ buildstate->indtuples += 1;
+ buildstate->indtuplesSize += IndexTupleSize(itup);
+
if (buildstate->bufferingMode == GIST_BUFFERING_ACTIVE)
{
/* We have buffers, so use them. */
@@ -471,10 +475,6 @@ gistBuildCallback(Relation index,
buildstate->giststate, buildstate->heaprel, true);
}
- /* Update tuple count and total size. */
- buildstate->indtuples += 1;
- buildstate->indtuplesSize += IndexTupleSize(itup);
-
MemoryContextSwitchTo(oldCtx);
MemoryContextReset(buildstate->giststate->tempCxt);
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index 90edb4061d..8515a63c07 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -22,9 +22,12 @@ 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;
+select g, point(g*10, g*10) 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) 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.sql
index b9d398ea94..0efb265ec3 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -20,11 +20,15 @@ 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;
+select g, point(g*10, g*10) 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) 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;
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-05-02 04:45 Alexander Lakhin <exclusion@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2020-05-02 04:45 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
06.04.2020 09:00, Alexander Lakhin wrote:
> 30.03.2020 00:00, PG Bug reporting form wrote:
>>> Running the following query under valgrind:
>>> create table gist_point_tbl(id int4, p point);
>>> insert into gist_point_tbl (id, p)
>>> select g, point(g*10+1, g*10+1) from generate_series(1, 30000) g;
>>> create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
>>> = on, fillfactor=50);
>>>
>>> leads to a memory access error:
>>> ==00:00:00:08.755 18585== Invalid read of size 2
>>> ==00:00:00:08.756 18585== at 0x1F94CA: gistBuildCallback
>>> (gistbuild.c:495)
> Please look at the patch that modifies the gist regression test to make
> the issue visible and fixes it by avoiding access to the memory context
> that can be reset in gistProcessEmptyingQueue().
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?
Best regards,
Alexander
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-05-05 18:59 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Tom Lane @ 2020-05-05 18:59 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Alexander Lakhin <exclusion@gmail.com> writes:
>> Please look at the patch that modifies the gist regression test to make
>> the issue visible and fixes it by avoiding access to the memory context
>> that can be reset in gistProcessEmptyingQueue().
> 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?
I think the real issue here is confusion about which level of function
"owns" the tempCxt and gets to decide when to reset it. We can't have
both gistBuildCallback and gistProcessEmptyingQueue doing that. Maybe
we need more than one temporary context, or maybe we could just skip
the context reset in gistProcessEmptyingQueue and let the storage
accumulate till we get back to gistBuildCallback. But in any case
it's going to require more analysis.
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?)
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-05-11 20:00 Alexander Lakhin <exclusion@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2020-05-11 20:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
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) download
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index 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.sql
index 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;
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-05-12 05:00 Alexander Lakhin <exclusion@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2020-05-12 05:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
11.05.2020 23:00, Alexander Lakhin wrote:
> Please look at the improved test that makes the code coverage for
> gistbuildbuffers.c almost 100%.
That improved test exposes another bug (when running under valgrind):
==00:00:00:34.861 1439277== Conditional jump or move depends on
uninitialised value(s)
==00:00:00:34.861 1439277== at 0x1FFE6A: gistpenalty (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F86E0:
gistRelocateBuildBuffersOnSplit (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F67B7: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6B6F: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6E0E: gistProcessItup (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F701F: gistProcessEmptyingQueue (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F706D: gistBufferingBuildInsert (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7817: gistBuildCallback (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2293F3:
heapam_index_build_range_scan (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7A23: gistbuild (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B8372: index_build (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B9C8D: index_create (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== Uninitialised value was created by a heap
allocation
==00:00:00:34.861 1439277== at 0x6B646A: palloc (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F5A8A: initGISTstate (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7913: gistbuild (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B8372: index_build (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B9C8D: index_create (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x361677: DefineIndex (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x563FE3: ProcessUtilitySlow (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x56358F: standard_ProcessUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x56366E: ProcessUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x55FAED: PortalRunUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x560640: PortalRunMulti (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x5612F8: PortalRun (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:gistpenalty
fun:gistRelocateBuildBuffersOnSplit
fun:gistbufferinginserttuples
fun:gistbufferinginserttuples
fun:gistProcessItup
fun:gistProcessEmptyingQueue
fun:gistBufferingBuildInsert
fun:gistBuildCallback
fun:heapam_index_build_range_scan
fun:gistbuild
fun:index_build
fun:index_create
}
==00:00:00:34.861 1439277== Use of uninitialised value of size 8
==00:00:00:34.861 1439277== at 0x68E92D: FunctionCall3Coll (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== Uninitialised value was created by a heap
allocation
==00:00:00:34.861 1439277== at 0x6B646A: palloc (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F5A8A: initGISTstate (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7913: gistbuild (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B8372: index_build (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2B9C8D: index_create (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x361677: DefineIndex (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x563FE3: ProcessUtilitySlow (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x56358F: standard_ProcessUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x56366E: ProcessUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x55FAED: PortalRunUtility (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x560640: PortalRunMulti (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x5612F8: PortalRun (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277==
{
<insert_a_suppression_name_here>
Memcheck:Value8
fun:FunctionCall3Coll
}
==00:00:00:34.861 1439277== Jump to the invalid address stated on the
next line
==00:00:00:34.861 1439277== at 0x0: ???
==00:00:00:34.861 1439277== by 0x68E92E: FunctionCall3Coll (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1FFE9F: gistpenalty (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F86E0:
gistRelocateBuildBuffersOnSplit (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F67B7: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6B6F: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6E0E: gistProcessItup (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F701F: gistProcessEmptyingQueue (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F706D: gistBufferingBuildInsert (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7817: gistBuildCallback (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2293F3:
heapam_index_build_range_scan (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7A23: gistbuild (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== Address 0x0 is not stack'd, malloc'd or
(recently) free'd
==00:00:00:34.861 1439277==
{
<insert_a_suppression_name_here>
Memcheck:Jump
obj:*
fun:FunctionCall3Coll
fun:gistpenalty
fun:gistRelocateBuildBuffersOnSplit
fun:gistbufferinginserttuples
fun:gistbufferinginserttuples
fun:gistProcessItup
fun:gistProcessEmptyingQueue
fun:gistBufferingBuildInsert
fun:gistBuildCallback
fun:heapam_index_build_range_scan
fun:gistbuild
}
==00:00:00:34.861 1439277==
==00:00:00:34.861 1439277== Process terminating with default action of
signal 11 (SIGSEGV): dumping core
==00:00:00:34.861 1439277== Bad permissions for mapped region at
address 0x0
==00:00:00:34.861 1439277== at 0x0: ???
==00:00:00:34.861 1439277== by 0x68E92E: FunctionCall3Coll (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1FFE9F: gistpenalty (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F86E0:
gistRelocateBuildBuffersOnSplit (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F67B7: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6B6F: gistbufferinginserttuples
(in .../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F6E0E: gistProcessItup (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F701F: gistProcessEmptyingQueue (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F706D: gistBufferingBuildInsert (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7817: gistBuildCallback (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x2293F3:
heapam_index_build_range_scan (in
.../tmp_install/usr/local/pgsql/bin/postgres)
==00:00:00:34.861 1439277== by 0x1F7A23: gistbuild (in
.../tmp_install/usr/local/pgsql/bin/postgres)
2020-05-12 06:51:30.764 MSK postmaster[1439231] LOG: server process
(PID 1439277) was terminated by signal 11: Segmentation fault
2020-05-12 06:51:30.764 MSK postmaster[1439231] DETAIL: Failed process
was running: create index gist_pointidx6 on gist_point_tbl using gist(p)
include(t) with (buffering = on, fillfactor=50);
Best regards,
Alexander
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-12 12:30 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Pavel Borisov @ 2020-10-12 12:30 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
>
>
> 2020-05-12 06:51:30.764 MSK postmaster[1439231] LOG: server process (PID
> 1439277) was terminated by signal 11: Segmentation fault
> 2020-05-12 06:51:30.764 MSK postmaster[1439231] DETAIL: Failed process
> was running: create index gist_pointidx6 on gist_point_tbl using gist(p)
> include(t) with (buffering = on, fillfactor=50);
>
> Best regards,
> Alexander
>
I investigated this issue under Valgrind and found that in v13 it is no
longer detected and the patch in the discussion above is no longer needed.
But I think that increasing test coverage is useful anyway, and it's good
to commit tests from this patch (the re-formatted version is below).
Also, there is a minor correction for the case of covering index where only
part of the columns are keys, others are just INCLUDE columns.
>
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com <http://www.postgrespro.com;
Attachments: download all (2 files, .tar.gz)
0002-Extra-tests-for-GiST-index-build.patch (application/octet-stream, 3.6K) download
From 099c7b078d6577be4d5095cc6825d97e1f1968ee Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Mon, 12 Oct 2020 15:40:24 +0400
Subject: [PATCH 2/2] Extra tests for GiST index build
---
src/test/regress/expected/gist.out | 11 ++++++++---
src/test/regress/sql/gist.sql | 13 ++++++++++---
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index 90edb4061d..881623c755 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.sql
index b9d398ea94..4e39cf176d 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;
--
2.28.0
0001-Fix-for-the-case-of-GiST-buffered-build-of-index-wit.patch (application/octet-stream, 1.2K) download
From be5285b754bc02b02c8f468c6a949606a3f8c465 Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Mon, 12 Oct 2020 15:36:37 +0400
Subject: [PATCH 1/2] Fix for the case of GiST buffered build of index with
INCLUDE clause
---
src/backend/access/gist/gistbuildbuffers.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 4eab9bb83a..4ad67c88b4 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -666,7 +666,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
zero_penalty = true;
/* Loop over index attributes. */
- for (j = 0; j < r->rd_att->natts; j++)
+ for (j = 0; j < IndexRelationGetNumberOfKeyAttributes(r); j++)
{
float usize;
@@ -692,7 +692,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
which = i;
best_penalty[j] = usize;
- if (j < r->rd_att->natts - 1)
+ if (j < IndexRelationGetNumberOfKeyAttributes(r) - 1)
best_penalty[j + 1] = -1;
}
else if (best_penalty[j] == usize)
--
2.28.0
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-12 16:03 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Pavel Borisov @ 2020-10-12 16:03 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
>
> I investigated this issue under Valgrind and found that in v13 it is no
> longer detected and the patch in the discussion above is no longer needed.
>
I want to correct myself that issue is no longer detected in master, but
not in 13stable. Everything else is without changes.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-12 20:03 Tom Lane <tgl@sss.pgh.pa.us>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Tom Lane @ 2020-10-12 20:03 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Pavel Borisov <pashkin.elfe@gmail.com> writes:
>> I investigated this issue under Valgrind and found that in v13 it is no
>> longer detected and the patch in the discussion above is no longer needed.
> I want to correct myself that issue is no longer detected in master, but
> not in 13stable. Everything else is without changes.
I went to try to evaluate this, and found that with or without your
proposed testing addition, the code coverage for gistbuildbuffers.c
remains at 0.0%. The previous testing patch is the same. So that's
why the issue is "no longer detectable": the code is not reached in
HEAD.
After some investigation, I found that commit 16fa9b2b3 broke this:
if the opclass(es) have GIST_SORTSUPPORT_PROC, then the buffering
code will not be used, no matter what the index's options say.
This seems like a spectacularly bad idea from a testing standpoint,
even if it's the right thing for most users. Basically, it is now
impossible to test buffering builds at all, unless you find a gist
opclass that lacks GIST_SORTSUPPORT_PROC. Although there are a
few candidates to pick from, someone could at any time add such
a support proc and silently break your testing plan, just as
16fa9b2b3 did by adding such a proc to point_ops.
So I think 16fa9b2b3 has to be reconsidered: if we have a
buffering=on index parameter, we must go with that independently
of the availability of sort support procs. Unless I hear very
loud squawks very quickly, I'll go make that happen.
(I observe that 16fa9b2b3 also failed to update the relevant
documentation in 65.4.1...)
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-12 22:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Tom Lane @ 2020-10-12 22:16 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Pavel Borisov <pashkin.elfe@gmail.com> writes:
> Also, there is a minor correction for the case of covering index where only
> part of the columns are keys, others are just INCLUDE columns.
Yeah, that's clearly a bug, and it reinforces the point about wanting
more thorough test coverage of this area.
I pushed the bug fix, but not yet the test addition, because I'm not
very happy about the latter:
1. It nearly triples the runtime of gist.sql, from ~650 ms to ~1700 ms
on my machine. That's a pretty bad increase for something we're
proposing to drop into the core regression tests. Given that this is
hardly the only index build in that test, I wonder why it's so much
(but I did not look for the reason).
2. The test exposed the gistRelocateBuildBuffersOnSplit bug only about
one time in ten for me. I suppose this is due to the random split
choices gistchoose makes for equally-good tuples, so it's not terribly
surprising; but it is problematic for a test that we're hoping to use
to provide reliable code coverage.
I'm not really sure what we could do about #2. Perhaps, instead of
relying on random(), we could make gistchoose() use our own PRNG and
then invent a debugging function that forces the seed to a known value.
(GEQO already does something similar, although I'd not go as far as
exposing the seed as a GUC. Resetting it via some quick-hack C
function in regress.c would be enough IMO.) Or perhaps gist.sql could
be adjusted so that the test data is less full of equally-good tuples.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-22 05:50 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: Andrey Borodin @ 2020-10-22 05:50 UTC (permalink / raw)
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
> 13 окт. 2020 г., в 03:16, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> I pushed the bug fix, but not yet the test addition, because I'm not
> very happy about the latter:
>
> 1. It nearly triples the runtime of gist.sql, from ~650 ms to ~1700 ms
> on my machine. That's a pretty bad increase for something we're
> proposing to drop into the core regression tests. Given that this is
> hardly the only index build in that test, I wonder why it's so much
> (but I did not look for the reason).
>
> 2. The test exposed the gistRelocateBuildBuffersOnSplit bug only about
> one time in ten for me. I suppose this is due to the random split
> choices gistchoose makes for equally-good tuples, so it's not terribly
> surprising; but it is problematic for a test that we're hoping to use
> to provide reliable code coverage.
>
> I'm not really sure what we could do about #2. Perhaps, instead of
> relying on random(), we could make gistchoose() use our own PRNG and
> then invent a debugging function that forces the seed to a known value.
> (GEQO already does something similar, although I'd not go as far as
> exposing the seed as a GUC. Resetting it via some quick-hack C
> function in regress.c would be enough IMO.) Or perhaps gist.sql could
> be adjusted so that the test data is less full of equally-good tuples.
I think we should use not entropy-based tie breaker in GiST. We can extract some randomness from tuples using hash.
I'd be much happier if GiST behaviour was deterministic.
> This seems like a spectacularly bad idea from a testing standpoint,
> even if it's the right thing for most users. Basically, it is now
> impossible to test buffering builds at all, unless you find a gist
> opclass that lacks GIST_SORTSUPPORT_PROC. Although there are a
> few candidates to pick from, someone could at any time add such
> a support proc and silently break your testing plan, just as
> 16fa9b2b3 did by adding such a proc to point_ops.
>
> So I think 16fa9b2b3 has to be reconsidered: if we have a
> buffering=on index parameter, we must go with that independently
> of the availability of sort support procs. Unless I hear very
> loud squawks very quickly, I'll go make that happen.
FWIW I think forcing buffered build on buffering=on is a good idea. Not just from testing point of view, it simply does what user asked for.
Thanks!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-22 14:01 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrey Borodin <x4mmm@yandex-team.ru>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2020-10-22 14:01 UTC (permalink / raw)
To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> 13 окт. 2020 г., в 03:16, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>> 2. The test exposed the gistRelocateBuildBuffersOnSplit bug only about
>> one time in ten for me. I suppose this is due to the random split
>> choices gistchoose makes for equally-good tuples, so it's not terribly
>> surprising; but it is problematic for a test that we're hoping to use
>> to provide reliable code coverage.
>>
>> I'm not really sure what we could do about #2. Perhaps, instead of
>> relying on random(), we could make gistchoose() use our own PRNG and
>> then invent a debugging function that forces the seed to a known value.
>> (GEQO already does something similar, although I'd not go as far as
>> exposing the seed as a GUC. Resetting it via some quick-hack C
>> function in regress.c would be enough IMO.) Or perhaps gist.sql could
>> be adjusted so that the test data is less full of equally-good tuples.
> I think we should use not entropy-based tie breaker in GiST. We can extract some randomness from tuples using hash.
> I'd be much happier if GiST behaviour was deterministic.
If we started using our own PRNG, we could very easily make the "random"
sequence be the same in every GiST build --- again, see GEQO for prior
art. I'm a little suspicious of trying to pull some entropy out of the
tuples themselves: that seems like it'd be tremendously prone to cross-
platform and cross-version behavioral differences.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-10-24 16:40 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 20+ messages in thread
From: Andrey Borodin @ 2020-10-24 16:40 UTC (permalink / raw)
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
> 22 окт. 2020 г., в 19:01, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Andrey Borodin <x4mmm@yandex-team.ru> writes:
>>> 13 окт. 2020 г., в 03:16, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>>> 2. The test exposed the gistRelocateBuildBuffersOnSplit bug only about
>>> one time in ten for me. I suppose this is due to the random split
>>> choices gistchoose makes for equally-good tuples, so it's not terribly
>>> surprising; but it is problematic for a test that we're hoping to use
>>> to provide reliable code coverage.
>>>
>>> I'm not really sure what we could do about #2. Perhaps, instead of
>>> relying on random(), we could make gistchoose() use our own PRNG and
>>> then invent a debugging function that forces the seed to a known value.
>>> (GEQO already does something similar, although I'd not go as far as
>>> exposing the seed as a GUC. Resetting it via some quick-hack C
>>> function in regress.c would be enough IMO.) Or perhaps gist.sql could
>>> be adjusted so that the test data is less full of equally-good tuples.
>
>> I think we should use not entropy-based tie breaker in GiST. We can extract some randomness from tuples using hash.
>> I'd be much happier if GiST behaviour was deterministic.
>
> If we started using our own PRNG, we could very easily make the "random"
> sequence be the same in every GiST build --- again, see GEQO for prior
> art. I'm a little suspicious of trying to pull some entropy out of the
> tuples themselves: that seems like it'd be tremendously prone to cross-
> platform and cross-version behavioral differences.
PFA copy of GEQO approach to GiST. I haven't found proper place to document this: current random tie breaker seems undocumented.
Will describing GUC here [0] be enough?
Thanks!
Best regards, Andrey Borodin.
[0] https://www.postgresql.org/docs/13/runtime-config-developer.html
Attachments:
0001-Make-GiST-deterministic.patch (application/octet-stream, 4.1K) download
From c7bf0aaa470028372b27e05cf8b9132793ee18a8 Mon Sep 17 00:00:00 2001
From: Borodin Andrey <borodinandrew@tty1.local>
Date: Sat, 24 Oct 2020 15:47:20 +0500
Subject: [PATCH] Make GiST deterministic
This commit adds PRNG state of randomness used in GiST insertion
to GiST state. This renders all test runs detrministic.
---
src/backend/access/gist/gist.c | 7 +++++++
src/backend/access/gist/gistutil.c | 5 +++--
src/backend/utils/misc/guc.c | 11 +++++++++++
src/include/access/gist.h | 2 ++
src/include/access/gist_private.h | 3 +++
5 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 25b42e38f2..0b5842068a 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -27,6 +27,8 @@
#include "utils/memutils.h"
#include "utils/rel.h"
+double gist_seed;
+
/* non-export function prototypes */
static void gistfixsplit(GISTInsertState *state, GISTSTATE *giststate);
static bool gistinserttuple(GISTInsertState *state, GISTInsertStack *stack,
@@ -1617,6 +1619,11 @@ initGISTstate(Relation index)
giststate->supportCollation[i] = InvalidOid;
}
+ memset(&giststate->random_state, 0, sizeof(giststate->random_state));
+ memcpy(&giststate->random_state,
+ &gist_seed,
+ Min(sizeof(giststate->random_state), sizeof(gist_seed)));
+
MemoryContextSwitchTo(oldCxt);
return giststate;
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 615b5ade23..2ca53068c4 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -19,6 +19,7 @@
#include "access/htup_details.h"
#include "access/reloptions.h"
#include "catalog/pg_opclass.h"
+#include "port.h"
#include "storage/indexfsm.h"
#include "storage/lmgr.h"
#include "utils/float.h"
@@ -507,7 +508,7 @@ gistchoose(Relation r, Page p, IndexTuple it, /* it has compressed entry */
if (keep_current_best == -1)
{
/* we didn't make the random choice yet for this old best */
- keep_current_best = (random() <= (MAX_RANDOM_VALUE / 2)) ? 1 : 0;
+ keep_current_best = pg_jrand48(giststate->random_state) & 1;
}
if (keep_current_best == 0)
{
@@ -529,7 +530,7 @@ gistchoose(Relation r, Page p, IndexTuple it, /* it has compressed entry */
if (keep_current_best == -1)
{
/* we didn't make the random choice yet for this old best */
- keep_current_best = (random() <= (MAX_RANDOM_VALUE / 2)) ? 1 : 0;
+ keep_current_best = pg_jrand48(giststate->random_state) & 1;
}
if (keep_current_best == 1)
break;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a62d64eaa4..f2be4700e7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -30,6 +30,7 @@
#include <unistd.h>
#include "access/commit_ts.h"
+#include "access/gist.h"
#include "access/gin.h"
#include "access/rmgr.h"
#include "access/tableam.h"
@@ -3552,6 +3553,16 @@ static struct config_real ConfigureNamesReal[] =
0.0, 0.0, 1.0,
NULL, NULL, NULL
},
+ {
+ {"gist_seed", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Seed for random path selection in GiST insertion."),
+ NULL,
+ GUC_EXPLAIN
+ },
+ &gist_seed,
+ 0.0, 0.0, 1.0,
+ NULL, NULL, NULL
+ },
{
{"hash_mem_multiplier", PGC_USERSET, RESOURCES_MEM,
diff --git a/src/include/access/gist.h b/src/include/access/gist.h
index 4f6dae9a76..9fde15a399 100644
--- a/src/include/access/gist.h
+++ b/src/include/access/gist.h
@@ -237,4 +237,6 @@ typedef struct
do { (e).key = (k); (e).rel = (r); (e).page = (pg); \
(e).offset = (o); (e).leafkey = (l); } while (0)
+extern double gist_seed;
+
#endif /* GIST_H */
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index b68c01a5f2..07319a225e 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -95,6 +95,9 @@ typedef struct GISTSTATE
/* Collations to pass to the support functions */
Oid supportCollation[INDEX_MAX_KEYS];
+
+ /* Random state for gistchoose tie breaker PRNG */
+ unsigned short random_state[3];
} GISTSTATE;
--
2.24.3 (Apple Git-128)
=
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2020-11-02 16:55 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Pavel Borisov @ 2020-11-02 16:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
пн, 2 нояб. 2020 г. в 20:05, Tom Lane <tgl@sss.pgh.pa.us>:
> Alexander Lakhin <exclusion@gmail.com> writes:
> >> Please look at the patch that modifies the gist regression test to make
> >> the issue visible and fixes it by avoiding access to the memory context
> >> that can be reset in gistProcessEmptyingQueue().
>
> > 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?
>
> I think the real issue here is confusion about which level of function
> "owns" the tempCxt and gets to decide when to reset it. We can't have
> both gistBuildCallback and gistProcessEmptyingQueue doing that. Maybe
> we need more than one temporary context, or maybe we could just skip
> the context reset in gistProcessEmptyingQueue and let the storage
> accumulate till we get back to gistBuildCallback. But in any case
> it's going to require more analysis.
>
I investigated the code and realized that there are two ways to call the
problematic gistProcessEmptyingQueue() function:
1. gistEmptyAllBuffers /tempCxt owner/ -> gistProcessEmptyingQueue
A potentially dangerous operation of resetting memory context inside
sub-function calls of a 'context owner' function is done at the very ending
of gistProcessEmptyingQueue(). Just after it we return back
to gistEmptyAllBuffers and immediately have a switch to old context. So
resetting tempCxt has no danger in this case as no pointers are used after.
2. gistBuildCallback /tempCxt owner/ -> gistBufferingBuildInsert ->
gistProcessEmptyingQueue
gistProcessEmptyingQueue() is at the very ending of
gistBufferingBuildInsert(). After the reset no pointers from tempCxt
context are used up to the 'owner' level of gistBuildCallback. So moving
the only itup pointer operation before gistBufferingBuildInsert() call will
fix the bug.
(Of course, the alternative of not re-setting tempCxt
inside gistProcessEmptyingQueue() and doing this level up will also fix
the issue)
So I think that the patch by Alexander will do the thing. I also added some
comments to the code and removed extra context reset in gistBuildCallback
(which is already done level down) to make things clear.
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com <http://www.postgrespro.com;
Attachments:
v2-0001-Fix-for-GiST-buffered-build-bug-related-to-contex.patch (application/octet-stream, 2.0K) download
From 6a12161c17052601ec7e7a398ec691adec7f48fb Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Mon, 2 Nov 2020 20:07:06 +0400
Subject: [PATCH v2] Fix for GiST buffered build bug related to context resets
inside a calls tree which can render itup pointer invalid
---
src/backend/access/gist/gistbuild.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 9d3fa9c3b7..94d65b39d8 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -818,10 +818,24 @@ gistBuildCallback(Relation index,
true);
itup->t_tid = *tid;
+ /* Update tuple count and total size. */
+ buildstate->indtuples += 1;
+ buildstate->indtuplesSize += IndexTupleSize(itup);
+
+ /*
+ * XXX Generally, memory context better be 'owned' by only one function.
+ * If the context is reset somewhere inside its calls, pointers can
+ * become dangling just immediately. Though in the case of
+ * gistBufferingBuildInsert, tempCxt can be reset inside
+ * gistProcessEmptyingQueue because it is the last operation inside the
+ * call and no pointers are used after it, so we can allow this piece of
+ * magic.
+ */
if (buildstate->buildMode == GIST_BUFFERING_ACTIVE)
{
/* We have buffers, so use them. */
gistBufferingBuildInsert(buildstate, itup);
+ /* tempCxt is already reset in gistProcessEmptyingQueue */
}
else
{
@@ -831,14 +845,9 @@ gistBuildCallback(Relation index,
*/
gistdoinsert(index, itup, buildstate->freespace,
buildstate->giststate, buildstate->heaprel, true);
+ MemoryContextReset(buildstate->giststate->tempCxt);
}
-
- /* Update tuple count and total size. */
- buildstate->indtuples += 1;
- buildstate->indtuplesSize += IndexTupleSize(itup);
-
MemoryContextSwitchTo(oldCtx);
- MemoryContextReset(buildstate->giststate->tempCxt);
if (buildstate->buildMode == GIST_BUFFERING_ACTIVE &&
buildstate->indtuples % BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET == 0)
--
2.28.0
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2023-03-29 15:37 Tom Lane <tgl@sss.pgh.pa.us>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-03-29 15:37 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org; kyzevan23@mail.ru
Pavel Borisov <pashkin.elfe@gmail.com> writes:
> So I think that the patch by Alexander will do the thing. I also added some
> comments to the code and removed extra context reset in gistBuildCallback
> (which is already done level down) to make things clear.
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 did use Alexander's original code change that just moved the
totals-incrementing. It looks to me like gistProcessEmptyingQueue
is not guaranteed to reach the context reset, and I'd rather have
an extra reset than miss one. (No-op context resets are cheap.)
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2023-03-29 22:21 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 20+ messages in thread
From: Tom Lane @ 2023-03-29 22:21 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org; kyzevan23@mail.ru
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.patch
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql
index 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);
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2023-03-30 04:42 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 20+ messages in thread
From: Noah Misch @ 2023-03-30 04:42 UTC (permalink / raw)
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
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.)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2023-03-30 05:00 Alexander Lakhin <exclusion@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2023-03-30 05:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; kyzevan23@mail.ru
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
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering
@ 2023-03-30 11:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Tom Lane @ 2023-03-30 11:47 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; pgsql-bugs@lists.postgresql.org; kyzevan23@mail.ru
Alexander Lakhin <exclusion@gmail.com> writes:
> 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.)
OK, there's certainly no urgency.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2023-03-30 11:47 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-29 21:00 BUG #16329: Valgrind detects an invalid read when building a gist index with buffering PG Bug reporting form <noreply@postgresql.org>
2020-03-30 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2020-04-06 06:00 ` Alexander Lakhin <exclusion@gmail.com>
2020-05-02 04:45 ` Alexander Lakhin <exclusion@gmail.com>
2020-05-05 18:59 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-05-11 20:00 ` Alexander Lakhin <exclusion@gmail.com>
2020-05-12 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2020-10-12 12:30 ` Pavel Borisov <pashkin.elfe@gmail.com>
2020-10-12 16:03 ` Pavel Borisov <pashkin.elfe@gmail.com>
2020-10-12 20:03 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-12 22:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-22 05:50 ` Andrey Borodin <x4mmm@yandex-team.ru>
2020-10-22 14:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-24 16:40 ` Andrey Borodin <x4mmm@yandex-team.ru>
2020-11-02 16:55 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-03-29 15:37 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-29 22:21 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 04:42 ` Noah Misch <noah@leadboat.com>
2023-03-30 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-30 11:47 ` Tom Lane <tgl@sss.pgh.pa.us>
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