Re: WIP: Faster Expression Processing v4

On 2017-03-23 21:26:19 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2017-03-23 20:36:32 -0400, Tom Lane wrote:
> >> 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?
>
> Yeah. The other order would be safe, because ExecEvalScalarVar would do
> slot_getattr which would re-extract the value from the newly materialized
> tuple. But there definitely seems to be a hazard for the order you
> mentioned.
>
> > Do we need to do anything about this in the back-branches,
> > given how unlikely this is going to be in practice?
>
> Probably not. As I mentioned, I think this may be only theoretical rather
> than real, if you believe that buffer pins would only be associated with
> slots holding references to regular tuples. And even if it's not
> theoretical, the odds of seeing a failure in the field seem pretty tiny
> given that a just-released buffer shouldn't be subject to recycling for
> a fair while. But I don't want to leave it like this going forward.

Ok.

> >> 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.
>
> Hmm, I see ... but that only works in the cases where the caller of
> ExecBuildProjectionInfo supplied a source slot, and a lot of 'em
> don't.

Right, the old and new code comment on that:

* inputDesc can be NULL, but if it is not, we check to see whether simple
* Vars in the tlist match the descriptor. It is important to provide
* inputDesc for relation-scan plan nodes, as a cross check that the relation
* hasn't been changed since the plan was made. At higher levels of a plan,
* there is no need to recheck.

and that seems like reasonable to me? That said, I think we can remove
that assumption, by checking once.

> As the code stands, we are unable to use ASSIGN_FOO_VAR in quite a lot
> of places, including everywhere above the relation scan level.

Hm? If inputDesc isn't given we just, before and after, do:
if (!inputDesc)
isSimpleVar = true; /* can't check type, assume OK */

> I'd already put in the infrastructure to add ASSIGN_FOO_VAR_FIRST
> step types. I could take it back out, but I wonder if it wouldn't be
> smarter to keep it and remove the restriction in ExecBuildProjectionInfo.
> Or maybe we could have ExecBuildProjectionInfo emit either
> ASSIGN_FOO_VAR_FIRST or ASSIGN_FOO_VAR depending on whether it can prove
> the reference safe.

I think it's probably ok to just leave the check in, and remove those
comments, and simplify the isSimpleVar stuff to only check if
IsA(tle->expr, Var) && ((Var *) tle->expr)->varattno > 0)

- Andres