Fix crash in error report of invalid tuple lock

My tweak of these error messages in commit c359a1b082 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
index 4059c666c66546a7913054e559dd0b1ad74b6105..6047d7c58e5591631d6a0d2c5170d0fc5d5b8cbd 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1936,7 +1936,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
index 39036fbc868d6ce9571dc1fe29564caa0eb4d01d..a9d1fecff5cf4b2b7aef3b5353448f25e35a1e94 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2243,7 +2243,7 @@ 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,
@@ -2251,56 +2251,49 @@ CheckSelectLocking(Query *qry)
                                 /*------
                                   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
index b24b205e45866c716fe90932d3ff0a317a9ffa75..2fef2f7a97f31a008a69f4de8cf611857a8d611c 100644 (file)
--- 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/portals.out b/src/test/regress/expected/portals.out
index 01152a939d3ff7637c63e4bbb032cd3f029e94fe..aafc6cf529416ae3e13c063c8f805dab1e578cdb 100644 (file)
--- 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
index bf8f1bcaf77f7751a45e72693d66d6857ecb995d..ae690cf9689ebc8e3e2cda052df2f0bb144f2938 100644 (file)
--- 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/portals.sql b/src/test/regress/sql/portals.sql
index 02286c4096eaa0ead285f14b44742098f28d33db..203e6577039201c317ea223ef867ba1b781e94aa 100644 (file)
--- 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
index 6194ce475110944abb69951b70c66ef368a8834a..d567cf148191870756825fde711784a2704e60e6 100644 (file)
--- 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
 --