Re: zheap: a new storage format for PostgreSQL

On 11/02/2018 12:12 PM, Amit Kapila wrote:
> On Thu, Nov 1, 2018 at 7:26 PM Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>>
>> On 11/01/2018 07:43 AM, Amit Kapila wrote:
>>>
>>> You can find the latest code at https://github.com/EnterpriseDB/zheap
>>>
>>
>> Seems valgrind complains about a couple of places in the code - nothing
>> major, might be noise, but probably worth a look.
>>
>
> I have looked at the report and one of those seems to be problematic,
> so I have pushed the fix for the same. The other one for below stack
> seems to be bogus:
> ==7569== Uninitialised value was created by a stack allocation
> ==7569== at 0x59043D: znocachegetattr (zheapam.c:6206)
> ==7569==
> {
> <insert_a_suppression_name_here>
> Memcheck:Cond
> fun:ZHeapDetermineModifiedColumns
> fun:zheap_update
>
> I have checked in the function znocachegetattr that if we initialize
> the value of ret_datum, it fixes the reported error, but actually
> there is no need for doing it as the code always assign the valid
> value to this variable. I have left it as is for now as I am not sure
> whether there is any value in doing such an initialization.
>

Well, the problem is the ret_datum is modified like this:

thisatt = TupleDescAttr(tupleDesc, attnum);
if (thisatt->attbyval)
memcpy(&ret_datum, tp + off, thisatt->attlen);
else
ret_datum = PointerGetDatum((char *) (tp + off));

which means that for cases with attlen < sizeof(Datum), this ends up
leaving part of the value undefined. So it's a valid issue. I'm sure
it's not the only place where we do something like this, and the other
places don't trigger the valgrind warning, so how do those places do
this? heapam seems to call fetch_att in the end, which essentially calls
Int32GetDatum/Int16GetDatum/CharGetDatum, so why not to use the same
trick here?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services