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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +