Re: BUG #15899: Valgrind detects errors on create gist index

On Sun, Jul 07, 2019 at 10:21:26PM +0300, Alexander Lakhin wrote:
>> When executing under valgrind the following script:
>> create table gist_point_tbl(id int4, p point);
>> insert into gist_point_tbl (id, p)
>> select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g;
>> create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
>> = on, fillfactor=50);
>
> The attached patch eliminates these errors (but maybe
> MemoryContextAllocZero is too superficial).

Thanks for the patch and the report.

I can see that most of the complaints from valgrind go away, and I am
still bumping into these two ones.
==14943== Conditional jump or move depends on uninitialised value(s)
==14943==    at 0x1FD7A5: gistchoose (gistutil.c:535)
==14943==    by 0x20B5BA: gistProcessItup (gistbuild.c:597)
==14943==    by 0x20C367: gistProcessEmptyingQueue (gistbuild.c:973)
==14943==    by 0x20C476: gistEmptyAllBuffers (gistbuild.c:1036)
==14943==    by 0x20ACD9: gistbuild (gistbuild.c:207)
==14943==    by 0x2DCDF9: index_build (index.c:2726)
==14943==    by 0x2DA4CA: index_create (index.c:1177)
==14943==    by 0x3C52BB: DefineIndex (indexcmds.c:1005)
==14943==    by 0x653A27: ProcessUtilitySlow (utility.c:1373)
==14943==    by 0x652AE0: standard_ProcessUtility (utility.c:927)
==14943==    by 0x651B78: ProcessUtility (utility.c:360)
==14943==    by 0x650AD7: PortalRunUtility (pquery.c:1175)
==14943==  Uninitialised value was created by a stack allocation
==14943==    at 0x4C6F3B0: ??? (in
/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==14943==
==14943== Conditional jump or move depends on uninitialised value(s)
==14943==    at 0x1FD770: gistchoose (gistutil.c:513)
==14943==    by 0x20B5BA: gistProcessItup (gistbuild.c:597)
==14943==    by 0x20C367: gistProcessEmptyingQueue (gistbuild.c:973)
==14943==    by 0x20C476: gistEmptyAllBuffers (gistbuild.c:1036)
==14943==    by 0x20ACD9: gistbuild (gistbuild.c:207)
==14943==    by 0x2DCDF9: index_build (index.c:2726)
==14943==    by 0x2DA4CA: index_create (index.c:1177)
==14943==    by 0x3C52BB: DefineIndex (indexcmds.c:1005)
==14943==    by 0x653A27: ProcessUtilitySlow (utility.c:1373)
==14943==    by 0x652AE0: standard_ProcessUtility (utility.c:927)
==14943==    by 0x651B78: ProcessUtility (utility.c:360)
==14943==    by 0x650AD7: PortalRunUtility (pquery.c:1175)
==14943==  Uninitialised value was created by a stack allocation
==14943==    at 0x4C6F3B0: ??? (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)

Still they don't make much sense, because these point to complaints
that keep_current_best may have an uninitialized value, and this
variable is initialized before scanning the tuples on the page.  I
would not have expected that an uninitialized thing from libcrypto
stack would have an effect here, and we have the project policy to
not silence stuff fro outside the tree as far as I know.

Regarding the patch, I have been reviewing it and the two changes are
fine by me.  I would have just reordered the fields in
gistGetNodeBuffer to match the structure, but that's mainly nanny-ism
from my side.

Regarding the zero-allocation, we have gistLoadNodeBuffer() loading 
directly the block's contents from a temporary file after allocating a
page, but gistPushItupToNodeBuffer() justifies that as a cheap
insurance.  So that's good for me.  Do others have objections or extra
thoughts?  This comes from 1d27dcf, so this should go all the way down.
--
Michael