Re: zheap: a new storage format for PostgreSQL

On Fri, Nov 2, 2018 at 6:41 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On 11/02/2018 12:12 PM, Amit Kapila wrote:
> > On Thu, Nov 1, 2018 at 7:26 PM Tomas Vondra
> > <tomas.vondra@2ndquadrant.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.
>

Agreed.

> 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?
>

This is because, in zheap, we have omitted all alignment padding for
pass-by-value types.  See the description in my previous email [1].  I
think here we need to initialize ret_datum at the beginning of the
function unless you have some better idea.

One thing unrelated to the above problem is that I have forgotten to
mention in my previous email that Daniel Westermann whom I have cc'ed
in this email has reported few bugs in this branch which seems to have
fixed.  He seems to be interested in doing more tests.  Daniel, I
encourage you to share your findings here.

Thanks, Tomas and Daniel for looking into the branch and reporting
problems, it is really helpful.

[1] - https://www.postgresql.org/message-id/CAA4eK1Lwb%2BrGeB_z%2BjUbnSndvgnsDUK%2B9tjfng4sy1AZyrHqRg%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com