Horton

Thread

Commits

  1. Remove ineffective check against dropped columns from slot_getattr().

  1. Does slot_deform_tuple need to care about dropped columns?

    Andres Freund <andres@anarazel.de> — 2018-11-07T17:44:03Z

    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

  2. Re: Does slot_deform_tuple need to care about dropped columns?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-07T17:58:16Z

    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

  3. Re: Does slot_deform_tuple need to care about dropped columns?

    Andres Freund <andres@anarazel.de> — 2018-11-10T01:41:46Z

    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