Обсуждение: [bug fix??] Fishy code in tts_cirtual_copyslot()

Hello,

In the following code in execTuples.c, shouldn' srcdesc point to the source slot's tuple descriptor?  The attached fix
passesmake check.  What kind of failure could this cause?
 

BTW, I thought that in PostgreSQL coding convention, local variables should be defined at the top of blocks, but this
functionwrites "for (int natts;".  I didn't modify it because many other source files also write in that way.
 


--------------------------------------------------
static void
tts_virtual_copyslot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
    TupleDesc   srcdesc = dstslot->tts_tupleDescriptor;

    Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);

    tts_virtual_clear(dstslot);

    slot_getallattrs(srcslot);

    for (int natt = 0; natt < srcdesc->natts; natt++)
    {
        dstslot->tts_values[natt] = srcslot->tts_values[natt];
        dstslot->tts_isnull[natt] = srcslot->tts_isnull[natt];
    }
--------------------------------------------------


Regards
Takayuki Tsunakawa

"Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes:
> In the following code in execTuples.c, shouldn' srcdesc point to the source slot's tuple descriptor?  The attached
fixpasses make check.  What kind of failure could this cause? 

Yeah, sure looks like a typo to me too.

I temporarily changed the Assert to be "==" rather than "<=", and
it still passed check-world, so evidently we are not testing any
cases where the descriptors are of different lengths.  This explains
the lack of symptoms.  It's still a bug though, so pushed.

> BTW, I thought that in PostgreSQL coding convention, local variables should be defined at the top of blocks, but this
functionwrites "for (int natts;". 

Yeah, we've agreed to join the 21st century to the extent of allowing
local for-loop variables.

Thanks for the report!

            regards, tom lane

From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> I temporarily changed the Assert to be "==" rather than "<=", and
> it still passed check-world, so evidently we are not testing any
> cases where the descriptors are of different lengths.  This explains
> the lack of symptoms.  It's still a bug though, so pushed.

Thank you for committing.

> > BTW, I thought that in PostgreSQL coding convention, local variables
> should be defined at the top of blocks, but this function writes "for (int
> natts;".
> 
> Yeah, we've agreed to join the 21st century to the extent of allowing
> local for-loop variables.

That's good news.  It'll help a bit to code comfortably.


Regards
Takayuki Tsunakawa

Hi,

On 2019-09-22 14:24:36 -0400, Tom Lane wrote:
> "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes:
> > In the following code in execTuples.c, shouldn' srcdesc point to the source slot's tuple descriptor?  The attached
fixpasses make check.  What kind of failure could this cause?
 
> 
> Yeah, sure looks like a typo to me too.

Indeed, thanks for catching and pushing.


> I temporarily changed the Assert to be "==" rather than "<=", and
> it still passed check-world, so evidently we are not testing any
> cases where the descriptors are of different lengths.  This explains
> the lack of symptoms.

I have a hard time seeing cases where it'd be a good idea to copy slots
of a smaller natts into a slot with larger natts. So i'm not too
surprised.


> It's still a bug though, so pushed.

Indeed.

Greetings,

Andres Freund