Re: [HACKERS] WIP: Faster Expression Processing v4 - Mailing list pgsql-hackers

On 2017-03-23 20:36:32 -0400, Tom Lane wrote:
> I found a rather nasty bug :-( ... the comment in EEOP_INNER_VAR_FIRST about
> 
> +            /*
> +             * Can't assert tts_nvalid, as wholerow var evaluation or such
> +             * could have materialized the slot - but the contents are still
> +             * valid :/
> +             */
> +            Assert(op->d.var.attnum >= 0);
> 
> is actually averting its eyes from a potentially serious problem.  If we
> go through ExecEvalWholeRowVar, which calls ExecFetchSlotTupleDatum, and
> ExecFetchSlotTuple decides it has to materialize the tuple, then
> ExecMaterializeSlot does this:
> 
>     /*
>      * Drop the pin on the referenced buffer, if there is one.
>      */
>     if (BufferIsValid(slot->tts_buffer))
>         ReleaseBuffer(slot->tts_buffer);
> 
>     slot->tts_buffer = InvalidBuffer;
> 
>     /*
>      * Mark extracted state invalid.  This is important because the slot is
>      * not supposed to depend any more on the previous external data; we
>      * mustn't leave any dangling pass-by-reference datums in tts_values.
>      * However, we have not actually invalidated any such datums, if there
>      * happen to be any previously fetched from the slot.  (Note in particular
>      * that we have not pfree'd tts_mintuple, if there is one.)
>      */
>     slot->tts_nvalid = 0;
> 
> 
> The problem here is that once we drop the buffer pin, any pointers we may
> have into on-disk data are dangling pointers --- we're at risk of some
> other backend taking away that shared buffer.  (So I'm afraid that the
> commentary there is overly optimistic.)  So even though those pointers
> may still be there beyond tts_nvalid, subsequent references to them are
> very dangerous.

This applies to the code in master as well, no?  An ExecEvalScalarVar()
followed by an ExecEvalWholeRowVar() would have precisely the same
effect?  Do we need to do anything about this in the back-branches,
given how unlikely this is going to be in practice?


> So I think that we have got to fix ExecEvalWholeRowVar so that it doesn't
> clobber the state of the slot.

That seems like a good plan.


> Also, while trying to test the above scenario, I realized that the patch
> as submitted was being way too cavalier about where it was applying 
> CheckVarSlotCompatibility and so on.  The ASSIGN_FOO_VAR steps, for
> instance, had no protection at all.

I don't think that's true - the assign checks had copied the code from
the old ExecBuildProjectionInfo, setting isSimpleVar iff
(!attr->attisdropped && variable->vartype == attr->atttypid) - we can
check that for projections in contrast to normal expressions because we
already know the slot.  The relevant comment for that, from before the
patch, is: * inputDesc.  (Note: if there is a type mismatch then ExecEvalScalarVar * will probably throw an error at
runtime,but we leave that to it.) */
 


> I hope to have a fully reviewed patch to pass back to you tomorrow.
> Or Saturday at the latest.

Cool.

Greetings,

Andres Freund