Re: btree_gist macaddr valgrind woes

On 05/16/2014 06:43 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> On 05/16/2014 01:28 PM, Andres Freund wrote:
>>> Presumably it's because the type is a fixed width type with a length of
>>> 6. I guess it's allocating stuff sizeof(macaddr) but then passes the
>>> result around as a Datum which doesn't work well.
>
> I've not tried valgrinding to check, but for macaddr I suspect the blame
> can be laid at the feet of gbt_num_compress, which is creating a datum of
> actual size 2 * tinfo->size (ie, 12 bytes for macaddr). This is later
> stored into an index column of declared type gbtreekey16, so the tuple
> assembly code is expecting the datum to have size 16.

Ah, I see.

> AFAICS there is no direct connection between the sizes the C code knows
> about and the sizes of the datatypes declared as STORAGE options in
> btree_gist--1.0.sql. Probably a reasonable fix would be to add a field
> to gbtree_ninfo that specifies the index datum size, and then make
> gbt_num_compress palloc0 that size rather than 2 * tinfo->size.

ISTM the "correct" fix would be to define a gbtreekey12 data type and
use that. That's not back-patchable though, and introducing a whole new
type to save a few bytes is hardly worth it. What you did makes sense.

>> The complaints seem to be coming from all the varlen data types, too,
>> not just macaddr:
>
> Dunno what's the problem for the varlena types, but that's a completely
> separate code path.

It seems to be a similar issue to what I fixed in the bit type earlier.
When the lower+upper keys are stored as one varlen Datum, the padding
bytes between them are not zeroed. With these datatypes (i.e. not
varbit) it doesn't lead to any real errors, however, because the padding
bytes are never read. Valgrind is just complaining that we're storing
uninitialized data on disk, even though it's never looked at.

Nevertheless, seems best to fix that, if only to silence Valgrind.

(And you just beat me to it. Thanks!)

- Heikki