Re: Use of ActiveSnapshot

Jan Wieck <JanWieck@Yahoo.com> writes:
> The comment for the call of pg_plan_queries in util/cache/plancache.c 
> line 469 for example is fatally wrong. Not only should the snapshot be 
> set by all callers at this point, but if the call actually does replan 
> the queries, the existing ActiveSnapshot is replaced with one allocated 
> on the current memory context. If this happens to be inside of a nested 
> SPI call sequence, the innermost SPI stack frame will free the snapshot 
> data without restoring ActiveSnapshot to the one from the caller.

Yeah, I'd been meaning to go back and recheck that point after the code
settled down, but forgot :-(.

It is possible for RevalidateCachedPlan to be called with no snapshot
yet set --- at least the protocol Describe messages can do that.  I
don't want Describe to force a snapshot because that would be bad for
cases like LOCK TABLE at the start of a serializable transaction, so
RevalidateCachedPlan had better be able to cope with this case.

Since the "typical" case in which no replan is necessary won't touch
the snapshot, I think we'd better adopt the rule that
RevalidateCachedPlan never causes any caller-visible change in
ActiveSnapshot, else we'll be risking very-hard-to-reproduce bugs.
So my proposal is that RevalidateCachedPlan should set a snapshot for
itself if it needs to replan and ActiveSnapshot is NULL (else it might
as well just use the existing snap); and that it should save and restore
ActiveSnapshot when it does this.
        regards, tom lane