On Tue, Aug 24, 2021 at 5:00 AM Robert Haas <robertmh...@gmail.com> wrote: > > > I think this looks pretty good. I am not sure I see any reason to > introduce a new function RestoreTxnSnapshotAndSetAsActive. Couldn't we > just use RestoreTransactionSnapshot() and then call > PushActiveSnapshot() from parallel.c? That seems preferable to me from > the standpoint of avoiding multiplication of APIs. >
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 also think that the comments should explain why we are doing this, > rather than just that we are doing this. So instead of this: > > + /* > + * If the transaction snapshot was serialized, restore it and restore the > + * active snapshot too. Otherwise, the active snapshot is also installed > as > + * the transaction snapshot. > + */ > > ...perhaps something like: > > If the transaction isolation level is READ COMMITTED or SERIALIZABLE, > the leader has serialized the transaction snapshot and we must restore > it. At lower isolation levels, there is no transaction-lifetime > snapshot, but we need TransactionXmin to get set to a value which is > less than or equal to the xmin of every snapshot that will be used by > this worker. The easiest way to accomplish that is to install the > active snapshot as the transaction snapshot. Code running in this > parallel worker might take new snapshots via GetTransactionSnapshot() > or GetLatestSnapshot(), but it shouldn't have any way of acquiring a > snapshot older than the active snapshot. > 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 ..."? since we have: #define XACT_READ_UNCOMMITTED 0 #define XACT_READ_COMMITTED 1 #define XACT_REPEATABLE_READ 2 #define XACT_SERIALIZABLE 3 #define IsolationUsesXactSnapshot() (XactIsoLevel >= XACT_REPEATABLE_READ) Regards, Greg Nancarrow Fujitsu Australia
v10-0001-Fix-parallel-worker-failed-assertion-and-coredump.patch
Description: Binary data
