FOR [SHARE|UPDATE] NOWAIT may still block in EvalPlanQualFetch

>From 76ff647f8997221de2a6981a51859d12a6b70276 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 2 Aug 2013 14:18:34 +0800
Subject: [PATCH] Add noWait param to EvalPlanQualFetch

FOR SHARE|UPDATE NOWAIT would still wait if they had to follow a
ctid chain because EvalPlanQualFetch couldn't tell if a statement
was NOWAIT and would just assume it should block.
---
 src/backend/executor/execMain.c                    |  7 +--
 src/backend/executor/nodeLockRows.c                |  2 +-
 src/include/executor/executor.h                    |  2 +-
 .../expected/for-updateshare-nowait-nonblock.out   | 41 +++++++++++++++
 src/test/isolation/isolation_schedule              |  1 +
 .../specs/for-updateshare-nowait-nonblock.spec     | 58 ++++++++++++++++++++++
 6 files changed, 106 insertions(+), 5 deletions(-)
 create mode 100644 src/test/isolation/expected/for-updateshare-nowait-nonblock.out
 create mode 100644 src/test/isolation/specs/for-updateshare-nowait-nonblock.spec

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 038f064..412dc9a 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1839,7 +1839,7 @@ EvalPlanQual(EState *estate, EPQState *epqstate,
 	/*
 	 * Get and lock the updated version of the row; if fail, return NULL.
 	 */
-	copyTuple = EvalPlanQualFetch(estate, relation, lockmode,
+	copyTuple = EvalPlanQualFetch(estate, relation, lockmode, false /* wait */,
 								  tid, priorXmax);
 
 	if (copyTuple == NULL)
@@ -1898,6 +1898,7 @@ EvalPlanQual(EState *estate, EPQState *epqstate,
  *	estate - executor state data
  *	relation - table containing tuple
  *	lockmode - requested tuple lock mode
+ *	noWait - wait mode to pass to heap_lock_tuple
  *	*tid - t_ctid from the outdated tuple (ie, next updated version)
  *	priorXmax - t_xmax from the outdated tuple
  *
@@ -1910,7 +1911,7 @@ EvalPlanQual(EState *estate, EPQState *epqstate,
  * but we use "int" to avoid having to include heapam.h in executor.h.
  */
 HeapTuple
-EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
+EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, bool noWait,
 				  ItemPointer tid, TransactionId priorXmax)
 {
 	HeapTuple	copyTuple = NULL;
@@ -1986,7 +1987,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
 			 */
 			test = heap_lock_tuple(relation, &tuple,
 								   estate->es_output_cid,
-								   lockmode, false /* wait */ ,
+								   lockmode, noWait,
 								   false, &buffer, &hufd);
 			/* We now have two pins on the buffer, get rid of one */
 			ReleaseBuffer(buffer);
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 5b5c705..efee8a3 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -170,7 +170,7 @@ lnext:
 				}
 
 				/* updated, so fetch and lock the updated version */
-				copyTuple = EvalPlanQualFetch(estate, erm->relation, lockmode,
+				copyTuple = EvalPlanQualFetch(estate, erm->relation, lockmode, erm->noWait,
 											  &hufd.ctid, hufd.xmax);
 
 				if (copyTuple == NULL)
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 75841c8..9c593a6 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -199,7 +199,7 @@ extern TupleTableSlot *EvalPlanQual(EState *estate, EPQState *epqstate,
 			 Relation relation, Index rti, int lockmode,
 			 ItemPointer tid, TransactionId priorXmax);
 extern HeapTuple EvalPlanQualFetch(EState *estate, Relation relation,
-				  int lockmode, ItemPointer tid, TransactionId priorXmax);
+				  int lockmode, bool noWait, ItemPointer tid, TransactionId priorXmax);
 extern void EvalPlanQualInit(EPQState *epqstate, EState *estate,
 				 Plan *subplan, List *auxrowmarks, int epqParam);
 extern void EvalPlanQualSetPlan(EPQState *epqstate,
diff --git a/src/test/isolation/expected/for-updateshare-nowait-nonblock.out b/src/test/isolation/expected/for-updateshare-nowait-nonblock.out
new file mode 100644
index 0000000..d86b2e6
--- /dev/null
+++ b/src/test/isolation/expected/for-updateshare-nowait-nonblock.out
@@ -0,0 +1,41 @@
+Parsed test spec with 3 sessions
+
+starting permutation: sl1_prep upd_getlock sl1_exec upd_doupdate lk1_doforshare upd_releaselock
+step sl1_prep: 
+	PREPARE sl1_run AS SELECT id FROM test_nowait WHERE pg_advisory_lock(0) is not null FOR UPDATE NOWAIT;
+
+step upd_getlock: 
+	SELECT pg_advisory_lock(0);
+
+pg_advisory_lock
+
+               
+step sl1_exec: 
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	EXECUTE sl1_run;
+	SELECT xmin, xmax, ctid, * FROM test_nowait;
+ <waiting ...>
+step upd_doupdate: 
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	UPDATE test_nowait SET value = value WHERE id % 2 = 0;
+	COMMIT;
+
+step lk1_doforshare: 
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	SELECT id FROM test_nowait WHERE id % 2 = 0 FOR SHARE;
+
+id             
+
+2              
+4              
+6              
+8              
+10             
+step upd_releaselock: 
+	SELECT pg_advisory_unlock(0);
+
+pg_advisory_unlock
+
+t              
+step sl1_exec: <... completed>
+error in steps upd_releaselock sl1_exec: ERROR:  could not obtain lock on row in relation "test_nowait"
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 081e11f..15053ca 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -21,3 +21,4 @@ test: delete-abort-savept-2
 test: aborted-keyrevoke
 test: drop-index-concurrently-1
 test: timeouts
+test: for-updateshare-nowait-nonblock
diff --git a/src/test/isolation/specs/for-updateshare-nowait-nonblock.spec b/src/test/isolation/specs/for-updateshare-nowait-nonblock.spec
new file mode 100644
index 0000000..bb1ef98
--- /dev/null
+++ b/src/test/isolation/specs/for-updateshare-nowait-nonblock.spec
@@ -0,0 +1,58 @@
+# Test excersising a case where NOWAIT still blocked in EvalPlanQualFetch.
+# Verifies that NOWAIT is now respected in this case.
+
+setup
+{
+
+  DROP TABLE IF EXISTS test_nowait;
+  CREATE TABLE test_nowait (
+	id integer PRIMARY KEY,
+	value integer not null
+  );
+
+  INSERT INTO test_nowait
+  SELECT x,x FROM generate_series(1,10) x;
+}
+
+teardown
+{
+  DROP TABLE test_nowait;
+}
+
+session "sl1"
+step    "sl1_prep"         {
+	PREPARE sl1_run AS SELECT id FROM test_nowait WHERE pg_advisory_lock(0) is not null FOR UPDATE NOWAIT;
+}
+step 	"sl1_exec" {
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	EXECUTE sl1_run;
+	SELECT xmin, xmax, ctid, * FROM test_nowait;
+}
+teardown                    { COMMIT; }
+
+# A session that's used for an UPDATE of the rows to be locked, for when we're testing ctid
+# chain following.
+session "upd"
+step	"upd_getlock" {
+	SELECT pg_advisory_lock(0);
+}
+step    "upd_doupdate" {
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	UPDATE test_nowait SET value = value WHERE id % 2 = 0;
+	COMMIT;
+}
+step 	"upd_releaselock" {
+	SELECT pg_advisory_unlock(0);
+}
+
+# A session that acquires locks that sl1 is supposed to avoid blocking on
+session "lk1"
+step    "lk1_doforshare" {
+	BEGIN ISOLATION LEVEL READ COMMITTED;
+	SELECT id FROM test_nowait WHERE id % 2 = 0 FOR SHARE;
+}
+teardown {
+	COMMIT;
+}
+
+permutation "sl1_prep" "upd_getlock" "sl1_exec" "upd_doupdate" "lk1_doforshare" "upd_releaselock"
-- 
1.8.3.1