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

Search Discussions

  • Bruce Momjian at Aug 14, 2012 at 9:41 pm
    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. +
  • Robert Haas at Aug 21, 2012 at 3:48 pm

    On Tue, Aug 14, 2012 at 5:41 PM, Bruce Momjian wrote:
    Did these comment updates ever get addressed?
    Partially.

    I just made a commit to clean up the rest of it.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Bruce Momjian at Aug 23, 2012 at 2:13 pm

    On Tue, Aug 21, 2012 at 11:48:19AM -0400, Robert Haas wrote:
    On Tue, Aug 14, 2012 at 5:41 PM, Bruce Momjian wrote:
    Did these comment updates ever get addressed?
    Partially.

    I just made a commit to clean up the rest of it.
    Thanks.

    --
    Bruce Momjian <bruce@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedAug 5, '11 at 3:02p
activeAug 23, '12 at 2:13p
posts4
users2
websitepostgresql.org...
irc#postgresql

2 users in discussion

Bruce Momjian: 2 posts Robert Haas: 2 posts

People

Translate

site design / logo © 2021 Grokbase