Обсуждение: updatable cursors and ORDER BY
The DECLARE reference page says: """ Another reason to use FOR UPDATE is that without it, a subsequent WHERE CURRENT OF might fail if the cursor query does not meet the SQL standard's rules for being “simply updatable” (in particular, the cursor must reference just one table and not use grouping or ORDER BY). Cursors that are not simply updatable might work, or might not, depending on plan choice details; so in the worst case, an application might work in testing and then fail in production. """ Indeed, grouping in cursors declared FOR UPDATE is rejected: DECLARE c CURSOR FOR SELECT f1,count(*) FROM uctest GROUP BY f1 FOR UPDATE; ERROR: FOR UPDATE is not allowed with GROUP BY clause But ORDER BY is allowed, contrary to what that note appears to say: DECLARE c CURSOR FOR SELECT f1, f2 FROM uctest ORDER BY f1 FOR UPDATE; -- no error, works fine Is this note outdated? A brief look into history of CheckSelectLocking() suggests that it might never have been correct. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The DECLARE reference page says:
> """
> Another reason to use FOR UPDATE is that without it, a subsequent WHERE
> CURRENT OF might fail if the cursor query does not meet the SQL
> standard's rules for being “simply updatable” (in particular, the cursor
> must reference just one table and not use grouping or ORDER BY). Cursors
> that are not simply updatable might work, or might not, depending on
> plan choice details; so in the worst case, an application might work in
> testing and then fail in production.
> """
> But ORDER BY is allowed, contrary to what that note appears to say:
> DECLARE c CURSOR FOR SELECT f1, f2 FROM uctest ORDER BY f1 FOR UPDATE;
> -- no error, works fine
I think you misread that note: it says nothing about what is allowed
in DECLARE CURSOR per se. It is talking about whether you can apply
UPDATE/DELETE WHERE CURRENT OF to that cursor. Moreover, what it says
is that if you use FOR UPDATE then such an UPDATE/DELETE *will* work,
whereas without it we don't guarantee that.
> Is this note outdated? A brief look into history of
> CheckSelectLocking() suggests that it might never have been correct.
The code that's relevant to this is in execCurrentOf(): see the bit about
* We have two different strategies depending on whether the cursor uses
* FOR UPDATE/SHARE or not. The reason for supporting both is that the
* FOR UPDATE code is able to identify a target table in many cases where
* the other code can't, while the non-FOR-UPDATE case allows use of WHERE
* CURRENT OF with an insensitive cursor.
regards, tom lane
The DECLARE reference page says:
"""
Another reason to use FOR UPDATE is that without it, a subsequent WHERE
CURRENT OF might fail if the cursor query does not meet the SQL
standard's rules for being “simply updatable” (in particular, the cursor
must reference just one table and not use grouping or ORDER BY). Cursors
that are not simply updatable might work, or might not, depending on
plan choice details; so in the worst case, an application might work in
testing and then fail in production.
"""
Indeed, grouping in cursors declared FOR UPDATE is rejected:
DECLARE c CURSOR FOR SELECT f1,count(*) FROM uctest GROUP BY f1 FOR UPDATE;
ERROR: FOR UPDATE is not allowed with GROUP BY clause
But ORDER BY is allowed, contrary to what that note appears to say:
DECLARE c CURSOR FOR SELECT f1, f2 FROM uctest ORDER BY f1 FOR UPDATE;
-- no error, works fine
Is this note outdated?
I'd say the note covers its scope of influence properly. Any query can be supplied to the cursor at declaration and it won't complain; since the problematic structure of the source query only comes into play for UPDATE and DELETE. If all the caller does is print the results it won't matter.
The error for GROUP BY is in the scope of the original select statement. If the FOR UPDATE was omitted the query would work but, as the note says, the subsequent UPDATE/DELETE would (likely?) fail.
A query with multiple tables would result in the same outcome - failure only if UPDATE/DELETE.
I'm having trouble imaging a failure mode for ORDER BY in this context...or how the other two options are plan-choice dependent.
I'm leaning toward the note needing to be made either more specific or more generic. This attempt to clarify implementation details only in part seems worse than just saying UPDATE/DELETE WHERE CURRENT OF requires a simply updatable query (though it is not strictly enforced) and that FOR UPDATE should be used unless the implications of the data behind the cursor are well understood in the specific situation.
I don't quite understand enough to know what the "fully accurate detail" version looks like and whether it is too much to include in the documentation.
David J.
On 5/9/18 22:57, Tom Lane wrote:
> I think you misread that note: it says nothing about what is allowed
> in DECLARE CURSOR per se. It is talking about whether you can apply
> UPDATE/DELETE WHERE CURRENT OF to that cursor. Moreover, what it says
> is that if you use FOR UPDATE then such an UPDATE/DELETE *will* work,
> whereas without it we don't guarantee that.
I think that last part isn't actually written down anywhere. (It only
states the converse.) How about a clarification like this:
@@ -271,7 +271,10 @@ <title id="sql-declare-notes-title">Notes</title>
and not use grouping or <literal>ORDER BY</literal>). Cursors
that are not simply updatable might work, or might not, depending on plan
choice details; so in the worst case, an application might work in testing
- and then fail in production.
+ and then fail in production. If <literal>FOR UPDATE</literal> is
+ specified, then the cursor is guaranteed to be updatable, or the
+ <command>DECLARE</command> command will error if an updatable cursor
+ cannot be created for the supplied query.
</para>
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I think that last part isn't actually written down anywhere. (It only
> states the converse.) How about a clarification like this:
> @@ -271,7 +271,10 @@ <title id="sql-declare-notes-title">Notes</title>
> and not use grouping or <literal>ORDER BY</literal>). Cursors
> that are not simply updatable might work, or might not, depending on plan
> choice details; so in the worst case, an application might work in testing
> - and then fail in production.
> + and then fail in production. If <literal>FOR UPDATE</literal> is
> + specified, then the cursor is guaranteed to be updatable, or the
> + <command>DECLARE</command> command will error if an updatable cursor
> + cannot be created for the supplied query.
> </para>
OK by me, except we don't usually use "error" as a verb. Either "fail"
or "throw an error" would read better IMO. Or you could just stop with
"guaranteed to be updatable"; I don't think the rest adds much.
regards, tom lane
On Thu, May 10, 2018 at 12:55:23PM -0400, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > I think that last part isn't actually written down anywhere. (It only > > states the converse.) How about a clarification like this: > > > @@ -271,7 +271,10 @@ <title id="sql-declare-notes-title">Notes</title> > > and not use grouping or <literal>ORDER BY</literal>). Cursors > > that are not simply updatable might work, or might not, depending on plan > > choice details; so in the worst case, an application might work in testing > > - and then fail in production. > > + and then fail in production. If <literal>FOR UPDATE</literal> is > > + specified, then the cursor is guaranteed to be updatable, or the > > + <command>DECLARE</command> command will error if an updatable cursor > > + cannot be created for the supplied query. > > </para> > > OK by me, except we don't usually use "error" as a verb. Either "fail" > or "throw an error" would read better IMO. Or you could just stop with > "guaranteed to be updatable"; I don't think the rest adds much. I have done as you suggested and just used the first part; patch attached and backpatched. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Вложения
On 5/28/18 13:17, Bruce Momjian wrote: > On Thu, May 10, 2018 at 12:55:23PM -0400, Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> I think that last part isn't actually written down anywhere. (It only >>> states the converse.) How about a clarification like this: >> >>> @@ -271,7 +271,10 @@ <title id="sql-declare-notes-title">Notes</title> >>> and not use grouping or <literal>ORDER BY</literal>). Cursors >>> that are not simply updatable might work, or might not, depending on plan >>> choice details; so in the worst case, an application might work in testing >>> - and then fail in production. >>> + and then fail in production. If <literal>FOR UPDATE</literal> is >>> + specified, then the cursor is guaranteed to be updatable, or the >>> + <command>DECLARE</command> command will error if an updatable cursor >>> + cannot be created for the supplied query. >>> </para> >> >> OK by me, except we don't usually use "error" as a verb. Either "fail" >> or "throw an error" would read better IMO. Or you could just stop with >> "guaranteed to be updatable"; I don't think the rest adds much. > > I have done as you suggested and just used the first part; patch > attached and backpatched. I think we should still add the second part, because it currently doesn't say anything about that a cursor declaration might fail if an updatable cursor cannot be created. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I think we should still add the second part, because it currently
> doesn't say anything about that a cursor declaration might fail if an
> updatable cursor cannot be created.
I still think it wouldn't add anything. Any other error in the command
would cause the cursor declaration to fail, too.
regards, tom lane