Обсуждение: new "row-level lock" error messages
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.
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Jul 23, 2013 at 2:16 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services