On Tue, Aug 24, 2021 at 12:20 AM Greg Nancarrow <gregn4422@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
