Thread

1. Hot standby, race condition between recovery snapshot and commit

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-14T12:59:16Z

   There's a race condition between transaction commit and
   GetRunningTransactionData(). If GetRunningTransactionData() runs between
    the RecordTransactionCommit() and ProcArrayEndTransaction() calls in
   CommitTransaction():
   
   > 	/*
   > 	 * Here is where we really truly commit.
   > 	 */
   > 	latestXid = RecordTransactionCommit(false);
   > 
   > 	TRACE_POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);
   > 
   > 	/*
   > 	 * Let others know about no transaction in progress by me. Note that this
   > 	 * must be done _before_ releasing locks we hold and _after_
   > 	 * RecordTransactionCommit.
   > 	 */
   > 	ProcArrayEndTransaction(MyProc, latestXid);
   
   The running-xacts snapshot will include the transaction that's just
   committing, but the commit record will be before the running-xacts WAL
   record. If standby initializes transaction tracking from that
   running-xacts record, it will consider the just-committed transactions
   as still in-progress until the next running-xact record (at next
   checkpoint).
   
   I can't see any obvious way around that. We could have transaction
   commit acquire the new RecoveryInfoLock across those two calls, but I'd
   like to avoid putting any extra overhead into such a critical path.
   
   Hmm, actually ProcArrayApplyRecoveryInfo() could check every xid in the
   running-xacts record against clog. If it's marked as finished in clog
   already (because we already saw the commit/abort record before the
   running-xacts record), we know it's not running after all.
   
   Because of the sequence that commit removes entry from procarray and
   releases locks, it also seems possible for GetRunningTransactionsData()
   to acquire a snapshot that contains an AccessExclusiveLock for a
   transaction, but that XID is not listed as running in the XID list. That
   sounds like trouble too.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

2. Re: Hot standby, race condition between recovery snapshot and commit

   Simon Riggs <simon@2ndquadrant.com> — 2009-11-14T21:08:51Z

   On Sat, 2009-11-14 at 14:59 +0200, Heikki Linnakangas wrote:
   > There's a race condition ....
   
   Yes, I believe this is a major showstopper for the current
   approach/attempt....but...
   
   > I can't see any obvious way around that. 
   
   Huh? We're only doing this strict locking approach because you insisted
   that the looser approach was not acceptable. Have you forgotten that
   discussion so completely that you can't even remember the existence of
   other options? 
   
   It amazes me that you should then use locking overhead as the reason to
   not pursue the current approach further, which was exactly my argument
   for not pursuing it in the first place.
   
   You're leading me a merry dance.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

