Subject: Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData()
	during Hot Standby by avoiding loop
From: Simon Riggs <simon@2ndQuadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
In-Reply-To: <14068.1271688932@sss.pgh.pa.us>
References: <20100418180608.16F067541D1@cvs.postgresql.org>
	<4BCC07F1.6040405@enterprisedb.com> <1271664332.8305.17430.camel@ebony>
	<4BCC1644.70008@enterprisedb.com> <1271667922.8305.17650.camel@ebony>
	<v2y603c8f071004190455gf18633fbx1a666c17c998b1a1@mail.gmail.com>
	<13439.1271687068@sss.pgh.pa.us> <1271687489.8305.18879.camel@ebony>
	<4BCC6C37.9090703@enterprisedb.com> <1271688445.8305.18966.camel@ebony>
	<14068.1271688932@sss.pgh.pa.us>
Content-Type: multipart/mixed; boundary="=-+6v72DCFilhtjCQAVbaW"
Date: Mon, 19 Apr 2010 19:17:56 +0100
Message-Id: <1271701076.8305.19751.camel@ebony>
Mime-Version: 1.0


--=-+6v72DCFilhtjCQAVbaW
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

On Mon, 2010-04-19 at 10:55 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Mon, 2010-04-19 at 17:44 +0300, Heikki Linnakangas wrote:
> >>> Choices are
> >>> 1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
> >>> Murphy
> >>> 2. Check RecoveryInProgress() before and after holding lock
> >>> 3. Check RecoveryInProgress() while holding lock
> >> 
> >> 4. Check RecoveryInProgress() once outside of lock, and scan the
> >> ProcArray anyway, just in case. That's what we did before this patch.
> >> Document that takenDuringRecovery == true means that the snapshot was
> >> most likely taken during recovery, but there is some race conditions
> >> where takenDuringRecovery is true even though the snapshot was taken
> >> just after recovery finished. AFAICS all of the other current uses of
> >> takenDuringRecovery work fine with that.
> 
> > Checking RecoveryInProgress() is much cheaper than scanning the whole
> > ProcArray, so (4) is definitely worse than 1-3.
> 
> If the lock we're talking about is an LWLock, #3 is okay.  If it's a
> spinlock, not so much.

Just committed the following patch to implement option #3.

We test RecoveryInProgress() after the LWLockAcquire rather than before.

-- 
 Simon Riggs           www.2ndQuadrant.com

--=-+6v72DCFilhtjCQAVbaW
Content-Disposition: attachment; filename="switcheroo.patch"
Content-Type: text/x-patch; name="switcheroo.patch"; charset="UTF-8"
Content-Transfer-Encoding: 7bit

*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***************
*** 1074,1081 **** GetSnapshotData(Snapshot snapshot)
  					 errmsg("out of memory")));
  	}
  
- 	snapshot->takenDuringRecovery = RecoveryInProgress();
- 
  	/*
  	 * It is sufficient to get shared lock on ProcArrayLock, even if we are
  	 * going to set MyProc->xmin.
--- 1074,1079 ----
***************
*** 1091,1098 **** GetSnapshotData(Snapshot snapshot)
  	globalxmin = xmin = xmax;
  
  	/*
! 	 * If in recovery get any known assigned xids.
  	 */
  	if (!snapshot->takenDuringRecovery)
  	{
  		/*
--- 1089,1103 ----
  	globalxmin = xmin = xmax;
  
  	/*
! 	 * 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();
+ 
  	if (!snapshot->takenDuringRecovery)
  	{
  		/*

--=-+6v72DCFilhtjCQAVbaW--