Re: Snapshot management, final - Mailing list pgsql-patches
From | Tom Lane |
---|---|
Subject | Re: Snapshot management, final |
Date | |
Msg-id | 25109.1210547423@sss.pgh.pa.us Whole thread Raw |
In response to | Re: Snapshot management, final (Alvaro Herrera <alvherre@commandprompt.com>) |
Responses |
Re: Snapshot management, final
Re: Snapshot management, final |
List | pgsql-patches |
Alvaro Herrera <alvherre@commandprompt.com> writes: > Tom Lane wrote: >> Also, I think that the whole snapshot-sharing mechanism is not working >> as intended except for the serializable case; otherwise sequences >> like >> x = RegisterSnapshot(GetTransactionSnapshot()); >> y = RegisterSnapshot(GetTransactionSnapshot()); >> will result in x and y being separate copies. Or are you assuming >> that this just isn't worth optimizing? > It's not that I don't think it's worth optimizing, but I think it's a > bit away from the scope of this patch. The problem here is how to > notice that two consecutive GetTransactionSnapshot calls should really > return different snapshots, considering that shared state may change in > between. Perhaps there's an easy way to optimize that; I don't know. Yeah, in general you could only optimize it if no other backend had changed state, and there doesn't seem any real simple way to know that. Maybe we could teach GetSnapshotData to test for it but it's a bit doubtful that it's worth the cycles. I'm fine with leaving this as-is. I have a few other gripes though: The UnregisterSnapshot call at line 631 of indexcmds.c is definitely too early: the snapshot is touched in the very next statement. I'd be inclined to move it down to right after the Pop at line 696; there's no point in unregistering till you pop anyway. In FreeQueryDesc, the unregister calls should be after the Assert. Drop this comment fragment in plancache.c, it's not relevant anymore: * Having to ! * replan is an unusual case, and it seems a really bad idea for ! * RevalidateCachedPlan to affect the snapshot only in unusual ! * cases. ! */ s_level and as_level fields must be int not uint32, because they are being compared to nesting levels that are declared as int. You risk getting the wrong comparison semantics. (Not that it should ever matter in this code, but mixing signed and unsigned arithmetic is just bad form.) Why Assert(snap->satisfies == HeapTupleSatisfiesMVCC) in PushActiveSnapshot and RegisterSnapshot? AFAICT this code will work fine on non-MVCC snapshots. Seems a bit odd that RegisterSnapshot and PushActiveSnapshot do their palloc's in opposite orders. I think making the list elt first is probably better; in either case, if the second palloc fails then you're gonna leak the first one, so it's better to make the smaller allocation first. Shouldn't UnregisterSnapshot insist that s_level be equal to current xact nest level? AtSubCommit_Snapshot can leave us with multiple RegdSnapshotElt's for the same snap and s_level, which seems a bit bogus. I don't think the unregister code will go wrong in its current form, but you at least need some comments about the invariants that are expected to hold for the list data structure. Example: the code depends on the assumption that elements are in nonincreasing s_level order, but that's nowhere stated. AtSubAbort_Snapshot has Assert(tofree->s_snap->active_count == 0) which seems wrong: couldn't the snap be active in an outer subxact? regards, tom lane
pgsql-patches by date: