On 05/06/2014 10:03 PM, Andres Freund wrote:
Hi,

I am not sure whether these are new for 9.4 but they're worth looking at
anyway:
Valgrind was run with:
  --trace-children=yes --quiet \
  --track-origins=yes --leak-check=no \
  --read-var-info=yes \
  --suppressions=/home/andres/src/postgresql/src/tools/valgrind.supp \
  <postgres with options>
All seem to be due btree_bit's fault and they seem to have a common
origin. Didn't look at code.

==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x4C2C812: bcmp (mc_replace_strmem.c:935)
==27401==    by 0x8A8533: byteacmp (varlena.c:2735)
==27401==    by 0x8D70A2: DirectFunctionCall2Coll (fmgr.c:1050)
==27401==    by 0x180822C6: gbt_bitcmp (btree_bit.c:68)
==27401==    by 0x18079C05: gbt_vsrt_cmp (btree_utils_var.c:430)
==27401==    by 0x919F66: qsort_arg (qsort_arg.c:121)
==27401==    by 0x91A531: qsort_arg (qsort_arg.c:186)
==27401==    by 0x91A531: qsort_arg (qsort_arg.c:186)
==27401==    by 0x18079E68: gbt_var_picksplit (btree_utils_var.c:484)
==27401==    by 0x180825AC: gbt_bit_picksplit (btree_bit.c:180)
==27401==    by 0x8D7AD3: FunctionCall2Coll (fmgr.c:1324)
==27401==    by 0x497701: gistUserPicksplit (gistsplit.c:433)

In a btree_gist bit/varbit index, an internal index item consists of a lower and upper key. The full Varbit struct is not stored as the lower/upper key, but only the "bits" array. The lower key is expanded to next INTALIGNed size, so that when the lower and upper keys are put together into one Datum, the length-field of the upper key is properly aligned.

The bug is in gbt_bit_xfrm(), which expands a VarBit struct to an INTALIGNed bytea, containing just the bits array. It doesn't initialize the padding bytes. That can confuse comparisons against lower/upper keys, as the comparison routine doesn't know which bytes are padding, and will merrily compare them as well. I was able to get wrong query results after modifying gbt_bit_xfrm() to knowingly initialize the padding bytes to random():

create table varbittest (b varbit);
insert into varbittest select '001'::varbit from generate_series(1, 100) union all select '01'::varbit from generate_series(1, 100) union all select '1'::varbit from generate_series(1, 100);
create index varbittest_idx on varbittest using gist(b);
set enable_seqscan=off;

-- should return 200
postgres=# select count(*) from varbittest where b >= '01';
 count
-------
   169
(1 row)


I committed a fix to gbt_bit_xfrm() to zero the padding bytes. However, if you have an existing btree_gist index on bit/varbit, you will have to reindex as there can be non-zero padding bytes on disk already. That should be mentioned in the release notes.

Thanks for the Valgrinding!

- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to