Re: GetSnapshotData() comments

Did these comment updates ever get addressed?

---------------------------------------------------------------------------

On Fri, Aug 5, 2011 at 11:02:24AM -0400, Robert Haas wrote:
> I think that the first sentence, in the following comment within
> GetSnapshotData(), is not quite right, because at the time this is
> executed, we already hold ProcArrayLock, which is the only lock that
> gets grabbed here:
>
> /*
> * If we're in recovery then snapshot data comes from a different place,
> * so decide which route we take before grab the lock. It is
> possible for
> * recovery to end before we finish taking snapshot, and for newly
> * assigned transaction ids to be added to the procarray. Xmax cannot
> * change while we hold ProcArrayLock, so those newly added transaction
> * ids would be filtered away, so we need not be concerned about them.
> */
> snapshot->takenDuringRecovery = RecoveryInProgress();
>
> Having thought it over for a bit, I believe that the code is correct
> and it's only the comment that needs to be updated. If we were to set
> snapshot->takenDuringRecovery before acquiring ProcArrayLock, then the
> following sequence of events might occur:
>
> T1: Enter GetSnapshotData(). Set snapshot->takenDuringRecovery = true.
> Recovery ends
> T2: Begin, get an XID.
> T3: Begin, get an XID.
> T3: Commit, advancing latestCompletedXid.
> T1: Acquire ProcArrayLock and use the "in recovery" path, missing T2's
> XID (because it's not in the subxip[] array).
> T1: Do some stuff with the snapshot... not seeing T2's XID...
> T2: Commit
> T1: Do some stuff with the snapshot... now seeing T2's XID...
>
> I think if we just delete the first sentence of the comment, the rest
> is all correct.
>
> A little further down, there is this comment:
>
> /*
> * Spin over procArray checking xid, xmin, and
> subxids. The goal is
> * to gather all active xids, find the lowest xmin,
> and try to record
> * subxids. During recovery no xids will be assigned,
> so all normal
> * backends can be ignored, nor are there any VACUUMs
> running. All
> * prepared transaction xids are held in
> KnownAssignedXids, so these
> * will be seen without needing to loop through procs here.
> */
>
> ...but this code is only executed when recovery is not in progress.
> So I feel like everything after "try to record subxids" should either
> be removed, or relocated to the following "else" block, where the
> recovery path is discussed in detail.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +