Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist
 index with buffering
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
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>
From: Alexander Lakhin <exclusion@gmail.com>
Message-ID: <93ca4540-0016-8217-2b31-535613499987@gmail.com>
Date: Mon, 11 May 2020 23:00:01 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <10261.1588705157@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------B7119CB6BD6C689D22862A98"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------B7119CB6BD6C689D22862A98
Content-Type: multipart/alternative;
 boundary="------------133C64CC8A2755D7B9134829"


--------------133C64CC8A2755D7B9134829
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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

--------------133C64CC8A2755D7B9134829
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">Hello Tom,<br>
      05.05.2020 21:59, Tom Lane wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:10261.1588705157@sss.pgh.pa.us">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">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?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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?</pre>
    </blockquote>
    Thanks for your review!<br>
    Yes, I'm not excited by this fix too, so I'll try to find another
    not so quick way to fix it.
    <blockquote type="cite" cite="mid:10261.1588705157@sss.pgh.pa.us">
      <pre class="moz-quote-pre" wrap="">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?)</pre>
    </blockquote>
    Please look at the improved test that makes the code coverage for <a>gistbuildbuffers.c</a>
    almost 100%.<br>
    <br>
    Best regards,<br>
    Alexander<br>
  </body>
</html>

--------------133C64CC8A2755D7B9134829--

--------------B7119CB6BD6C689D22862A98
Content-Type: text/x-patch; charset=UTF-8;
 name="gist-buffering-test.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="gist-buffering-test.patch"

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;
 

--------------B7119CB6BD6C689D22862A98--