Fix crash in error report of invalid tuple lock

alvherre · alvherre · commit 88c556680ca3 · 2013-08-02T13:18:37.000-04:00
My tweak of these error messages in commit c359a1b contained the thinko that a query would always have rowMarks set for a query containing a locking clause. Not so: when declaring a cursor, for instance, rowMarks isn't set at the point we're checking, so we'd be dereferencing a NULL pointer. The fix is to pass the lock strength to the function raising the error, instead of trying to reverse-engineer it. The result not only is more robust, but it also seems cleaner overall. Per report from Robert Haas.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
@@ -1962,7 +1962,8 @@ preprocess_rowmarks(PlannerInfo *root)
 		 * CTIDs invalid.  This is also checked at parse time, but that's
 		 * insufficient because of rule substitution, query pullup, etc.
 		 */
-		CheckSelectLocking(parse);
+		CheckSelectLocking(parse, ((RowMarkClause *)
+							linitial(parse->rowMarks))->strength);
 	}
 	else
 	{
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
@@ -2243,64 +2243,57 @@ LCS_asString(LockClauseStrength strength)
  * exported so planner can check again after rewriting, query pullup, etc
  */
 void
-CheckSelectLocking(Query *qry)
+CheckSelectLocking(Query *qry, LockClauseStrength strength)
 {
 	if (qry->setOperations)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with UNION/INTERSECT/EXCEPT",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (qry->distinctClause != NIL)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with DISTINCT clause",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (qry->groupClause != NIL)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with GROUP BY clause",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (qry->havingQual != NULL)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with HAVING clause",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (qry->hasAggs)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with aggregate functions",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (qry->hasWindowFuncs)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with window functions",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 	if (expression_returns_set((Node *) qry->targetList))
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 /*------
 				   translator: %s is a SQL row locking clause such as FOR UPDATE */
 				 errmsg("%s is not allowed with set-returning functions in the target list",
-						LCS_asString(((RowMarkClause *)
-									  linitial(qry->rowMarks))->strength))));
+						LCS_asString(strength))));
 }
 
 /*
@@ -2321,7 +2314,7 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
 	Index		i;
 	LockingClause *allrels;
 
-	CheckSelectLocking(qry);
+	CheckSelectLocking(qry, lc->strength);
 
 	/* make a clause we can pass down to subqueries to select all rels */
 	allrels = makeNode(LockingClause);
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
@@ -37,7 +37,7 @@ extern Query *transformStmt(ParseState *pstate, Node *parseTree);
 extern bool analyze_requires_snapshot(Node *parseTree);
 
 extern char *LCS_asString(LockClauseStrength strength);
-extern void CheckSelectLocking(Query *qry);
+extern void CheckSelectLocking(Query *qry, LockClauseStrength strength);
 extern void applyLockingClause(Query *qry, Index rtindex,
 				   LockClauseStrength strength, bool noWait, bool pushedDown);
 
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
@@ -292,6 +292,9 @@ NOTICE:  materialized view "no_such_mv" does not exist, skipping
 -- make sure invalid comination of options is prohibited
 REFRESH MATERIALIZED VIEW CONCURRENTLY tvmm WITH NO DATA;
 ERROR:  CONCURRENTLY and WITH NO DATA options cannot be used together
+-- no tuple locks on materialized views
+SELECT * FROM tvvm FOR SHARE;
+ERROR:  cannot lock rows in materialized view "tvvm"
 -- test join of mv and view
 SELECT type, m.totamt AS mtot, v.totamt AS vtot FROM tm m LEFT JOIN tv v USING (type) ORDER BY type;
  type | mtot | vtot 
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
@@ -1226,6 +1226,10 @@ DECLARE c1 CURSOR FOR SELECT * FROM uctest;
 DELETE FROM uctest WHERE CURRENT OF c1; -- fail, no current row
 ERROR:  cursor "c1" is not positioned on a row
 ROLLBACK;
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT MIN(f1) FROM uctest FOR UPDATE;
+ERROR:  FOR UPDATE is not allowed with aggregate functions
+ROLLBACK;
 -- WHERE CURRENT OF may someday work with views, but today is not that day.
 -- For now, just make sure it errors out cleanly.
 CREATE TEMP VIEW ucview AS SELECT * FROM uctest;
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
@@ -317,6 +317,8 @@ SELECT q1 FROM int8_tbl EXCEPT ALL SELECT DISTINCT q2 FROM int8_tbl;
               123
 (3 rows)
 
+SELECT q1 FROM int8_tbl EXCEPT ALL SELECT q1 FROM int8_tbl FOR NO KEY UPDATE;
+ERROR:  FOR NO KEY UPDATE is not allowed with UNION/INTERSECT/EXCEPT
 --
 -- Mixed types
 --
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
@@ -95,6 +95,9 @@ DROP MATERIALIZED VIEW IF EXISTS no_such_mv;
 -- make sure invalid comination of options is prohibited
 REFRESH MATERIALIZED VIEW CONCURRENTLY tvmm WITH NO DATA;
 
+-- no tuple locks on materialized views
+SELECT * FROM tvvm FOR SHARE;
+
 -- test join of mv and view
 SELECT type, m.totamt AS mtot, v.totamt AS vtot FROM tm m LEFT JOIN tv v USING (type) ORDER BY type;
 
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
@@ -447,6 +447,9 @@ BEGIN;
 DECLARE c1 CURSOR FOR SELECT * FROM uctest;
 DELETE FROM uctest WHERE CURRENT OF c1; -- fail, no current row
 ROLLBACK;
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT MIN(f1) FROM uctest FOR UPDATE;
+ROLLBACK;
 
 -- WHERE CURRENT OF may someday work with views, but today is not that day.
 -- For now, just make sure it errors out cleanly.
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
@@ -109,6 +109,8 @@ SELECT q1 FROM int8_tbl EXCEPT ALL SELECT q2 FROM int8_tbl;
 
 SELECT q1 FROM int8_tbl EXCEPT ALL SELECT DISTINCT q2 FROM int8_tbl;
 
+SELECT q1 FROM int8_tbl EXCEPT ALL SELECT q1 FROM int8_tbl FOR NO KEY UPDATE;
+
 --
 -- Mixed types
 --

Original file line number	Diff line number	Diff line change
`@@ -1962,7 +1962,8 @@ preprocess_rowmarks(PlannerInfo *root)`
`1962`	`1962`	`* CTIDs invalid. This is also checked at parse time, but that's`
`1963`	`1963`	`* insufficient because of rule substitution, query pullup, etc.`
`1964`	`1964`	`*/`
`1965`		`- CheckSelectLocking(parse);`
	`1965`	`+ CheckSelectLocking(parse, ((RowMarkClause *)`
	`1966`	`+ linitial(parse->rowMarks))->strength);`
`1966`	`1967`	`}`
`1967`	`1968`	`else`
`1968`	`1969`	`{`
Original file line number	Diff line number	Diff line change
`@@ -317,6 +317,8 @@ SELECT q1 FROM int8_tbl EXCEPT ALL SELECT DISTINCT q2 FROM int8_tbl;`
`317`	`317`	`123`
`318`	`318`	`(3 rows)`
`319`	`319`
	`320`	`+SELECT q1 FROM int8_tbl EXCEPT ALL SELECT q1 FROM int8_tbl FOR NO KEY UPDATE;`
	`321`	`+ERROR: FOR NO KEY UPDATE is not allowed with UNION/INTERSECT/EXCEPT`
`320`	`322`	`--`
`321`	`323`	`-- Mixed types`
`322`	`324`	`--`