On Wed, Aug 18, 2021 at 9:28 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> Yes, I think I agree on that.
> I've updated the patch to restore the actual transaction snapshot in
> the IsolationUsesXactSnapshot() case, otherwise the active snapshot is
> installed as the transaction snapshot.
> I've tested the patch for the different transaction isolation levels,
> and the reported coredump (from assertion failure) is not occurring.
> (In the "serializable" case there are "could not serialize access due
> to read/write dependencies among transactions" errors, as Pavel has
> previously reported, but these occur without the patch and it appears
> to be an unrelated issue)
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 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.
--
Robert Haas
EDB: http://www.enterprisedb.com