Does slot_deform_tuple need to care about dropped columns?
Hi,
Currently functions like slot_getattr() first check if the attribute is
already deformed:
Datum
slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull)
{
...
/*
* fast path if desired attribute already cached
*/
if (attnum <= slot->tts_nvalid)
{
*isnull = slot->tts_isnull[attnum - 1];
return slot->tts_values[attnum - 1];
}
...
but later, in the case the attribute isn't already deformed, the
following hunk exists:
/*
* If the attribute's column has been dropped, we force a NULL result.
* This case should not happen in normal use, but it could happen if we
* are executing a plan cached before the column was dropped.
*/
if (TupleDescAttr(tupleDesc, attnum - 1)->attisdropped)
{
*isnull = true;
return (Datum) 0;
}
Which strikes me as quite odd. If somebody previously accessed a *later*
column (be it via slot_getattr, or slot_getsomeattrs), the whole
attisdropped check is neutralized.
I think we either should remove that check as unnecessary, or move it to
slot_deform_tuple(), so it also protects other accesses (like the very
very common direct access to tts_values/isnull).
Tom, you added that code way back when, in a9b05bdc8330. And as far as I
can tell that issue existed back then too.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
... in the case the attribute isn't already deformed, the
following hunk exists:
/*
* If the attribute's column has been dropped, we force a NULL result.
* This case should not happen in normal use, but it could happen if we
* are executing a plan cached before the column was dropped.
*/
if (TupleDescAttr(tupleDesc, attnum - 1)->attisdropped)
{
*isnull = true;
return (Datum) 0;
}
Which strikes me as quite odd. If somebody previously accessed a *later*
column (be it via slot_getattr, or slot_getsomeattrs), the whole
attisdropped check is neutralized.
Good point. Let's remove it and see what happens.
Tom, you added that code way back when, in a9b05bdc8330. And as far as I
can tell that issue existed back then too.
I was just transposing code that had existed before that in ExecEvalVar.
Evidently I didn't think hard about whether the protection was
bulletproof. But since it isn't, maybe we don't need it at all.
I think our checks for obsoleted plans are a lot more bulletproof
than they were back then, so it's entirely likely the issue is moot.
regards, tom lane
Hi,
On 2018-11-07 12:58:16 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
... in the case the attribute isn't already deformed, the
following hunk exists:/*
* If the attribute's column has been dropped, we force a NULL result.
* This case should not happen in normal use, but it could happen if we
* are executing a plan cached before the column was dropped.
*/
if (TupleDescAttr(tupleDesc, attnum - 1)->attisdropped)
{
*isnull = true;
return (Datum) 0;
}Which strikes me as quite odd. If somebody previously accessed a *later*
column (be it via slot_getattr, or slot_getsomeattrs), the whole
attisdropped check is neutralized.Good point. Let's remove it and see what happens.
Done that just now.
Tom, you added that code way back when, in a9b05bdc8330. And as far as I
can tell that issue existed back then too.I was just transposing code that had existed before that in ExecEvalVar.
Evidently I didn't think hard about whether the protection was
bulletproof. But since it isn't, maybe we don't need it at all.
I think our checks for obsoleted plans are a lot more bulletproof
than they were back then, so it's entirely likely the issue is moot.
Yea, I think it ought to be moot these days. If not we better make it
so.
Greetings,
Andres Freund