On Tue, Aug 24, 2021 at 12:20 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > I initially thought this too, but RestoreTransactionSnapshot() sets up > the resultant transaction snapshot in "CurrentSnapshot", which is > static to snapmgr.c (like the other pointers to valid snapshots) and I > didn't really want to mess with the visibility of that, to allow a > call to PushActiveSnapshot(CurrentSnapshot) in parallel.c. Also, I > wasn't sure if it was safe to call GetTransactionSnapshot() here > without the risk of unwanted side-effects - but, looking at it again, > I think it is probably OK, so I did use it in my revised patch > (attached) and removed > that new function RestoreTxnSnapshotAndSetAsActive(). Do you agree > that it is OK to call GetTransactionSnapshot() here?
I guess I was thinking more of rejiggering things so that we save the results of each RestoreSnapshot() call in a local variable, e.g. asnapshot and tsnapshot. And then I think we could just RestoreTransactionSnapshot() on whichever one we want, and then PushActiveSnapshot(asnapshot) either way. I think it would be worth trying to move the PushActiveSnapshot() call out of the if statement instead it in two places, written differently but doing the same thing. > I agree, that is a better comment and detailed description, but didn't > you mean "If the transaction isolation level is REPEATABLE READ or > SERIALIZABLE ..."? I sure did! -- Robert Haas EDB: http://www.enterprisedb.com