From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, daniel(dot)westermann(at)dbi-services(dot)com |
Subject: | Re: zheap: a new storage format for PostgreSQL |
Date: | 2018-11-03 04:00:47 |
Message-ID: | CAA4eK1J=o4O0raiQQCpGJu5+nzRgCGW4CB5AHL=Q6JJZtq7yew@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Nov 2, 2018 at 6:41 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> 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.
>
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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2018-11-03 04:41:28 | Re: WIP: Avoid creation of the free space map for small tables |
Previous Message | Tom Lane | 2018-11-03 03:56:58 | Re: backend crash on DELETE, reproducible locally |