Re: [HACKERS] new "row-level lock" error messages

2013-08-01 Thread Alvaro Herrera 
Robert Haas escribió:

> The fact that there are no tests of this functionality is probably not
> a good thing.  We should add some.  At the moment, the following test
> case crashes, and it looks like this commit is responsible:
> 
> create table test_update2 (id integer);
> DECLARE test_update_cursor CURSOR FOR SELECT id, MIN(id) FROM
> test_update2 GROUP By id HAVING MIN(id) < 1 FOR UPDATE;

The attached patch fixes it, and I think it makes more sense than the
original coding to start with.

I will commit this tomorrow, adding a few test cases while at it.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 9ff8050..49bd930 100644
--- 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
index 39036fb..a9d1fec 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 b24b205..2fef2f7 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);
+exter

Re: [HACKERS] new "row-level lock" error messages

2013-08-01 Thread Alvaro Herrera 
Robert Haas escribió:

> The fact that there are no tests of this functionality is probably not
> a good thing.  We should add some.

No disagreement.

> At the moment, the following test
> case crashes, and it looks like this commit is responsible:
> 
> create table test_update2 (id integer);
> DECLARE test_update_cursor CURSOR FOR SELECT id, MIN(id) FROM
> test_update2 GROUP By id HAVING MIN(id) < 1 FOR UPDATE;

Grr.  Thanks.  Will fix.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] new "row-level lock" error messages

2013-08-01 Thread Robert Haas 
On Tue, Jul 23, 2013 at 2:16 PM, Alvaro Herrera
 wrote:
> Alvaro Herrera wrote:
>> Peter Eisentraut wrote:
>>
>> > I would suggest that these changes be undone, except that the old
>> > "SELECT FOR ..." be replaced by a dynamic string that reverse-parses the
>> > LockingClause to provide the actual clause that was used.
>>
>> Here's a patch for this.
>
> Pushed to 9.3 and master.  Sample output:
>
> alvherre=# select * from foo, bar for update of foof for share of bar;
> ERROR:  relation "foof" in FOR UPDATE clause not found in FROM clause
>
> alvherre=# select * from foo, bar for update of foo for share of barf;
> ERROR:  relation "barf" in FOR SHARE clause not found in FROM clause
>
> Amusingly, the only test in which these error messages appeared, in
> contrib/file_fdw, was removed after the two commits that changed the
> wording.  So there's not a single test which needed to be tweaked for
> this change.

The fact that there are no tests of this functionality is probably not
a good thing.  We should add some.  At the moment, the following test
case crashes, and it looks like this commit is responsible:

create table test_update2 (id integer);
DECLARE test_update_cursor CURSOR FOR SELECT id, MIN(id) FROM
test_update2 GROUP By id HAVING MIN(id) < 1 FOR UPDATE;

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] new "row-level lock" error messages

2013-07-23 Thread Alvaro Herrera 
Alvaro Herrera wrote:
> Peter Eisentraut wrote:
> 
> > I would suggest that these changes be undone, except that the old
> > "SELECT FOR ..." be replaced by a dynamic string that reverse-parses the
> > LockingClause to provide the actual clause that was used.
> 
> Here's a patch for this.

Pushed to 9.3 and master.  Sample output:

alvherre=# select * from foo, bar for update of foof for share of bar;
ERROR:  relation "foof" in FOR UPDATE clause not found in FROM clause

alvherre=# select * from foo, bar for update of foo for share of barf;
ERROR:  relation "barf" in FOR SHARE clause not found in FROM clause

Amusingly, the only test in which these error messages appeared, in
contrib/file_fdw, was removed after the two commits that changed the
wording.  So there's not a single test which needed to be tweaked for
this change.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] new "row-level lock" error messages

2013-07-22 Thread Alvaro Herrera 
Peter Eisentraut wrote:

> I would suggest that these changes be undone, except that the old
> "SELECT FOR ..." be replaced by a dynamic string that reverse-parses the
> LockingClause to provide the actual clause that was used.

Here's a patch for this.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 839ed9d..8efb94b 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -26,6 +26,7 @@
 #include "optimizer/prep.h"
 #include "optimizer/restrictinfo.h"
 #include "optimizer/var.h"
+#include "parser/analyze.h"
 #include "rewrite/rewriteManip.h"
 #include "utils/lsyscache.h"
 
@@ -883,7 +884,10 @@ make_outerjoininfo(PlannerInfo *root,
 			(jointype == JOIN_FULL && bms_is_member(rc->rti, left_rels)))
 			ereport(ERROR,
 	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-	 errmsg("row-level locks cannot be applied to the nullable side of an outer join")));
