Does slot_deform_tuple need to care about dropped columns?

[Andres Freund]

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