Date: Tue, 9 Jul 2019 13:43:41 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15899: Valgrind detects errors on create gist index
Message-ID: <20190709044341.GA23827@paquier.xyz>
References: <15899-0d24fb273b3dd90c@postgresql.org>
 <38f18230-e766-3a87-e3ad-fef6562c0c23@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="+QahgC5+KEYLbs62"
Content-Disposition: inline
In-Reply-To: <38f18230-e766-3a87-e3ad-fef6562c0c23@gmail.com>
Precedence: bulk


--+QahgC5+KEYLbs62
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 (buffer=
ing
>> =3D on, fillfactor=3D50);
>
> 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.
=3D=3D14943=3D=3D Conditional jump or move depends on uninitialised value(s)
=3D=3D14943=3D=3D    at 0x1FD7A5: gistchoose (gistutil.c:535)
=3D=3D14943=3D=3D    by 0x20B5BA: gistProcessItup (gistbuild.c:597)
=3D=3D14943=3D=3D    by 0x20C367: gistProcessEmptyingQueue (gistbuild.c:973)
=3D=3D14943=3D=3D    by 0x20C476: gistEmptyAllBuffers (gistbuild.c:1036)
=3D=3D14943=3D=3D    by 0x20ACD9: gistbuild (gistbuild.c:207)
=3D=3D14943=3D=3D    by 0x2DCDF9: index_build (index.c:2726)
=3D=3D14943=3D=3D    by 0x2DA4CA: index_create (index.c:1177)
=3D=3D14943=3D=3D    by 0x3C52BB: DefineIndex (indexcmds.c:1005)
=3D=3D14943=3D=3D    by 0x653A27: ProcessUtilitySlow (utility.c:1373)
=3D=3D14943=3D=3D    by 0x652AE0: standard_ProcessUtility (utility.c:927)
=3D=3D14943=3D=3D    by 0x651B78: ProcessUtility (utility.c:360)
=3D=3D14943=3D=3D    by 0x650AD7: PortalRunUtility (pquery.c:1175)
=3D=3D14943=3D=3D  Uninitialised value was created by a stack allocation
=3D=3D14943=3D=3D    at 0x4C6F3B0: ??? (in
/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
=3D=3D14943=3D=3D
=3D=3D14943=3D=3D Conditional jump or move depends on uninitialised value(s)
=3D=3D14943=3D=3D    at 0x1FD770: gistchoose (gistutil.c:513)
=3D=3D14943=3D=3D    by 0x20B5BA: gistProcessItup (gistbuild.c:597)
=3D=3D14943=3D=3D    by 0x20C367: gistProcessEmptyingQueue (gistbuild.c:973)
=3D=3D14943=3D=3D    by 0x20C476: gistEmptyAllBuffers (gistbuild.c:1036)
=3D=3D14943=3D=3D    by 0x20ACD9: gistbuild (gistbuild.c:207)
=3D=3D14943=3D=3D    by 0x2DCDF9: index_build (index.c:2726)
=3D=3D14943=3D=3D    by 0x2DA4CA: index_create (index.c:1177)
=3D=3D14943=3D=3D    by 0x3C52BB: DefineIndex (indexcmds.c:1005)
=3D=3D14943=3D=3D    by 0x653A27: ProcessUtilitySlow (utility.c:1373)
=3D=3D14943=3D=3D    by 0x652AE0: standard_ProcessUtility (utility.c:927)
=3D=3D14943=3D=3D    by 0x651B78: ProcessUtility (utility.c:360)
=3D=3D14943=3D=3D    by 0x650AD7: PortalRunUtility (pquery.c:1175)
=3D=3D14943=3D=3D  Uninitialised value was created by a stack allocation
=3D=3D14943=3D=3D    at 0x4C6F3B0: ??? (in /usr/lib/x86_64-linux-gnu/libcry=
pto.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
=66rom my side.

Regarding the zero-allocation, we have gistLoadNodeBuffer() loading=20
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

--+QahgC5+KEYLbs62
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl0kG30ACgkQnvQgOdby
QH3HDA//Q/YzbHyq9lhNiqNEdYC9TWxz5Z5Pi+lUhtPVU4Ur3Prqw2mp8eYHZtCr
eR9UTgbpLlX0lriJLl3bGejBSJD5rFTp0b2eYAIavbDESp52Elq/EkcmCmpIi9YT
qi+IkIc5FVncplB38md8o+kiWdXaQkfyLx3xiAxybQIIJLaCfcIdD6dtsx/KE19T
+LhpFofiUZpbcKIk8CQUfId5hvPt0JUUsH9YMgfgOT62FbDn7xl8NiX6qrkZqYES
TKHAXyvzVht/Ae1LzXoO/57WgVjTMWvRll2JoewDkrFnJcTwo5TOBOV0eOl6NGWn
vpsc8Vg/cdHmHZxNSaV8JBSg8ygsYd72qmWt1UVnqahhyCKHJxRHGE4h8EWJ/k3E
7i2z7I/riSk8CRtKYoXbXR5q4r+8j6oDublZg04/4oY3yTzpnaOUnMw8YN9Vj3qM
7ELPsEU7whmWQ/O5Yx0FvF0FABu9rwNZboHIQY2ijB04p0Uvn33DdC64WBK2wPK0
ggGszE2ZEO7KdqN4lva6t/Cc8nZUGSNv+gD4OPYGJbusp7vRvmSEn7aYW8zro7Jn
kus7z6lITmS62gR9CurPbdkD3MC+1X+PVrgCvMNzXvOsE347qCZMewNMv7N08gh7
kifaaBU8iwnOEksMGtTwBJ6s1yVEVb6gbkHVRW+4q3xwsgdaJqA=
=FIY0
-----END PGP SIGNATURE-----

--+QahgC5+KEYLbs62--