On Tue, Aug 24, 2021 at 5:00 AM Robert Haas <robertmhaas@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
