From 0009462e985b90d07dc430bb3c4f1e6f57e0c318 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 2 Aug 2013 12:49:03 -0400 Subject: [PATCH] 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. --- src/backend/optimizer/plan/planner.c | 3 ++- src/backend/parser/analyze.c | 25 +++++++++---------------- src/include/parser/analyze.h | 2 +- src/test/regress/expected/portals.out | 4 ++++ src/test/regress/expected/union.out | 2 ++ src/test/regress/sql/portals.sql | 3 +++ src/test/regress/sql/union.sql | 2 ++ 7 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 4059c666c6..6047d7c58e 100644 --- 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 39036fbc86..a9d1fecff5 100644 --- 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 b24b205e45..2fef2f7a97 100644 --- 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 01152a939d..aafc6cf529 100644 --- 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 bf8f1bcaf7..ae690cf968 100644 --- 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 02286c4096..203e657703 100644 --- 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 6194ce4751..d567cf1481 100644 --- 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 -- -- 2.40.0