Re: Use of ActiveSnapshot

On 5/14/2007 1:29 PM, Tom Lane wrote:
> 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.

The only problem with that is that there are code paths that set 
ActiveSnapshot to palloc()'d memory that is released due to a 
MemoryContextDelete() without resetting ActiveSnapshot to NULL. So it 
might be possible for RevalidateCachedPlan to go ahead with an 
ActiveSnapshot pointing to garbage.

I think it would be cleaner if RevalidateCachedPlan()'s API would have a 
Snapshot argument. If it needs a snapshot and the argument is NULL, it 
can create (and free) one itself, otherwise it'd use the one given.


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #