|
Daniel John Debrunner made changes - 31/Aug/07 11:41 PM
Daniel John Debrunner made changes - 31/Aug/07 11:41 PM
Mamta A. Satoor made changes - 06/Dec/07 07:44 PM
I committed a change(601830) into trunk with following commit comments
DERBY3037 This commit makes sure that EmbeddedResultSet.close() calls Language Resultset.close rather than Language Resultset.finish. This change caused few tests to fail. The tests had runtime statistics on but the code to dump the runtime statistics was in Language Resutlset.finish and not in Language Resultset.close. To fix this, I have moved the code to dump runtime statistics from Language Resutlset.finish into Lanugage ResultSet.close This has fixed the test failures. As for the 2nd part of this jira entry which is to have activation.close() call Language Resultset.finish(). This already happens in impl.sql.execute.BaseActivation.close() and hence no work was needed for the 2nd part. Will merge this change into 10.3 codeline soon.
Merged changes from trunk into 10.3 codeline with revision 602198.
Mamta A. Satoor made changes - 07/Dec/07 07:50 PM
Daniel John Debrunner made changes - 08/Dec/07 01:26 AM
Other than Activation.close(), there are 3 other places in the code, which call Language Resultset.finish(). Those cases are listed below
1)impl.jdbc..EmbedResultSet.deleteRow() line 3789 Here we generate internal SQL to implement deleteRow. The internal sql is DELETE FROM ..... WHERE CURRENT OF ........ After the execution of this sql, we close the Language ResultSet, then finish the Language ResultSet and then we close the Activation associated with that Language ResultSet. Theoretically, we can simply reply on Activation.close to do the jobs of closing the Language ResultSet, finishing the Language ResultSet and then closing itself. I will give this a try once I address the failure in outerjoin.sql caused by my earlier checkin for this jira entry. 2)impl.sql.execute.AlterTableConstantAction.executeUpdate() line 2242 The current code here looks like following PreparedStatement ps = lcc.prepareInternalStatement(updateStmt); // This is a substatement; for now, we do not set any timeout // for it. We might change this behaviour later, by linking // timeout to its parent statement's timeout settings. ResultSet rs = ps.execute(lcc, true, 0L); rs.close(); rs.finish(); As can be seen from code above, here too we execute an internal SQL statement and after it's execution, we call close the Language ResultSet, then finish the Language ResultSet. We do not currently get the handle to Activation object in the code above. I can try to do following to avoid direct call to Language Resultset.finish PreparedStatement ps = lcc.prepareInternalStatement(updateStmt); Activation act = ps.getActivation(lcc, false); // This is a substatement; for now, we do not set any timeout // for it. We might change this behaviour later, by linking // timeout to its parent statement's timeout settings. ResultSet rs = ps.execute(lcc, true, 0L); act.close() 3)And lastly, impl.jdbc.EmbedStatement.executeStatement line 1276 Here, for sql statements that do not return rows, we call Language Resultset.finish because we do not need the Language Resultset anymore(as per the code in the comments). In this particular case, I do not think I can simply call activation.close to finish the Language Resultset because activation is still getting used after the Language Resultset has been finished. I would like to know if I am interpreting this incorrectly. Would appreciate feedback on these 3 items, especially item 3. I will start working on 1 and 2 after I resolve the problem with outerjoin.sql On 2) calling ps.getActivation(lcc, false); and then closing it would be incorrect. That would create a new activation unrelated to the subsequent execute() call.
The execute() call used here creates a single use activation (as indicated in its javadoc). A single use activation will be closed once its language ResultSet is closed (not well documented), and in fact may need cleanup since the activation.close() is called from finishAndRts() and not the result set closing. on 3) I think the comments are incorrect at line (1276) which is the justification for this bug.
I addressed the item 1 above and fixed the failure in outjoin.sql with revision 603823 in trunk. The commit comments were as follows
The outerjoin.sql was failing because the part of the runtime statistcis info was getting erased before LanguageResultSet.close() code collected it all. I moved the erasing of runtime stat code so that it happened once the stat was collected successfully. In addition, I removed redundant code of closing and finishing the LanguageResultSet from EmbedResultSet.java because these steps happen as part of activation.close I will merge this into 10.3 codeline and fire the tests there.
Fixed item 3) from the list above in the trunk using 604976. The commit comments were as follows (will merge these changes into 10.3 after the tests finish successfully there)
DERBY-3037 EmbedStatement.executeStatement at line 1276 was calling finish rather than close on the Language Resultset. I fixed that to make a call to close. In addition, I also had to move the code for collecting the stats from finish to close method in NoRowsResultSetImpl.
Meged change 605976 into 10.3.2.2 codeline with revision 605219.
With this patch(Derby_3037_AlterTableConstantActionChanges_v1_diff.txt), I am removing the Language Resultset.finish from AlterTableConstantAction. In addition, like Dan mentioned, what's being created in this part of AlterTableConstantAction is a single use activation which should be closed when its language Resultset is closed. In order to achieve that, I have added following code in NoRowsResultSetImpl.close to take care of the activation
+ if (activation.isSingleExecution()) + activation.close(); The derbyall and junit tests have run with no problems. I will go ahead and check this patch by tomorrow. Any feedback? Next task can be to move close of single use activation from BasicNoPutResultSet.finishAndRTS(line 607) into NoPutResultSetImpl.close. In other words, if Language Resultset associated with single use activation is closed, we should close the activation too. Would like to know if anyone has any feedback on this.
Mamta A. Satoor made changes - 19/Dec/07 09:25 PM
Mamta A. Satoor made changes - 20/Dec/07 01:14 AM
Commited the patch Derby_3037_AlterTableConstantActionChanges_v1_diff.txt with revision 606106 into trunk. Will merge into 10.3 later.
Merged change 606106 from trunk into 10.3.2.2 using revision 606924.
Removing 'patch available' since the issue only has one patch which appears to have been committed (and merged to 10.3).
Dyre Tjeldvoll made changes - 04/Jan/08 10:05 AM
I have a very small patch for the last item left on DERBY-3037 which is to move close of a single use activation from BasicNoPutResultSet.finishRTS into NoPutResultSetImpl.close. The patch is attached as DERBY3037_patch_not_ready_for_commit_v2_diff.txt but I have included the diff here to explain the failure caused by the patch
Index: java/engine/org/apache/derby/impl/sql/execute/BasicNoPutResultSetImpl.java =================================================================== --- java/engine/org/apache/derby/impl/sql/execute/BasicNoPutResultSetImpl.java(revision 613756) +++ java/engine/org/apache/derby/impl/sql/execute/BasicNoPutResultSetImpl.java(working copy) @@ -603,9 +603,6 @@ close(); finished = true; - - if (isTopResultSet && activation.isSingleExecution()) - activation.close(); } } Index: java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java =================================================================== --- java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java(revision 613756) +++ java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java(working copy) @@ -183,6 +183,9 @@ isOpen = false; + if (isTopResultSet && activation.isSingleExecution()) + activation.close(); + } /** @see NoPutResultSet#setTargetResultSet */ $ This change in code causes a failure with lang/nestedCommit.sql. The specific test case inside nestedCommit.sql has to do with a call to a Java Stored routine (specifically, a function) which looks as follows public static int doConnCommitInt() throws Throwable { Connection conn = DriverManager.getConnection("jdbc:default:connection"); conn.commit(); return 1; } And the stack strace in derby.log for nestedCommit.sql looks as follows 2008-01-21 21:52:49.328 GMT Thread[main,5,main] (XID = 157), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Failed Statement is: -- call doConnStmt('call doConnStmt(''call doConnStmt(''''commit'''')'')'); values doConnCommitInt() org.apache.derby.shared.common.sanity.AssertFailure: ASSERT FAILED closed at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityManager.java:120) at org.apache.derby.impl.sql.execute.BaseActivation.setCurrentRow(BaseActivation.java:1276) at org.apache.derby.impl.sql.execute.NoPutResultSetImpl.setCurrentRow(NoPutResultSetImpl.java:316) at org.apache.derby.impl.sql.execute.RowResultSet.getNextRowCore(RowResultSet.java:156) at org.apache.derby.impl.sql.execute.BasicNoPutResultSetImpl.getNextRow(BasicNoPutResultSetImpl.java:460) at org.apache.derby.impl.jdbc.EmbedResultSet.movePosition(EmbedResultSet.java:425) at org.apache.derby.impl.jdbc.EmbedResultSet.next(EmbedResultSet.java:369) at org.apache.derby.tools.JDBCDisplayUtil.indent_DisplayResults(JDBCDisplayUtil.java:382) at org.apache.derby.tools.JDBCDisplayUtil.indent_DisplayResults(JDBCDisplayUtil.java:338) at org.apache.derby.tools.JDBCDisplayUtil.indent_DisplayResults(JDBCDisplayUtil.java:241) at org.apache.derby.tools.JDBCDisplayUtil.DisplayResults(JDBCDisplayUtil.java:229) at org.apache.derby.impl.tools.ij.utilMain.displayResult(utilMain.java:435) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:509) at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:350) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:248) at org.apache.derby.impl.tools.ij.Main.go(Main.java:215) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:181) at org.apache.derby.impl.tools.ij.Main.main(Main.java:73) at org.apache.derby.tools.ij.main(ij.java:59) The reason for the stack failure is that the Language ResultSet associated with the function call has been closed by the new code in NoPutResultSetImpl.close which was invoked by the commit inside the user function doConnCommitInt. During commit processing, we need to somehow distinguish the fact that the Language ResultSet is still being constructed and used and hence should not be closed. I would appreciate if anyone has any ideas on how to achieve this.
Mamta A. Satoor made changes - 22/Jan/08 05:28 PM
Here is a standalone reproducible test case that shows closure of resultset when code changes of the patch DERBY3037_patch_not_ready_for_commit_v2_diff.txt are applied.
Mamta A. Satoor made changes - 22/Jan/08 06:31 PM
This is a similar issue to
It seems that the commit() needs to be smarter about which language result sets to close. It's not just the current one that's the issue, actually any that are actively in use. E.g. If a procedure P1 calls procedure P2 which calls procedure P3 and P3 commits, then current all the language result sets will be closed (ignoring held result sets), but the language result sets (CallStatementResultSets) for P1, P2 and P3 should remain open. For functions it's a similar problem, though it's complicated by that fact that a function is called from a language result set that returns rows, whereas a procedure is not. So with a SQL select executed through a JDBC statement with close result sets on commit: SELECT f(a) FROM T if f(a) commits then is the JDBC ResultSet that is processing the query closed, meaning that its next next() call will thrown an exception? Two thoughts:
- What is the purpose of commit() closing the language result sets? If it's to close JDBC ResultSets that are marked close at commit then a possibility is to only close language result sets that return rows (returnRows() returns true). That would leave language result sets that do not return rows open, but by definition (I think) those are the ones that are actively executing and the very ones we don't want to close. :-) - Should commit() be allowed in all routines? I can see a commit makes sense in a procedure, to allow a section of logic to be encapsulated in a procedure with transaction semantics. But a commit() in a function seems dubious, what should happen when a function performs a commit (or rollback) in the middle of a SELECT statement or even worse a DML statement like an INSERT? I will explore the first suggestion of closing only language result sets that return rows and see how that goes.
As for the 2nd thought, I am not sure if SQL spec has anything to say here ie should commit be allowed in a function? I spent some time on Dan's suggestion
*********************** What is the purpose of commit() closing the language result sets? If it's to close JDBC ResultSets that are marked close at commit then a possibility is to only close language result sets that return rows (returnRows() returns true). That would leave language result sets that do not return rows open, but by definition (I think) those are the ones that are actively executing and the very ones we don't want to close. :-) *********************** While debugging through the commit code path, I found that the current check for what languages resultset should be closed is the exact opposite of what we are thinking of trying. I have included the code path stack trace below Thread [main] (Suspended) ac601a400fx0117xafb0x415ax00000045b6980(BaseActivation).reset() line: 337 GenericLanguageConnectionContext.resetActivations(boolean) line: 2748 GenericLanguageConnectionContext.doCommit(boolean, boolean, int, boolean) line: 1125 GenericLanguageConnectionContext.userCommit() line: 1003 TransactionResourceImpl.commit() line: 237 EmbedConnection40(EmbedConnection).commit() line: 1288 DERBY_3304_Repro.doConnCommitInt() line: 137 ac601a400fx0117xafb0x415ax00000045b6980.e0() line: not available DirectCall.invoke(Object) line: 139 RowResultSet.getNextRowCore() line: 148 RowResultSet(BasicNoPutResultSetImpl).getNextRow() line: 460 EmbedResultSet40(EmbedResultSet).movePosition(int, int, String) line: 425 EmbedResultSet40(EmbedResultSet).next() line: 369 DERBY_3304_Repro.doSingleDriver() line: 71 DERBY_3304_Repro.main(String[]) line: 104 Here, the Java stored procedure is issuing a commit and the code path for the commit is as shown above. In the reset() method in BaseActivation (that is where the above stack trace is), we decide what resultsets should be closed based on following logic // if resultset holdability after commit is false, close it if (resultSet != null) { if (!resultSetHoldability || !resultSet.returnsRows()) { // would really like to check if it is open, // this is as close as we can approximate that. resultSet.close(); } else if (resultSet.returnsRows()) { resultSet.clearCurrentRow(); } } So, if the result set holdability is false, we close the language resultset whether it returns rows or not, which sounds correct. If the result set holdability is true, we close the resultset if it does not return rows. But for resultsets that do return rows and their holdablity is true, we simply clear the current row. After doing a little more research, it appears that Derby code is already closing the resultsets that return rows if the holdability of the resultset is false. This is done in BaseActivation.reset method. Part of the code in the reset() method looks as follows
if (!resultSetHoldability || !resultSet.returnsRows()) { // would really like to check if it is open, // this is as close as we can approximate that. resultSet.close(); The first part of the if statement above, which is !resultSetHoldability, will ensure that we will close all the resultsets(including resultsets that return rows) that have their holdability set to false. In the test case attached to this jira(DERBY_3304_Repro.java), we set the holdability of the JDBC Connection object to false and then create a Java stored function doConnCommitInt(this function, doConnCommitInt, has a Connection.commit() inside it.) Next, we execute "values doConnCommitInt()". Since the holdability of the Connection object is false, the language resultset that gets created for values doConnCommitInt() also has it's holdability set to false. Next, when the doConnCommitInt() function does a Connection.commit, we come to BaseActivation.reset() and the blanket check for !resultSetHoldability causes Derby to close the language resultset associated with values doConnCommitInt() and that behavior is incorrect. So, the problem to solve here is to have additional rule for qualifying the language resultsets for closure. > So, the problem to solve here is to have additional rule for qualifying the language resultsets for closure.
but maybe first clarify what it means to support a commit() [or rollback] in a function call? I will look into SQL spec to see what it says there.
Found interesting info in Section 13.1<SQL-client module definition> General Rules of SQL 2003 specification
4) After the last time that an SQL-agent performs a call of an <externally-invoked procedure>: a) A <rollback statement> or a <commit statement> is effectively executed. If an unrecoverable error has occurred, or if the SQL-agent terminated unexpectedly, or if any constraint is not satisfied, then a <rollback statement> is performed. Otherwise, the choice of which of these SQL-statements to perform is implementation-dependent. If the implementation choice is <commit statement>, then all holdable cursors are first closed. The determination of whether an SQL-agent has terminated unexpectedly is implementation-dependent. b) For every SQL descriptor area that is currently allocated within an SQL-session associated with the SQL-agent, let D be the <descriptor name> of that SQL descriptor area; a <deallocate descriptor statement> that specifies DEALLOCATE DESCRIPTOR D is effectively executed. c) All SQL-sessions associated with the SQL-agent are terminated. I am puzzled by the blurb in 4a)"If the implementation choice is <commit statement>, then all holdable cursors are first closed. " That seems to contradict the fact that holdable cursors by definition should be held over the commit. Also, I am probably misinterpreting 4c)"All SQL-sessions associated with the SQL-agent are terminated." but does it mean that we close the resultset associated with call to <externally-invoked procedure>: that is what my patch is doing. I don't think section 13.1 applies to Derby's functions or procedures.
Derby's functions & procedures are SQL-Invoked routines, their schema definition is defined by 11.50, specifically SQL-invoked function and SQL-invoked procedure, and execution by 10.4. Derby's functions and procedures are external routines (ie. not implemented in SQL), but they are not externally invoked routines. The behaviour for SQL-invoked routines that are external and implemented in Java is in part 13 of the SQL spec which for routines is written as a series of modifications to section 11.50 and 10.4 (and probably other sections). I did further investigation into SQL specs and following is what seems to apply for what Derby supports which is SQL-invoked routines which are external routines written in Java.
SQL foundation spec section 10.4<routine invocation> GR 8)f)ii)6)B) says "If, before the completion of the execution of P, an attempt is made to execute an SQLtransaction statement that is not <savepoint statement> or <release savepoint statement>, or is a <rollback statement> that does not specify a <savepoint clause>, then an exception condition is raised: external routine exception — prohibited SQL-statement attempted." The P above is the program identified by the external name of R, where R is in an external routine. The Part 13 of the SQL spec which is specific to behavior of SQL-invoked routines which are external and written in Java does not include any modification to the general rule above. (The place to check in Part 13 would be Section 8.3 <routine invocation> Page 34 and couple pages after that.) Based on these 2 specifications, Derby is not following SQL specification by allowing commit and rollbacks inside SQL-invoked functions and SQL-invoked procedures. Other databases including Oracle, DB2, Sybase support commit and rollback inside SQL-invoked procedures so eventhough it is not a SQL standard, it appears to be a de-facto industry standard. It allows the users to finish one unit of task completely inside a stand along SQL-invoked procedures and since procedures do not directly return resultsets, supporting commit and rollback inside them do not cause a problem. But that is not true for SQL-invoked functions. A SQL-invoked function for instance can be called from a SELECT statement and SELECT statement has resultset associated with it. If the SQL-invoked function does a commit inside it, what should happen to the resultset associated with SELECT statement if the resultset set is created with holdability false? Because of this, I do not think Derby should support commit and rollback inside of a SQL-invoked function. I will go ahead and enter a Jira entry for that. I think we will need to reach some kind of resolution for that jira enty before the patch attached to this jira entry (DERBY3037_patch_not_ready_for_commit_v2_diff.txt) can be committed. This patch exposes the vulnerability of Derby explained in this paragraph through lang/nestedCommit.sql. Please let me know if anyone has any questions/comments regarding this. I will work on opening a jira entry for commit/rollback inside SQL-invoked functions.
Andrew McIntyre made changes - 07/May/08 06:12 AM
Triaged for 10.5.2: Assigned normal urgency, noted that repro is available.
Rick Hillegas made changes - 06/Jul/09 05:58 PM
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Reopened
Major
Language ResultSet.finish() is called even when the ResultSet is going to be re-used.
$ svn diff
Index: java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
===================================================================
--- java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java (revision 599587)
+++ java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java (working copy)
@@ -567,7 +567,8 @@
try {
try {
- theResults.finish(); // release the result set, don't just close it
+// theResults.finish(); // release the result set, don't just close it
+ theResults.close();
if (this.singleUseActivation != null)
{