Derby
  1. Derby
  2. DERBY-31

Statement.setQueryTimeout() support.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC
    • Labels:
      None
    • Environment:
      ALL

      Description

      Calling Statement.setQueryTimeout() throws exception saying that function is not supported. This is an important JDBC feature and is limiting our options to use Derby with our JDBC code. Implementing this JDBC function would make Derby much easier to adopt.

      1. DERBY-31.svn.diff
        81 kB
        Oyvind Bakksjo
      2. DERBY-31.svn.status
        2 kB
        Oyvind Bakksjo
      3. derbyall_report.txt
        14 kB
        Oyvind Bakksjo

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Appears to be implemented now in embedded and network drivers.

          Show
          Rick Hillegas added a comment - Appears to be implemented now in embedded and network drivers.
          Hide
          Satheesh Bandaram added a comment -

          Is this issue ready to be RESOLVED and/or CLOSED? Oyvind seems to have done pretty much all activities that need to be done for this issue.

          Show
          Satheesh Bandaram added a comment - Is this issue ready to be RESOLVED and/or CLOSED? Oyvind seems to have done pretty much all activities that need to be done for this issue.
          Hide
          Oyvind Bakksjo added a comment -

          Updated documentation which stated setQueryTimeout was not implemented.

          Sending src/ref/rrefjdbc40794.dita
          Transmitting file data .
          Committed revision 344345.

          Show
          Oyvind Bakksjo added a comment - Updated documentation which stated setQueryTimeout was not implemented. Sending src/ref/rrefjdbc40794.dita Transmitting file data . Committed revision 344345.
          Hide
          Daniel John Debrunner added a comment -

          Øyvind's patch committed revision 208650. (into trunk)

          Show
          Daniel John Debrunner added a comment - Øyvind's patch committed revision 208650. (into trunk)
          Hide
          Oyvind Bakksjo added a comment -

          Attached patch addresses the following issues:

          • Throw exception on negative input to setQueryTimeout()
          • Test case for the above
          • Use milliseconds instead of seconds in the PreparedStatement interface and GenericPreparedStatement class
          • Exclude SetQueryTimeoutTest from DerbyNet and DerbyNetClient, since these environments don't yet support the new functionality (will create a fix for the DerbyClient driver later

          Derbyall has been run with two failures, report attached.
          StmtCloseFunTest - hard to tell if it's related to my changes, very little output from the test about what exactly failed.
          miscerrors - doesn't seem related

          Show
          Oyvind Bakksjo added a comment - Attached patch addresses the following issues: Throw exception on negative input to setQueryTimeout() Test case for the above Use milliseconds instead of seconds in the PreparedStatement interface and GenericPreparedStatement class Exclude SetQueryTimeoutTest from DerbyNet and DerbyNetClient, since these environments don't yet support the new functionality (will create a fix for the DerbyClient driver later Derbyall has been run with two failures, report attached. StmtCloseFunTest - hard to tell if it's related to my changes, very little output from the test about what exactly failed. miscerrors - doesn't seem related
          Hide
          Oyvind Bakksjo added a comment -

          Hi Dan, thanks for reviewing this patch so quickly.

          I am fully aware of and share you concern for the performance impact of creating all the TimerTask objects. It is far from ideal. The problem is that the Timer class forces recreation of TimerTask objects, since these can't be reused; when a TimerTask has run or been cancelled, it is pure waste. Trying to schedule the same task again will cause an exception. If it wasn't for this, we could associate a TimerTask with each StatementContext object.

          This performance impact will, however, only affect queries with a timeout value set. As such, it is a tradeoff - you can get timeouts, but that will slow down your execution.

          If it turns out that the calls to checkCancellationFlag() seriously affect performance, these calls could be decimated by doing the call only for each n-th row. (It sure will be nice to get that performance regression test, so we'll see the actual impact.) For the time being, I can do some rudimentary testing of this myself.

          I agree a separate module for the timer might be overkill. As for the interface, my idea was that more methods than getCancellationTimer() could be added as needs arose for Timers for other purposes. It would be up to the implementing class whether to actually return the same Timer object. I guess it would depend on the duration of the TimerTasks and the responsiveness requirements whether to use multiple Timers or just a single one.

          I meant to support milliseconds use internally and seconds at the JDBC API level. I guess it would be more consistent if I used milliseconds in the language PreparedStatement interface as well. And yes, I'll fix throwing an exception on negative timeout values.

          Show
          Oyvind Bakksjo added a comment - Hi Dan, thanks for reviewing this patch so quickly. I am fully aware of and share you concern for the performance impact of creating all the TimerTask objects. It is far from ideal. The problem is that the Timer class forces recreation of TimerTask objects, since these can't be reused; when a TimerTask has run or been cancelled, it is pure waste. Trying to schedule the same task again will cause an exception. If it wasn't for this, we could associate a TimerTask with each StatementContext object. This performance impact will, however, only affect queries with a timeout value set. As such, it is a tradeoff - you can get timeouts, but that will slow down your execution. If it turns out that the calls to checkCancellationFlag() seriously affect performance, these calls could be decimated by doing the call only for each n-th row. (It sure will be nice to get that performance regression test, so we'll see the actual impact.) For the time being, I can do some rudimentary testing of this myself. I agree a separate module for the timer might be overkill. As for the interface, my idea was that more methods than getCancellationTimer() could be added as needs arose for Timers for other purposes. It would be up to the implementing class whether to actually return the same Timer object. I guess it would depend on the duration of the TimerTasks and the responsiveness requirements whether to use multiple Timers or just a single one. I meant to support milliseconds use internally and seconds at the JDBC API level. I guess it would be more consistent if I used milliseconds in the language PreparedStatement interface as well. And yes, I'll fix throwing an exception on negative timeout values.
          Hide
          Daniel John Debrunner added a comment -

          Some general comments on the patch

          • giving the patch file a specific name (rather than svn-diff) helps out the committers (and anyone else) when the patch is downloaded
          • try to resist the temptation to clean up the code (e.g. your javadoc fixes), it makes it much hard to review the patch and see the real changes. If you want to do such cleanup that's great, by a separate patch is much better.

          Specific comments

          • the patch needs to be re-submitted, it doesn't apply for me on messages_en.properties and SQLState,
            just update your codeline and see if a merge is needed.

          In general I believe the functionality is correct and clearly implemented but I have a few minor concerns and one major one.

          The major one is the performance impact of this fix . Every time StatementContext.setInUse() is called a new CancelQueryTask object will be created, that will be very expensive for queries, imagine a ResultSet of one milllion rows, that's at least one million short lived objects being created and going through gc. Derby tries to limit object creation during execution to be not be a function of the number of rows, except where (indirectly) mandated by the JDBC spec. The impact of the checkCancellationFlag() calls is also of concern.

          • I wonder if a module is really needed for the time task, a simpler approach would be just to have the Monitor return the TimerTask directly. The TimerFactory interface doesn't add any value over TimerTask itself, and its one method ie badly named. I don't see any reason this timer should be limited to cancellation events.
          • The code seems to vary a bit about choosing to use milli-seconds or seconds, it has to be seconds at the JDBC level, but then internally you use milli-seconds and then back to seconds in the langauage PreparedStatement interface. I would stick to seconds since that is the granuality at the api level and only convert to ms at the TimerTask level. You might consider using the constant 0L if you are passing zero into a long method argument, means the method resolution remains the same if ever the method is overloaded with an int argument.
          • For the JDBC Statement.setQueryTimeout() Derby traditionally checks that the input value is valid, thus I think an exception should be thrown for a negative timeout. (and that's the specified JDBC behaviour)

          In spite of those comments (I'm very picky) I believe this is a good patch, especially for your first. In the open source world it's probably a 50-50 call as to if the patch could be committed as-is and these items subsequently addressed or they need to be addressed first. Some folks on the list may be concerned about the performance impact of this. I would like to see the functionality committed but the performance addressed before this becomes part of any release.
          [but the patch can only be committed there was a newer version that applied cleanly]

          It's actually great to have a concrete implementation because the issues with it give me some ideas on how it possibly could be done without such a performance impact, but that will have to be a later e-mail. I need to run

          Show
          Daniel John Debrunner added a comment - Some general comments on the patch giving the patch file a specific name (rather than svn-diff) helps out the committers (and anyone else) when the patch is downloaded try to resist the temptation to clean up the code (e.g. your javadoc fixes), it makes it much hard to review the patch and see the real changes. If you want to do such cleanup that's great, by a separate patch is much better. Specific comments the patch needs to be re-submitted, it doesn't apply for me on messages_en.properties and SQLState, just update your codeline and see if a merge is needed. In general I believe the functionality is correct and clearly implemented but I have a few minor concerns and one major one. The major one is the performance impact of this fix . Every time StatementContext.setInUse() is called a new CancelQueryTask object will be created, that will be very expensive for queries, imagine a ResultSet of one milllion rows, that's at least one million short lived objects being created and going through gc. Derby tries to limit object creation during execution to be not be a function of the number of rows, except where (indirectly) mandated by the JDBC spec. The impact of the checkCancellationFlag() calls is also of concern. I wonder if a module is really needed for the time task, a simpler approach would be just to have the Monitor return the TimerTask directly. The TimerFactory interface doesn't add any value over TimerTask itself, and its one method ie badly named. I don't see any reason this timer should be limited to cancellation events. The code seems to vary a bit about choosing to use milli-seconds or seconds, it has to be seconds at the JDBC level, but then internally you use milli-seconds and then back to seconds in the langauage PreparedStatement interface. I would stick to seconds since that is the granuality at the api level and only convert to ms at the TimerTask level. You might consider using the constant 0L if you are passing zero into a long method argument, means the method resolution remains the same if ever the method is overloaded with an int argument. For the JDBC Statement.setQueryTimeout() Derby traditionally checks that the input value is valid, thus I think an exception should be thrown for a negative timeout. (and that's the specified JDBC behaviour) In spite of those comments (I'm very picky) I believe this is a good patch, especially for your first. In the open source world it's probably a 50-50 call as to if the patch could be committed as-is and these items subsequently addressed or they need to be addressed first. Some folks on the list may be concerned about the performance impact of this. I would like to see the functionality committed but the performance addressed before this becomes part of any release. [but the patch can only be committed there was a newer version that applied cleanly] It's actually great to have a concrete implementation because the issues with it give me some ideas on how it possibly could be done without such a performance impact, but that will have to be a later e-mail. I need to run
          Hide
          Oyvind Bakksjo added a comment -

          Here is an implementation of Statement.setQueryTimeout.

          The patch includes a new functional test in the jdbcapi testsuite.

          It is my first Derby patch, so I ask any reviewer; please, use your magnifying glass and be critical.

          May I suggest that Dan looks at it, since he has given me the most feedback on this issue? (I don't write this to exclude other volunteers, I just think that Mr. "Someone" already has enough on his todo list, given all the requests of the kind "Can someone review this patch?". ;o)

          Notes/thoughts/issues:

          • Perhaps other ResultSet classes than [Bulk]TableScanResultSet should check for cancellation, but I think these are the most important ones to begin with. I will look into this later.
          • Currently, no kind of substatement execution has any timeout value set. This includes update operations on updatable resultsets, triggers, alter table operations etc.
          • To a certain extent, I have cleaned up javadoc warnings in the files touched by me, since they made it difficult for me to see whether I had introduced any new javadoc warnings myself.

          I'm running derbyall and will attach the results once it's finished.

          Show
          Oyvind Bakksjo added a comment - Here is an implementation of Statement.setQueryTimeout. The patch includes a new functional test in the jdbcapi testsuite. It is my first Derby patch, so I ask any reviewer; please, use your magnifying glass and be critical. May I suggest that Dan looks at it, since he has given me the most feedback on this issue? (I don't write this to exclude other volunteers, I just think that Mr. "Someone" already has enough on his todo list, given all the requests of the kind "Can someone review this patch?". ;o) Notes/thoughts/issues: Perhaps other ResultSet classes than [Bulk] TableScanResultSet should check for cancellation, but I think these are the most important ones to begin with. I will look into this later. Currently, no kind of substatement execution has any timeout value set. This includes update operations on updatable resultsets, triggers, alter table operations etc. To a certain extent, I have cleaned up javadoc warnings in the files touched by me, since they made it difficult for me to see whether I had introduced any new javadoc warnings myself. I'm running derbyall and will attach the results once it's finished.
          Hide
          Oyvind Bakksjo added a comment -

          Output of the 'svn diff' command.

          Show
          Oyvind Bakksjo added a comment - Output of the 'svn diff' command.
          Hide
          Oyvind Bakksjo added a comment -

          Output of the 'svn status' command.

          Show
          Oyvind Bakksjo added a comment - Output of the 'svn status' command.
          Hide
          Shreyas Kaushik added a comment -

          This is the implementation of the Query Timer that triggers the statement cancelling on time out.

          Show
          Shreyas Kaushik added a comment - This is the implementation of the Query Timer that triggers the statement cancelling on time out.
          Hide
          Shreyas Kaushik added a comment -

          This is the patch for implementing setQueryTimeout() in EmbedStatement.java . I clsoe the activation for the statement when the query times out so that the query stops executing. I tested this by inserting data into a table continuosly for about 12 hours and then doing a select * on it by setting the time out to 1 sec. It worked fine by cancelling the statement execution when the timeout happened.

          Show
          Shreyas Kaushik added a comment - This is the patch for implementing setQueryTimeout() in EmbedStatement.java . I clsoe the activation for the statement when the query times out so that the query stops executing. I tested this by inserting data into a table continuosly for about 12 hours and then doing a select * on it by setting the time out to 1 sec. It worked fine by cancelling the statement execution when the timeout happened.
          Hide
          Ali Demir added a comment -

          When a time out is set to n millis, and if the query takes n+m millis, then caller would expect that time out occurs around n millis and not wait until n+m. Also, when the timeout occurs, the processing of the query should have been stopped and there should not be any locks held after timeout. Therefore, a simple solution that simply checks how long the query processing is taking is not enough. The internals of the system need to know that it should stop the query and release any locks held by it when the timeout occurs.

          Show
          Ali Demir added a comment - When a time out is set to n millis, and if the query takes n+m millis, then caller would expect that time out occurs around n millis and not wait until n+m. Also, when the timeout occurs, the processing of the query should have been stopped and there should not be any locks held after timeout. Therefore, a simple solution that simply checks how long the query processing is taking is not enough. The internals of the system need to know that it should stop the query and release any locks held by it when the timeout occurs.
          Hide
          Amit Handa added a comment -

          Just to correct what I have commenetd earlier,
          The solution can be from the time a query is executeXXX is called till the time
          executeXXX fetches a ResultSet handle. If this time is greater than the time set,
          throw SQLException

          Show
          Amit Handa added a comment - Just to correct what I have commenetd earlier, The solution can be from the time a query is executeXXX is called till the time executeXXX fetches a ResultSet handle. If this time is greater than the time set, throw SQLException
          Hide
          Amit Handa added a comment -

          One solution can be to check for time(set a flag) when setQueryTimeout() is set.
          Every time any executeXXX() method is called, check whether query timed out
          with differrence from the present time and the flag set earlier.
          If time out throw SQLException else do nothing

          Show
          Amit Handa added a comment - One solution can be to check for time(set a flag) when setQueryTimeout() is set. Every time any executeXXX() method is called, check whether query timed out with differrence from the present time and the flag set earlier. If time out throw SQLException else do nothing
          Hide
          Daniel John Debrunner added a comment -

          Change to new feature and JDBC component

          Show
          Daniel John Debrunner added a comment - Change to new feature and JDBC component

            People

            • Assignee:
              Oyvind Bakksjo
              Reporter:
              Ali Demir
            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development