Message-ID: <51FB6703.9090801@2ndquadrant.com>
Date: Fri, 02 Aug 2013 16:00:03 +0800
From: Craig Ringer <craig@2ndquadrant.com>
Organization: 2nd Quadrant
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:17.0) Gecko/20130625 Thunderbird/17.0.7
MIME-Version: 1.0
To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: FOR [SHARE|UPDATE] NOWAIT may still block in EvalPlanQualFetch
Content-Type: multipart/mixed; boundary="------------010709070503000206070801"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------010709070503000206070801
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid
chain because the call to EvalPlanQualFetch doesn't take a param for
noWait, so it doesn't know not to block if the updated row can't be locked.

The attached patch against master includes an isolationtester spec to
demonstrate this issue and a proposed fix. Builds with the fix applied
pass "make check" and isolationtester "make installcheck".

To reach this point you need to apply the patch in
http://www.postgresql.org/message-id/51FB4305.3070600@2ndquadrant.com
first, otherwise you'll get stuck there and won't touch the code changed
in this patch.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

--------------010709070503000206070801
Content-Type: text/x-patch;
 name="0001-Add-noWait-param-to-EvalPlanQualFetch.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-Add-noWait-param-to-EvalPlanQualFetch.patch"

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


--------------010709070503000206070801
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--------------010709070503000206070801--