[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 passes make 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 function writes "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
Attachments:
virtslot_tiny_fix.patchapplication/octet-stream; name=virtslot_tiny_fix.patchDownload
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 5900d96..87bc510 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -244,7 +244,7 @@ tts_virtual_materialize(TupleTableSlot *slot)
static void
tts_virtual_copyslot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
- TupleDesc srcdesc = dstslot->tts_tupleDescriptor;
+ TupleDesc srcdesc = srcslot->tts_tupleDescriptor;
Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);
"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 fix passes 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 function writes "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 fix passes 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