3. Re: Hot standby, race condition between recovery snapshot and commit

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-15T12:32:31Z

   Simon Riggs wrote:
   > On Sat, 2009-11-14 at 14:59 +0200, Heikki Linnakangas wrote:
   >> I can't see any obvious way around that. 
   > 
   > Huh? We're only doing this strict locking approach because you insisted
   > that the looser approach was not acceptable.
   
   Take it easy, Simon. By obvious, I meant "trivial" or "easy".  I believe
   you're referring to this
   (http://archives.postgresql.org/message-id/4A8CE561.4000302@enterprisedb.com):
   > If there's a way, I would prefer a solution where the RunningXacts
   > snapshot represents the situation the moment it appears in WAL, not some
   > moment before it. It makes the logic easier to understand.
   
   or did we have further discussion on that since?
   
   > Have you forgotten that
   > discussion so completely that you can't even remember the existence of
   > other options? 
   
   I do remember that. I've been thinking about the looser approach a lot
   since yesterday.
   
   So, if we drop the notion that the running-xacts record represents the
   situation at the exact moment it appears in WAL, what do we have to
   change? Creating the running-xacts snapshot becomes easier, but when we
   replay it, we must take the snapshot with a grain of salt.
   
   1. the snapshot can contain xids that have already finished (= we've
   already seen the commit/abort record)
   2. the snapshot can lack xids belonging to transactions that have just
   started, between the window when the running-xacts snapshot is taken in
   the master and it's written to WAL.
   
   Problem 1 is quite easy to handle: just check every xid in clog. If it's
   marked there as finished already, it can be ignored.
   
   For problem 2, if a transaction hasn't written any WAL yet, we might as
   well treat it as not-yet-started in the standby, so we're concerned
   about transactions that have written a WAL record between when the
   running-xacts snapshot was taken and written to WAL. Assuming the
   snapshot was taken after the REDO pointer of the checkpoint record, the
   standby has seen the WAL record and therefore has all the information it
   needs. Currently, the standby doesn't add xids to known-assigned list
   until it sees the running-xacts record, but we could change that.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

4. Re: Hot standby, race condition between recovery snapshot and commit

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-15T12:43:02Z

   Oh, forgot to mention another thing that I've been pondering:
   
   Currently, the running-xacts record is written to the WAL after the
   checkpoint record. There's a small chance that you get an xlog switch in
   between. If that happens, it might take a long time after the checkpoint
   record until the standby sees the running-xacts record, so it might take
   a long time until the standby can open up for connections.
   
   In general, I'd like to remove as many as possible of those cases where
   the standby starts up, and can't open up for connections. It makes the
   standby a lot less useful if you can't rely on it being open. So I'd
   like to make it so that the standby can *always* open up. There's
   currently three cases where that can happen:
   
   1. If the subxid cache has overflown.
   
   2. If there's no running-xacts record after the checkpoint record for
   some reason. For example, one was written but not archive yet, or
   because the master crashed before it was written.
   
   3. If too many AccessExclusiveLocks was being held.
   
   Case 3 should be pretty easy to handle. Just need to WAL log all the
   AccessExclusiveLocks, perhaps as separate WAL records (we already have a
   new WAL record type for logging locks) if we're worried about the
   running-xacts record growing too large. I think we could handle case 2
   if we wrote the running-xacts record *before* the checkpoint record.
   Then it would be always between the REDO pointer of the checkpoint
   record, and the checkpoint record itself, so it would always be seen by
   the WAL recovery. To handle case 1, we could scan pg_subtrans. It would
   add some amount of code and would add some more work to taking the
   running-xacts snapshot, but it could be done.
   
   This isn't absolutely necessary for the first version, but it's
   something to keep in mind...
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

5. Re: Hot standby, race condition between recovery snapshot and commit

   Simon Riggs <simon@2ndquadrant.com> — 2009-11-15T14:19:24Z

   On Sun, 2009-11-15 at 14:43 +0200, Heikki Linnakangas wrote:
   
   > This isn't absolutely necessary for the first version, but it's
   > something to keep in mind...
   
   Do I take that as agreement to the phased plan?
   
   > In general, I'd like to remove as many as possible of those cases
   > where the standby starts up, and can't open up for connections. It
   > makes the standby a lot less useful if you can't rely on it being
   > open. So I'd like to make it so that the standby can *always* open up.
   
   Yes, of course. The only reason for restrictions being acceptable is
   that we have 99% of what we want, yet may lose everything if we play for
   100% too quickly.
   
   The standby will open quickly in many cases, as is. There are also a
   range of other ways of doing this.
   
   > There's currently three cases where that can happen:
   > 
   > 1. If the subxid cache has overflown.
   > 
   > 2. If there's no running-xacts record after the checkpoint record for
   > some reason. For example, one was written but not archive yet, or
   > because the master crashed before it was written.
   > 
   > 3. If too many AccessExclusiveLocks was being held.
   > 
   > Case 3 should be pretty easy to handle. Just need to WAL log all the
   > AccessExclusiveLocks, perhaps as separate WAL records (we already have
   > a
   > new WAL record type for logging locks) if we're worried about the
   > running-xacts record growing too large. I think we could handle case 2
   > if we wrote the running-xacts record *before* the checkpoint record.
   > Then it would be always between the REDO pointer of the checkpoint
   > record, and the checkpoint record itself, so it would always be seen
   > by
   > the WAL recovery. To handle case 1, we could scan pg_subtrans. It
   > would
   > add some amount of code and would add some more work to taking the
   > running-xacts snapshot, but it could be done.
   
   "Some amount of code" requires some amount of thought, followed by some
   amount of review which takes some amount of time.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

6. Re: Hot standby, race condition between recovery snapshot and commit

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-15T19:37:59Z

   Heikki Linnakangas wrote:
   > Simon Riggs wrote:
   >> Have you forgotten that
   >> discussion so completely that you can't even remember the existence of
   >> other options? 
   > 
   > I do remember that. I've been thinking about the looser approach a lot
   > since yesterday.
   > 
   > So, if we drop the notion that the running-xacts record represents the
   > situation at the exact moment it appears in WAL, what do we have to
   > change? Creating the running-xacts snapshot becomes easier, but when we
   > replay it, we must take the snapshot with a grain of salt.
   > 
   > 1. the snapshot can contain xids that have already finished (= we've
   > already seen the commit/abort record)
   > 2. the snapshot can lack xids belonging to transactions that have just
   > started, between the window when the running-xacts snapshot is taken in
   > the master and it's written to WAL.
   > 
   > Problem 1 is quite easy to handle: just check every xid in clog. If it's
   > marked there as finished already, it can be ignored.
   > 
   > For problem 2, if a transaction hasn't written any WAL yet, we might as
   > well treat it as not-yet-started in the standby, so we're concerned
   > about transactions that have written a WAL record between when the
   > running-xacts snapshot was taken and written to WAL. Assuming the
   > snapshot was taken after the REDO pointer of the checkpoint record, the
   > standby has seen the WAL record and therefore has all the information it
   > needs. Currently, the standby doesn't add xids to known-assigned list
   > until it sees the running-xacts record, but we could change that.
   
   Ok, I tried out that approach. Attached is a complete patch against CVS
   HEAD (see commit db15148b930 in the git branch for the diff against the
   old approach):
   
   - We start tracking transactions in the known-assigned hash table
   immediately from the start of WAL replay. We have to do that because the
   running-xacts record we will eventually see lack XIDs belonging to
   transactions that started between when the running-xacts snapshot was
   taken and written to WAL. If we start tracking at the running-xacts
   record, we will miss them. To keep the size of the known-assigned table
   bounded, we ignore any XIDs smaller than the oldest XID present in the
   running-xacts record (any such transaction must've finished before the
   running-xacts record, so we're not interested in them). We wouldn't know
   the oldest running XID until we see the running-xacts record, so we
   store it in the checkpoint record too, which we have access to right
   from the start.
   
   - StartupCLOG/SUBTRANS/MultiXact are now called at the beginning of WAL
   replay. We used to delay that until we saw the running-xacts record, but
   that always felt a bit weird to me. StartupSUBTRANS takes the
   oldest-running-xid as argument, but now that we store that in the
   checkpoint record, that's not a problem.
   
   - Because the running-xacts record can contain XIDs belonging to
   transactions that finished before the record was written to WAL, we
   ignore any already-finished XIDs when it's replayed.
   
   - The running-xacts record is written to WAL before the checkpoint
   record. That guarantees that WAL replay will see it.
   
   - RecoveryInfoLock is no longer needed.
   
   This also lays the foundation to allow standby mode even with subxid or
   lock overflows. We could now emit separate log records for overflowed
   subxids or locks before the running-xacts record to fill that gap.
   
   Am I missing anything?
   
   I also experimented with including the running-xacts information in the
   checkpoint record itself. That somehow feels more straightforward to me,
   but it wasn't really any less code, and it wouldn't allow us to do the
   running-xacts snapshot as multiple WAL records, so the current approach
   with separate running-xacts record is better.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   

7. Re: Hot standby, race condition between recovery snapshot and commit

   Simon Riggs <simon@2ndquadrant.com> — 2009-11-15T20:10:45Z

   On Sun, 2009-11-15 at 21:37 +0200, Heikki Linnakangas wrote:
   
   > Am I missing anything?
   
   Will review.
   
   > I also experimented with including the running-xacts information in the
   > checkpoint record itself. That somehow feels more straightforward to me,
   > but it wasn't really any less code, and it wouldn't allow us to do the
   > running-xacts snapshot as multiple WAL records, so the current approach
   > with separate running-xacts record is better.
   
   Agreed, more modular.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

8. Re: Hot standby, race condition between recovery snapshot and commit

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-15T20:23:45Z

   Simon Riggs wrote:
   > On Sun, 2009-11-15 at 21:37 +0200, Heikki Linnakangas wrote:
   > 
   >> Am I missing anything?
   > 
   > Will review.
   
   Thanks! Please use the head of git branch, I already found one major
   oversight in what I posted that's fixed there... I should go to bed already.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com