+	 /*--
+	  translator: %s is a SQL row locking clause such as FOR UPDATE */
+	 errmsg("%s cannot be applied to the nullable side of an outer join",
+			LCS_asString(rc->strength;
 	}
 
 	sjinfo->syn_lefthand = left_rels;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 01e2fa3..9ff8050 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1107,7 +1107,11 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
 		if (parse->rowMarks)
 			ereport(ERROR,
 	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-	 errmsg("row-level locks are not allowed with UNION/INTERSECT/EXCEPT")));
+	 /*--
+	   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(parse->rowMarks))->strength;
 
 		/*
 		 * Calculate pathkeys that represent result ordering requirements
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 16ff234..39036fb 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1221,7 +1221,11 @@ transformValuesClause(ParseState *pstate, SelectStmt *stmt)
 	if (stmt->lockingClause)
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-			 errmsg("SELECT FOR UPDATE/SHARE cannot be applied to VALUES")));
+ /*--
+   translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("%s cannot be applied to VALUES",
+		LCS_asString(((LockingClause *)
+	  linitial(stmt->lockingClause))->strength;
 
 	qry->rtable = pstate->p_rtable;
 	qry->jointree = makeFromExpr(pstate->p_joinlist, NULL);
@@ -1312,7 +1316,11 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
 	if (lockingClause)
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("SELECT FOR UPDATE/SHARE is not allowed with UNION/INTERSECT/EXCEPT")));
+ /*--
+   translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("%s is not allowed with UNION/INTERSECT/EXCEPT",
+		LCS_asString(((LockingClause *)
+	  linitial(lockingClause))->strength;
 
 	/* Process the WITH clause independently of all else */
 	if (withClause)
@@ -1506,7 +1514,11 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
 	if (stmt->lockingClause)
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("SELECT FOR UPDATE/SHARE is not allowed with UNION/INTERSECT/EXCEPT")));
+ /*--
+   translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("%s is not allowed with UNION/INTERSECT/EXCEPT",
+		LCS_asString(((LockingClause *)
+	  linitial(stmt->lockingClause))->strength;
 
 	/*
 	 * If an internal node of a set-op tree has ORDER BY, LIMIT, FOR UPDATE,
@@ -2063,21 +2075,33 @@ transformDeclareCursorStmt(ParseState *pstate, DeclareCursorStmt *stmt)
 	if (result->rowMarks != NIL && (stmt->options & CURSOR_OPT_HOLD))
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("DECLARE CURSOR WITH HOLD ... FOR UPDATE/SHARE is not supported"),
+ /*--
+   translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("DECLARE CURSOR WITH HOLD ... %s is not supported",
+		LCS_asString(((RowMarkClause *)
+	  linitial(result->rowMarks))->strength)),
  errdetail("Holdable cursors must be READ ONLY.")));
 
 	/* FOR UPDATE and SCROLL are not compatible */
 	if (result->rowMarks != NIL && (stmt->options & CURSOR_OPT_SCROLL))
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-		errmsg("DECLARE SCROLL CURSOR ... FOR UPDATE/SHARE is not supported"),
+ /*--
+   translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("DECLARE

Re: [HACKERS] new "row-level lock" error messages

2013-07-17 Thread Alvaro Herrera 
Peter Eisentraut wrote:

> In general, I find these new wordings to be a loss of clarity.  There is
> no indication on the SELECT man page or in the documentation index what
> a "row-level lock" is at all.
> 
> I would suggest that these changes be undone, except that the old
> "SELECT FOR ..." be replaced by a dynamic string that reverse-parses the
> LockingClause to provide the actual clause that was used.

Hmm, that's an idea.  If there are no objections, I'll get this fixed.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] new "row-level lock" error messages

2013-07-15 Thread Peter Eisentraut 
In 9af4159f in combination with cb9b66d3 a bunch of error messages were
changed from something like

"SELECT FOR UPDATE/SHARE is not allowed with UNION/INTERSECT/EXCEPT"

to

"row-level locks are not allowed with UNION/INTERSECT/EXCEPT"

because the intermediate state of

"SELECT FOR UPDATE/SHARE/KEY UPDATE/KEY SHARE is not allowed with
UNION/INTERSECT/EXCEPT"

was presumably considered too bulky.

I think that went too far in some cases.  For example, the new message

"row-level locks must specify unqualified relation names"

has little to do with its original meaning.

In general, I find these new wordings to be a loss of clarity.  There is
no indication on the SELECT man page or in the documentation index what
a "row-level lock" is at all.

I would suggest that these changes be undone, except that the old
"SELECT FOR ..." be replaced by a dynamic string that reverse-parses the
LockingClause to provide the actual clause that was used.




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