On 2017-03-23 21:26:19 -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers