Performance can be improved by re-using language ResultSets across Activation executions.

• Key: DERBY-827
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Dyre Tjeldvoll
• Reporter: Daniel John Debrunner
• Fix Version/s: 10.3.1.4

Description

>Shouldn't DistinctScalarAggregateRS implement a close or a finish method
>>(not sure what the difference is) and close the scan controller there.


The close() and finish() methods are actually explained in their javadoc
in the language org.apache.derby.iapi.sql.ResultSet class.
[note this is not a JDBC java.sql.ResultSet object]

close() - Tells the system that there will be no more calls to
getNextRow() (until the next open() call)

finish() - Tells the system that there will be no more access to any
database information via this result set

So close means the ResultSet may be opened again for more access, while
finish means it will not be used again.

However, their use in the code always doesn't match that, and that does
cause confusion, at least to me.

Language ResultSets (not JDBC ones) can be and are opened multiple
times, for example when scanning a table multiple times within a join.

An Activation, which represents the internal state of
java.sql.PreparedStatement object & has the lifetime of the
java.sql.PreparedStatement, contains a top-level language ResultSet.
This top-level language ResultSet provides the execution of the SQL
statement, DML, DDL or a query. The top-level ResultSet may contain
other ResultSets and could be seen as a tree structure. For the simple
case of a primary key lookup query like:

   select name from customer where id = ?

The activation would contain this:

top result set
ProjectRestrictRS << IndexRowToBaseRowRS << TableScanRS

Now for some reason, even though the api of ResultSet say they can be
re-used, and in some cases they are, this result set tree is thrown away
after each execution. That is, the top result set has its finish()
method called and then the activation removes its reference to it. Then
on the next execution a new (identical) tree is set up.

There is potential for a huge performance gain if this top level result
set and its tree are re-used and have the same lifetime as the
Activation. The saving comes in two forms, not having to create many
objects on each execution, and not creating short-lived objects for the
garbage collector to handle.

I made a simple fix, it's a couple of lines of code, just calling close
& finish at the correct times, and for the above simple primary key
lookup query, the performance went from 17,300 to 24,000 selects per
second (cached data, single user). I'll post a patch shortly as an
indication of the direction, once I can separate it from other changes
in my client.

However, I'm running the Derby tests and there are some (maybe 25-30)
failures, I think because not all the language ResultSet implementations
are correctly written to be re-opened. Interestingly, the first failure
I saw was in an aggregrate test, which goes back to the issue Manish saw.

Even if derbyall passed I would be nervous about submitting this patch
for real, because I don't think there's a lot of testing using repeat
executions of PreparedStatements in the tests. The ij tests mainly use
Statement, this is a single use of an activation so this change would
not affect them. Thus such a patch could regress Derby by making it more
likely existing bugs would be exposed.

Given the performance gains, I think we need to start re-using
ResultSets from Activation, and devise a way to ensure the testing
covers the re-use. The main issue is there is a large number of
ResultSet implementations to cover.

Comments

[Daniel John Debrunner]

Patch that re-uses language ResultSets across activation executions.
This is a draft patch to enable further testing only, around 25-30 tests fail with this patch.
Some possibly due to incorrect close logic in ResultSet implementations,
Some probably due to changes in runtime statistics output for items like the execution count.

M java\engine\org\apache\derby\impl\sql\execute\NoPutResultSetImpl.java
M java\engine\org\apache\derby\impl\sql\execute\BaseActivation.java
M java\engine\org\apache\derby\impl\sql\GenericActivationHolder.java
M java\engine\org\apache\derby\impl\jdbc\EmbedResultSet.java
M java\engine\org\apache\derby\impl\jdbc\EmbedStatement.java
M java\engine\org\apache\derby\iapi\sql\Activation.java

[Daniel John Debrunner]

Updated version of the patch, accounting for the fact some of the changes were made under DERBY-1142. This is still an example patch, much more work is needed to ensure ResultSet implementations work as expected and can safely be re-used. Plus more testing of repeated use of PreparedStatements.

[Dyre Tjeldvoll]

I've attached a preliminary (hence no ASF license) patch that creates a new junit test that attempts to fill the mentioned gap in the existing test suites. The new test instantiates all ResultSets returned from GenericResultSetFactory from a PreparedStatement that is executed repeatedly.

At this point all ResultSets except InsertVTIResultSet, DeleteVTIResultSet, UpdateVTIResultSet and MaterializedResultSet are created. According to the EMMA reports these ResultSets are never created when running derbyall or the junit tests, so I don't know how to write a test case that does.

The preliminary version of this patch adds a println to the ResultSetFactory which shows the ResultSets that are instantiated and the Statement ID and query that did so. This makes it easier to see if the test does what it is supposed to, but would obviously have to be removed in a final version of the patch.

Reviews/comments would be much appreciated. Thanks.

[Dyre Tjeldvoll]

I tried to run the new test with latest version of Dan's patch, but only one test case, testLastIndexKeyResultSet fails. When I think about it I think it could be because this test case is one of few where the underlying table/data is changed between each execution of the PreparedStatement.

[Daniel John Debrunner]

Thanks for working on this. I would recommend not using the ASF licence flag as an indicator for if the patch is ready to be used or not. I think a simple comment with the attachment is fine.
By not clicking the ASF licence flag you stop others from working on the patch, thus removing the chance for community development.

[Dyre Tjeldvoll]

Attaching a modified test with better result checking, and that deletes rows from the underlying tables while executing PreparedStatements. (with ASF license this time, but it still isn't ready for inclusion).

[Daniel John Debrunner]

Thanks for marking it asf, makes it easier for others to look at it :-)

Just looking at the test I couldn't see any described reason as to why the test breaks the model of a single connection per fixture and instead uses a shared connection and has code to manage a prepared statement cache. Is this addiitonal complexity (and more chance for bugs & confusion) needed for the test, or is it an attempt to make the test go faster? I think a cleaner approach to sharing connections across fixtures would be to have a connection pooling decorator, so that the test continues to look & feel like other tests.

[Dyre Tjeldvoll]

It was not to make the test go faster. The goal was to reuse any PreparedStatement as much as possible. I admit that as the test stands this does not buy you all that much. I had originally planned to have many more (smaller) test cases where each test case reused a ps with different parameter values, (I was told that the JUnit way was to have small and simple test cases). In the end I gave up on that approach since the number of test cases became unmanageable. But even now, the cache lets the PreparedStatements for creating tables and filling them (which is done in the beginning of most test cases) be reused. The same is true for some of the selects that are executed to verify that the content of a table is correct.

But if the added complexity outweighs the benefit of repeated execution of all PreparedStatements, then I can always change it. I may have to add another test case or two if some ResultSets only get created from PreparedStatements that are shared across test cases through the cache.

Not sure about how the connection pool decorator would work. It would have to be a very special pool with only one entry, since the goal isn't to improve speed, but to force every instance of a query to use an existing prepared statement if available.

[Daniel John Debrunner]

Dyre> I had originally planned to have many more (smaller) test cases where each test case reused a ps with different parameter values, (I was told that the JUnit way was to have small and simple test cases). In the end I gave up on that approach since the number of test cases became unmanageable.

That's how I'd thought the test would be, I'm intrigued by what made it unmanageable, a fixture is just a Java method.

[Dyre Tjeldvoll]

Now I'm utterly confused. Just to be clear: What I've been calling a "test case", that is what you refer to as a "fixture"? (a method in the JUnit class that has the prefix "test")

Assuming that is true:

You want each fixture to execute a prepared statement only once with a given set of parameters?
But you don't want a framework for sharing connections/prepared statements between fixtures?

Sorry for being slow here, but I don't see how I can satisfy both these requirements AND execute the same prepared statement multiple times. That WAS the primary goal, wasn't it?

Could you perhaps re-write one of the fixtures the way you think it should be and post it? That would be really helpful.

[Daniel John Debrunner]

Yes, fixture = test case

Sorry, I meant to say that each fixture would prepare its own statement and then execute it multiple times. i thpught that's what you had meant by "each test case reused a ps with different parameter values".

public void testSomething() {
    PreparedStatement ps = prepareStatement("some SQL");
    
     for at least three times {
         // set the parameters (at least once without reseting so same values are used)
         // execute
         // check results
     }

      ps.close();
}

[Dyre Tjeldvoll]

Thanks for the clarification. I'll upload a new patch when I've added 'repeat execution with existing values'-testing where it applies.

Not all PreparedStatements in this test have parameters. Sometimes I don't see how to add a parameter that will actually affect the ResultSets that are created/tested. E.g. some of the joins can always be parameterized by adding 'where col = ?' but I think that will only feed the result through a ProjectRestrictResultSet? And changing the parameter will then only affect the ProjectRestrictResultSet? I'm not sure, but that's how it looks...

Also the ps used to create OnceResultSets seems like a candidate for adding parameters. But when I tried that it no longer created OnceResultSets... Maybe not that strange, given the name, but I thought I'd ask...

[Dyre Tjeldvoll]

Forget the Q about OnceResultSet. Figured it out...

[Dyre Tjeldvoll]

I've attached a complete patch (v1) for the new test. It runs cleanly on trunk, but two fixtures fail when running with derby827_update920.txt applied. The new test has been added to lang._Suite. Please comment/review.

[Dyre Tjeldvoll]

The two fixtures are testLastIndexKeyResultSet and testOnceResultSet.
 
I don't yet know what is causing the second failure, but the first is caused by the boolean member variable LastIndexKeyResultSet.returnedRow not being set to false in LastIndexKeyResultSet.close(). If this variable is true, the RS will clear its 'currentRow' variable before returning it. Re-executing a ps that has created a LastIndexKeyResultSet will then give an empty result, rather than the max as it did on the first execution.

Would it not have been better to throw an exception when returnedRow is true? Isn't this an error for this type of RS?

[Dyre Tjeldvoll]

The failure in testOnceResultSet happens because the startKeyGetter of the TableScanResultSet returns the same key when the PS is re-executed even though this key depends on the PS-parameters and the current content of the emp table.

This seems related to how the startKeyGetter code is generated from a OnceResultSet. For an "ordinary" query (select * from emp where name = ?),
startKeyGetter returns the correct key (the current parameter value). OnceResultSet.getNextRowCore() only gets called the first time the PS gets executed, and then it returns 'ASHOK'. Seems like somehow this value gets "hard coded" into the startKeyGetter code...

Example:
prepare tst2 as 'select * from emp where name = (select name from emp where c0 <= ? intersect select name from emp where c0 >= ?)';

execute tst2 using 'values (1,1)';
-- => ASHOK
execute tst2 using 'values (2,2)';
-- => still ASHOK, but should be JOHN

prepare tst3 as 'select * from emp where name = ?';
execute tst3 using 'values (''ASHOK'')';
-- => ASHOK as expected
execute tst3 using 'values (''ROBIN'')';
-- => ROBIN as expected
execute tst3 using 'values (''LILY2'')';
-- => LILY2 as expected

[Dyre Tjeldvoll]

I can get the testOnceResultSet fixture to pass also, by doing

A) Call reinitialize on all Qualifiers in TableScanResultSet.close(), and

b) Change the following in SubqueryNode.java (I've added '&& false' to the if-test. Effectively always choosing the else case)
  /*
** If we have an expression subquery, then we
** can materialize it if it has no correlated
** column references and is invariant.
*/
if (isMaterializable() && false)
{
LocalField lf = generateMaterialization(acb, mb, subqueryTypeString);
mbex.getField(lf);

} else {
/* Generate the call to the new method */
mbex.pushThis();
mbex.callMethod(VMOpcode.INVOKEVIRTUAL, (String) null, mb.getName(), subqueryTypeString, 0);
}

a) seems rather innocent, but b) seems problematic. Does it mean that all rows that are to be qualified need to evaluate the subquery repeatedly?

I see that the resultsets in the subquery don't get created until the query is executed the first time. So obviously the generated byte code knows how to evaluate the entire subquery. The question is how to tell it to do that again when the TableScanResultSet is opened again, but not for each row...
 

[Knut Anders Hatlen]

Thanks for writing the test, Dyre. I think it is a valuable addition
to the test suite. I committed rsfromps.v1 with revision 513679.

Some small comments:

  - assertRow() and assertResultSet() duplicate functionality that is
    already implemented in JDBC.assertRowInResultSet() and
    JDBC.assertFullResultSet(). Perhaps they can be reused?

  - the use of try/finally in assertResultSet() might hide the
    original error if an exception is thrown from within the finally
    block.

  - the suite() method only runs the test in embedded mode. I think
    this is OK since it's a test of language result sets, but it would
    be good if there were a comment stating that explicitly. Or the
    test could be enabled in network client mode too (could be
    achieved by creating two test suites where one of them is wrapped
    by TestConfiguration.clientServerDecorator()).

  - perhaps the empty test methods (those flagged with TODO) should be
    commented out? Now JUnit will report that they ran successfully
    whereas they haven't actually tested anything.

[Dyre Tjeldvoll]

Thanks for looking at the test Knut-Anders. I'll try to make a
followup-patch which addresses your comments.

I did some more poking around in a debugger and I now think that
materialization of subqueries whose values are used as qualifiers
in table scans, is unnecessary. The qualifier actually caches the
value itself after the first invocation of the byte code, so
optimizing the byte code to return a cached value also, seems
redundant. This may be different in cases where the subquery isn't
used as a qualifier.

I think the byte code generation for sub queries must be changed so
that it matches the changes in derby827_update920.txt, and keeps a
reference to internal resultsets created by the subquery during the
initial execution. Subsequent executions would only check for a valid
result set tree and then return. That way the resultsets from the
subquery can be reused also.

I'm not quite sure what should go in the Activation.execute() stage,
and what should go into the ResultSet.open() stage. It seems like the
execute() stage is responsible for creating the resultset tree, and
that ResultSet.open() should propagate to open() (openCore()) calls
down the tree? That would give the execute() stage very little to do
when the result sets are already there? Anyway, with this strategy
TableScanResultSet.close() would invalidate the qaulifier cache, and
the next use of the qualifier would trigger a sequence of open calls
to the subquery result set tree.

Materialization of subqueries becomes a problem here, beacuse with it
one needs some way of invalidating the materialization (as well as the
qualifier cache) when a single execution is complete. Assuming that's
doable somehow, which stage should then be responsible for placing a
new qualifier inside the TableScanResultSet? Seems kind of awkward to
let rs.open() manipulate its own state that way. On the other hand,
letting the execution stage re-open the subquery result set tree to
get the new materialization and stuff into the existing
TableScanResultSet doesn't seem very clean either...

Feel free to provide opinions/comments/suggestions/advice...

[Dyre Tjeldvoll]

It does not seem like this issue is anyone's itch but mine,
but I thought I'd nevertheless record what what I found so
far. I've tried running the JUnit tests with the patch to
see what's causing the failures (there are a few).

A subset of the failures seem to be due to the fact that the
ownership of SQLChar's FormatIdStream (inherited by SQLClob) is
unclear. SQLChar itself seems to use the state of this stream as
part of its own state. Specifically, it assumes that if its
'value' and 'rawData' members are null but 'stream' member isn't,
then it can read its value from the stream.

But when doing EmbedResultSet.getClob() the very same
stream is passed to the client which may do anything with it. The
problem occurs when the reused XResultSet calls openCore() which
tries to clone the candidate row which still referencs exhausted
stream. The cloning fails with an exception.

So should XResultSet.close() simply set all stream references in
the candidate row to null? Or should this be done sooner? An
annoyance is that SQLChar.toString() depends on getString() which
in turn depends on the stream. So at any point after giving the
stream to the client it may be unsafe to call toString() on the
SQLChar object (and thereby on the candidate row).

[Dyre Tjeldvoll]

I tried adding the following to BulkTableScanResultSet.close():

// Set the candidate back to NULL to prepare for another open
DataValueDescriptor[] c = candidate.getRowArray();
if (c != null) {
for (int i = 0; i < c.length; ++i) {
if (c[i] == null) continue;
c[i] = c[i].getNewNull();
}
}

I would have preferred to use c[i].setToNull(), since that would
reuse the DataValueDescriptor objects, (ideally one should be
able to reuse BulkTableScanResultSet.rowArray and associated
obejcts as well), but this doesn't work for all implementations
of DataValueDescriptor (e.g. HeapRowLoaction which will trigger
an ASSERT when doing this).

Another alternative would be to switch on the real type of the
DataValueDescriptor and only call setToNull() for types that have
a stream (brittle and not very clean IMHO). Yet another
possibility would be null out the DVDs in the openCore() method
before cloning. This naturally leads to the question of why
openCore() wants a clone rather than a null copy...?

With this change suites.All has 5 failures and 0 errors. 1 of
those failures are the testOnceResultSet failure mentioned
previously.

[Dyre Tjeldvoll]

Of the 5 remaining failures in suites.All 3 come from
lang.TimeHandlingTest. The problem here seems to be yet another
variant of the "byte code with stale values"
problem. Specifically the RowResultSet created when using
CURRENT_TIMESTAMP, does not reset the byte code when it is closed
or opened again. The byte code appears to instantiate a
CurrentDatetime object that's reused for all subsequent
executions. Interestingly, this class has a finish() method that
resets it, but I don't understand how I call that method from say,
RowResultSet.close().

[Daniel John Debrunner]

I'll try and look into the CURRENT_TIMESTAMP issue, in writing that test I tried to understand how the current time was implemented.
The next execute of the activation automatically resets the current time value for the statement, which is held in the activation.
Maybe RowResultSet is only expecting constants?

[Daniel John Debrunner]

Is there a patch that changes the system to re-use the result sets? My patch from 21 Sep 06, by accident, has some additional unrelated changes to the time datatypes. So I was wondering if there's a new version that also includes and fixups to the ResultSets that you have been making?

[Daniel John Debrunner]

ps. I hope those unrelated changes are not causing the diffs you are seeing :-(

[Dyre Tjeldvoll]

Hi Dan, thanks for the feedback :) I'll try to answer your questions.

Dan> Maybe RowResultSet is only expecting constants?

Maybe. What I see is that calling
currentRow = (ExecRow) row.invoke(activation);
(in RowResultSet.getNextRowCore()) always returns the same set of
values for the columns, even for CURRENT_TIMESTAMP columns.


Dan> Is there a patch that changes the system to re-use the result sets?

Uh, I was under the impression that derby827_update920.txt did
just that. Anyway, that's the patch I have been running with,
plus the additional fixes I've described in my comments.

Dan> So I was wondering if there's a new version that also
includes and fixups to the ResultSets that you have been making?

No, I have not created a new patch, mostly because even though
the fixups seemed to remove the symptoms, I wasn't sure that they
were really the best (most general) solution. But I can do that,
I just need to extract those changes from my sandbox.

Dan> I hope those unrelated changes are not causing the diffs you are seeing

I'm not sure what the relation between SQLTime and
SQLTimestamp (which were modified in derby827_update920.txt) and
impl/sql/execute/CurrentDatetime.java is. From my tracing it
seemed like the latter is responsible for the cached
CURRENT_TIMESTAMP. At least I reached a breakpoint in
CurrentDatetime.getCurrentTimestamp() and the call-stack in the
debugger suggested that the call to this method came from
generated byte code.

I also checked CurrentDateTimeOperatorNode.generateExpression()
which has the following:

case CURRENT_TIMESTAMP:
acb.getCurrentTimestampExpression(mb);
break;

ExpressionClassBuilder.getCurrentTimeStampExpression() looks like this:

void getCurrentTimestampExpression(MethodBuilder mb) {
// do any needed setup
LocalField lf = getCurrentSetup();

// generated Java:
// this.cdt.getCurrentTimestamp();
mb.getField(lf);
mb.callMethod(VMOpcode.INVOKEVIRTUAL, (String) null,
"getCurrentTimestamp", "java.sql.Timestamp", 0);

So, my gut feeling is that those unrelated changes aren't to
blame, but I don't know...

[Daniel John Debrunner]

The getCurrentSetup() method called seems to set the generated execute() method correctly to call the forget() method of CurrentDateTime.
This should reset the CurrentDateTime on each execution of the activation.
(See ActivationClassBuilder.getCurrentSetup())

If the forget wasn't been called that could cause the time not to be reset, but since activations are reused today and current timestamp seems to work ok, it doesn't seem like that is the problem.

[Daniel John Debrunner]

one more comment on the patch (derby827_update920.txt), it was an example patch of the intended direction.
It might need confirming that the functionality is correct and it does set up the re-use the result sets correctly in all situations.
It worked for me for the simple primary key lookup that I was testing with, no guarantees it does the right thing in all situations.
I don't know of any issues, but just wanted to throw that out there.

[Dyre Tjeldvoll]

Here is a patch with ONLY the extra fixups that I've been playing with. With only this patch applied to trunk suites.All runs without failures.

[Dyre Tjeldvoll]

I realize that the patch is experimental, but it is the best
starting point I have :)

Thanks for the explanation of getCurrentSetup(). In the debugger
I have seen forget() being called from the first call to
activation.execute(). But the next time activation.execute() gets
called not much is happening.

So, somehow, much of the code is omitted when
activation.execute() is called the second time. I ran the
test (TimeHandlingTest.java) with some tracing and this is what I
get (right before the error occurs):

class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): activation.e
xecute(): INSERT INTO TIME_ALL(ID, C_TS) VALUES (?, CURRENT TIMESTAMP)
class org.apache.derby.impl.sql.execute.CurrentDatetime.forget()
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs.open()
class org.apache.derby.impl.sql.execute.CurrentDatetime FRESH TS
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.RowResultSet.getNextRowCore() currentRow
={ 1, NULL, NULL, 2007-03-21 17:04:01.594, 17:04:01, 17:04:01, 2007-03-21 17:04:
01.594, 2007-03-21 17:04:01.594 }
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs opened
-- First execution OK current timestamp is 17:04:01.594

class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): activation.e
xecute(): SELECT C_TS, D_TS0, D_TS1 FROM TIME_ALL WHERE ID = ?
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs.open()
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs opened
-- SELECT OK

class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): activation.e
xecute(): INSERT INTO TIME_ALL(ID, C_TS) VALUES (?, CURRENT TIMESTAMP)
-- Oops, no call to finish here

class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs.open()
-- Oops, no FRESH TS here

class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-21 17:04:01.594
class org.apache.derby.impl.sql.execute.RowResultSet.getNextRowCore() currentRow
={ 2, NULL, NULL, 2007-03-21 17:04:01.594, 17:04:01, 17:04:01, 2007-03-21 17:04:
01.594, 2007-03-21 17:04:01.594 }
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs opened
-- Second execution is still giving 17:04:01.594

class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): activation.e
xecute(): SELECT C_TS, D_TS0, D_TS1 FROM TIME_ALL WHERE ID = ?
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs.open()
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs opened
-- SELECT returns the same value as before

junit.framework.AssertionFailedError: CURRENT TIME before start of statement
-- Triggers junit assert

I can create a patch with this trace, if that is of any use...

[Daniel John Debrunner]

The issue is in StatementNode.generate().

I think the generated code will be somewhat equivalent to:

   if (resultSet == null)
   {
      resultSet = ...
      cdt.forget();
   }
   else
   {
      
   }
   return resultSet;

This works fine when the top level result set is never reused, but breaks once the result set is re-used,
since the intention for the forget() call would be once per execution, but re-using the result set changes it to once per activation.

Some cleanup/clarification is needed here for this issue as the execute() method is being used both to generate a result set tree
and perform per-execution tasks.

[Daniel John Debrunner]

d827_execute_cleanup is a patch that cleans up the generation of the execute method and cleans up comments to reflect reality.
E.g. that fillResultSet() is required to create a result set, not just an artifact of a code generation limit (which no longer exists anyway).

I think this will fix the problem with current_timestamp you are seeing,

I've run limited testing on this, will run the complete tests, this can be committed independent of any re-use of ResultSet change, it's basically just cleanup that clarifies how execute & fillResultSet work.

[Daniel John Debrunner]

Tests pass for d827_execute_cleanup (derbyall & suites.All on IBM 1.5). Any feedback on the patch or if it solves the failures in current timestamp would be useful.

[Dyre Tjeldvoll]

Thank you for looking at this. I have tried running with your
patch in my sandbox with tracing and it does indeed seem to solve
the problem I was seeing. However TimeHandlingTest still does
not pass when reusing resultsets, but this seems to be due to an
unrelated (?) problem.

The problem appears to be that CurrentDatetime gets the current
time by instantiating a java.util.Date object, but the test
calculates the current time after the query has executed by
instantiating a java.sql.Timestamp object with the value returned
from System.currentTimeMillis(). When the test fails the number
of ms returned from java.util.Date.getTime() is typically 1-2
higher than the value used to create the java.sql.Timestamp (even
though this value was obtained AFTER the query had executed):

class org.apache.derby.impl.sql.compile.ActivationClassBuilder.getCurrentSetup()
class org.apache.derby.impl.sql.execute.CurrentDatetime.<init>
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): activation.e
xecute(): VALUES (CURRENT TIMESTAMP, CURRENT_TIMESTAMP),(CURRENT TIMESTAMP, CURR
ENT_TIMESTAMP),(CURRENT TIMESTAMP, CURRENT_TIMESTAMP)
class org.apache.derby.impl.sql.execute.CurrentDatetime.forget()
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs.open()
class org.apache.derby.impl.sql.execute.CurrentDatetime.forget()
class org.apache.derby.impl.sql.GenericPreparedStatement.execute(): rs opened
class org.apache.derby.impl.sql.execute.CurrentDatetime FRESH TS
currentDatetime.getTime()=1174555658423
-- First row

class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-22 10:27:38.423
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-22 10:27:38.423
class org.apache.derby.impl.sql.execute.RowResultSet.getNextRowCore() currentRow
={ 2007-03-22 10:27:38.423, 2007-03-22 10:27:38.423 }
-- OK so far

class org.apache.derby.impl.sql.execute.CurrentDatetime.forget()
class org.apache.derby.impl.sql.execute.CurrentDatetime FRESH TS
currentDatetime.getTime()=1174555658426
-- This value is higher than the test expects, see below

class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-22 10:27:38.426
class org.apache.derby.impl.sql.execute.CurrentDatetime.getCurrentTimestamp()=20
07-03-22 10:27:38.426
class org.apache.derby.impl.sql.execute.RowResultSet.getNextRowCore() currentRow
={ 2007-03-22 10:27:38.426, 2007-03-22 10:27:38.426 }
-- Second row

junit.framework.AssertionFailedError: CURRENT TIME after end of statement tsv=20
07-03-22 10:27:38.426 et=2007-03-22 10:27:38.424 start=1174555658411 end=1174555
658424
-- Fails because end=1174555658424 < currentDatetime.getTime()=1174555658426

But I don't think this problem is related so please go ahead and commit
d827_execute_cleanup :)

[Dyre Tjeldvoll]

Actually the analysis I presented in my previous comment is
completely wrong. Please disregard. The failure was caused by
some experimental debug code I had forgotten to remove.
Sorry about the confusion. TimeHandlingTest now passes for me.

[Dyre Tjeldvoll]

I have run suites.All with ONLY derby827_update920.txt and
d827_execute_method_cleanup.txt to verify that
derby-827.extra.diff is still needed, and then I see 6 errors and 1
failure. After applying derby-827.extra.diff also, I see 3 errors:

jdbcapi.AutoGenJDBC30Test.testResultSetGarbageCollection():
"ASSERT FAILED you cannot insert rows after starting to drain"

and
jdbcapi.DriverMgrAuthenticationTest
jdbcapi.PoolDSAuthenticationTest

the first fails consistently, but the latter two pass when the test is run separately.
Not sure what is going on there.

But, at any rate, it seems reasonable to conclude that
d827_execute_method_cleanup.txt fixes both the timestamp issue AND the
OnceResultSet sub-query problem. Excellent work! Thanks Dan :)

[Daniel John Debrunner]

Great I will commit d827_execute_method_cleanup.txt.

Just to be a broken record on the derby827_update920.txt patch, the changes to the SQLTime/Date/Timestamp classes are not intended for this bug and should just be discarded.

[Dyre Tjeldvoll]

I think I've figured out what the failure in
jdbcapi.AutoGenJDBC30Test.testResultSetGarbageCollection() is all
about. InsertResultSet doesn't implement its own close() method,
so when close() is called, only some super.close() gets
called. This is probably not a good idea since InsertResultSet
has quite a bit of specific state information that probably needs
to be reset before one can re-use it. This particular problem occurs
because InsertResultSet.autoGeneratedKeysRowsHolder.close() never
gets called, and hence autoGeneratedKeysRowsHolder.state is never
set back to its initial value. This, in turn, triggers the
ASSERT.

Adding

  public void close() throws StandardException {
        super.close();
        if (autoGeneratedKeysRowsHolder != null) autoGeneratedKeysRowsHolder.close();
  }

to InsertResultSet makes the test pass. But I'm wondering if it
wouldn't be good to close/reset some (all?) of the other data
members, as well.

[Dyre Tjeldvoll]

Inspired by the initial discussion in this issue and my own
observations, I have compiled and attached 3 lists:

noclose_nofinish.txt: Lang ResultSets that neither override
close() nor finish() noclose_finish.txt: Lang ResultSets that
don't override close() but overrides finish() close_nofinish.txt:
Lang ResultSets that override close() but not finish()

The explanation of the difference between close() and finish() makes sense to me,
but how does cleanUp() fit into this?

Looking at DeleteResultSet as an example, it seems like cleanUp()
gets called at the end of open(). Presumably the author of the
class knew that all the cleanup could be done at the end of open,
and by calling the method 'cleanUp()' rather than 'close()' no
extra work would have to be done when the
ResultSet was closed. Maybe. I'm just guessing here...

[Dyre Tjeldvoll]

Dan> Just to be a broken record on the derby827_update920.txt
patch, the changes to the SQLTime/Date/Timestamp classes are not
intended for this bug and should just be discarded.

I actually don't think this is a problem. Whenever I apply
derby827_update920.txt, I first revert to revision 448292 (which
the patch was created from) and then do svn up. The changes to
SQLTime/Date/Timestamp must have been added in some later
revision and svn is smart enough to see this and not
complain about it. After updating to HEAD svn stat shows that
SQLTime/Date/Timestamp have no local modifications.

Here is the output from svn stat in my current sandbox:

dt136804@khepri29~/exp/derby-clean$ svn stat
? derby-827.extra.diff
M java/engine/org/apache/derby/impl/sql/execute/LastIndexKeyResultSet.java
M java/engine/org/apache/derby/impl/sql/execute/DeleteCascadeResultSet.java
M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java
M java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java
M java/engine/org/apache/derby/impl/sql/execute/BulkTableScanResultSet.java
M java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java
M java/engine/org/apache/derby/impl/sql/execute/AnyResultSet.java
M java/engine/org/apache/derby/impl/sql/GenericActivationHolder.java
M java/engine/org/apache/derby/iapi/sql/Activation.java
X tools/testing/derby

[Dyre Tjeldvoll]

Dyre> ...how does cleanUp() fit into this?

I figured it out. I mistakenly thought that 'BasicNoPutResultSet'
and 'NoRowsResultSetImpl' implement 'java.sql.ResultSet', when
they, in fact, implement org.apache.derby.iapi.sql.ResultSet. The
javadoc for the latter clearly describes purpose of each
method (also cleanUp()).

But this makes me think that all lang ResultSets should either
override all three methods, or clearly document why that isn't
required. When a method is missing in a class, it is difficult to
know if the method was intentionally left out or simply
forgotten. It also makes it difficult to know if a later change
actually requires you to start overriding a method.

Given that the javadoc for cleanUp() says 'Tells the system to
clean up on an error', its use in DeleteResultSet (called at the
end of every open()) seems a bit strange.

[Daniel John Debrunner]

> Given that the javadoc for cleanUp() says 'Tells the system to
> clean up on an error', its use in DeleteResultSet (called at the
> end of every open()) seems a bit strange.

Language ResultSets that do not return rows (ie. DML & DDL) perform all of their action in the open method,
thus the end of the open method is the correct time to clean up and close any open resources etc. I guess in
the delete case the clean up for an error is the same as the clean up for the non-error case, thus the code is
being shared. Of course more comments in the code would probably make this clear.

[Daniel John Debrunner]

and the javadoc for cleanUp saying 'Tells the system to clean up on an error' I think is wrong, at least it's misleading.

The method does not tell the system anything, it (I assume) is called when there is an error and the ResultSet will not be used anymore until another open.

A better comment might be:

  Clean up resources for this ResultSet after an error. [add some information about state change to close, if it's driven by this method or not]

[Daniel John Debrunner]

By the way, Dyre, I see this bug as a great example of open source open development. There's been incremental patches, resulting in ideas, and a test being committed before any changes (a good thing :-). There's been discussion, mistakes which are just accepted, experimental patches, analysis by you that made it quicker for me to guess where a problem might be. You've even continued with the open approach even when it seemed no-one was listening, when in reality I think it was just no-one had any value to add beyond your excellent analysis.

+1

[Dyre Tjeldvoll]

I'm glad some of my rambling proved useful. suites.All now pass
when re-using resultsets, but there are still som failures in
derbyall that I'm looking at. One is that

lang/altertable.sql fails with

ERROR XSAI2: The conglomerate (1,568) requested does not exist.

which seems to come from MiscResultSet.open():

public void open() throws StandardException
{
constantAction.executeConstantAction(activation);
super.close();
}

constantAction is a private member which only gets set in the
constructor. It seems like this member somehow gets stale,
because if that line is changed to

activation.getConstantAction().executeConstantAction(activation);

so that the constantAction is obtained from the activation on
each open, the test passes. But is this correct? That is, do we
expect to have to call getConstantAction() again for each open?

[Dyre Tjeldvoll]

lang/staleplans.sql is another failure in derbyall, but the
difference in output is only due to different query
plans. Specifically 'Number of opens' and 'Rows seen' in the
query plans are different when the result sets are re-used. But
what is the correct way of fixing this? I think it is possible to
reset these variables in close() so that the output is the same
as before, but it seem rather silly to always report the number
of opens as 1, when the result sets actually has been opened (and
closed) many times. Does the optimizer use these numbers in any
way? And if so, will it be affected by the change?

[A B]

> I think it is possible to reset these variables in close() so that the output is the same as before,
> but it It seem rather silly to always report the number of opens as 1, when the result sets
> actually has been opened (and closed) many times.

I tend to agree with this, though I'm not sure what the best way around it would be...

I think the intent of "number of opens" is to record how many times a result set was opened for a _single_ execution (which can be more than 1--namely, if "reopenCore()" is called). So is there any way to tell the difference between "reopens" that occur within a single execution vs "reopens" that occur because of multiple executions of the same statement? For example, is it true that "openCore()" will only be called once per statement execution while "reopenCore()" may be called multiple times per statement execution? If so, could we reset numOpens in the "openCore()" method instead of in the "close()" method (since the latter may be called several times per execution)? My apologies if that was already covered in earlier discussions; if so, please feel free to ignore me :)

> Does the optimizer use these numbers in any way? And if so, will it be affected by the change?

Based on my very limited experience with result sets (most of which came from working on DERBY-47), I don't think the optimizer uses "number of opens" nor "rows seen" in its estimates. But please keep in mind that I could be wrong here...

The one thing I noticed was that the value of "rows this scan" is, in certain situations, written to store and *that* value can affect estimates (because the optimizer can use that to guess how many rows will be returned from a table). For more see the "setRowCountIfPossible()" method in exec/TableScanResultSet.java. I don't know the details on how that works but I don't think "rows seen" nor "number of opens" directly affects the rowsThisScan value...

But again, that's just speculation based on limited experience. If anyone out there knows more hopefully s/he will post here...

[Dyre Tjeldvoll]

Thanks for the feedback AB. Here are some answers:

AB> If so, could we reset numOpens in the "openCore()" method
instead of in the "close()" method (since the latter may be
called several times per execution)?

I think so. It seems consistent with what I've seen so far, but I'm
not certain.

AB> I don't know the details on how that works but I don't
think "rows seen" nor "number of opens" directly affects the
rowsThisScan value...

That is promising. I've not seen a diff for "rows this scan", but
I guess that could be because there wasn't one in the tests that
dump the query plan...

[Dyre Tjeldvoll]

Here is the repro (test_inbetween.sql) requested by AB as well as a snapshot patch of
my sandbox (includes Dan's changes and my fixups) (derby-827.snapshot.diff). Feel free to
comment on the patch, but please note that it is work in
progress, (that means I will ignore any comments about missing
comments and/or poor indentation).

The patch traces creation of all language result sets, and this
creates a lot of output. This means that no diff-based tests
will pass with this patch. Most of this output can be removed by
reverting GenericResultSetFactory.java after applying the patch.

[A B]

In a thread on derby-dev I wrote the following:

>> I think the approach would be to generate the array of probe values
>> as a "saved object", which can then be retrieved from the activation
>> just like any other saved object. My perhaps slightly uninformed
>> guess is that this shouldn't be too difficult--maybe a day or two of
>> coding?

It turns out my guess was indeed misinformed. I looked at the various places where existing code calls "addSavedObject()" and in all cases the object being passed in is a specific compile-time object. That's also what the javadoc for addSavedObject() indicates. But in the case of IN list probing values the object of interest (namely, an array of DataValueDescriptors) does not exist until execution time because we create it as part of code generation. So use of saved objects is not going to work here.

That said, the generated values for parameters are "pluggable", meaning that if we generate some DataValueDescriptor for a parameter p1, then whatever value is bound to p1 will end up in the generated DataValueDescriptor at execution time. If we re-execute the statement with a different value assigned to p1, then the generated DataValueDescriptor will end up with new value, as well. Given that, I think all we need to do is save off the DataValueDescriptor array that we receive in the MultiProbeTableScanResultSet constructor, and then "reload" from that array on every call to "openCore()". The relevant code changes are pretty small; I'm attaching them as multiprobe_notTested.patch.

With these simple changes to MultiProbeTableScanResultSet, the "test_inbetween.sql" script that you attached to DERBY-827 returned the correct results (even after applying derby-827.snapshot.diff).

I haven't tested this at all, though (aside from running test_inbetween.sql), so please take this suggestion with caution. Just something I thought of when I realized that the savedObject approach wasn't going to work...

[Daniel John Debrunner]

Dyre>
-----------------------
constantAction is a private member which only gets set in the
constructor. It seems like this member somehow gets stale,
because if that line is changed to

activation.getConstantAction().executeConstantAction(activation);

so that the constantAction is obtained from the activation on
each open, the test passes. But is this correct?
---------------------------------------------------------

I don' t think that''s a correct change, it's strange it seems to be fixing something.
ConstantActions are constant and created once at compile time, and then passed over to the execution time.
Thus activation.getConstantAction() should return the same value.

Do you know which statement was causing this, basically I think the statement should have been invalidated once it was executed,
thus any future execution should be a recompile?

[Dyre Tjeldvoll]

AB> The relevant code changes are pretty small; I'm attaching
AB> them as multiprobe_notTested.patch.

Great! I'm looking forward to testing it. Thanks. I'll post my
findings here when I'm done.

Dan> Do you know which statement was causing this, basically I
Dan> think the statement should have been invalidated once it was
Dan> executed, thus any future execution should be a recompile?

Not off the top my head. But I'll revert the (incorrect) change I
made and dig into it some more.

[Dyre Tjeldvoll]

Dan> Do you know which statement was causing this [...]

The test that fails is lang/altertable.sql. There first occurs
when re-executing a prepared statement that adds a constraint to a
table that has been dropped and re-created:

ij> create table xxx(c1 int, c2 int);
0 rows inserted/updated/deleted
ij> prepare p1 as 'alter table xxx add check(c2 = 1)';
ij> execute p1;
0 rows inserted/updated/deleted
ij> drop table xxx;
0 rows inserted/updated/deleted
ij> create table xxx(c1 int);
0 rows inserted/updated/deleted
ij> execute p1;
ERROR 42X04: Column 'C2' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'C2' is not a column in the target table.
ij> alter table xxx add column c2 int;
0 rows inserted/updated/deleted
ij> execute p1;
ERROR XSAI2: The conglomerate (1,568) requested does not exist.
-- Should have been: "0 rows inserted/updated/deleted"

The second failure is similar (now dropping a constraint):
ij> -- verify that alter table works after drop/recreate of table
prepare p1 as 'alter table t0_1 drop constraint p2';
ij> execute p1;
0 rows inserted/updated/deleted
ij> drop table t0_1;
0 rows inserted/updated/deleted
ij> create table t0_1 (c1 int, c2 int not null constraint p2 primary key);
0 rows inserted/updated/deleted
ij> execute p1;
ERROR XSAI2: The conglomerate (992) requested does not exist.
-- Should have been: "0 rows inserted/updated/deleted"

So I'd say that your idea about immediate invalidation and forced
recompile looks correct.

[Dyre Tjeldvoll]

AB> I'm attaching them as multiprobe_notTested.patch.

I've now run both suites.All and derbyall with this patch applied
to a clean trunk. There were no failures (except for
SecurityPolicyReloadingTest)

I also ran suites.All and derbyall with this patch in my
derby-827 sandbox. I saw no failures in suites.All (except for
SecurityPolicyReloadingTest). lang/inbetween.sql passes and there
are no new failures in derbyall. So this patch gets two thumbs up
from me :)

[Dyre Tjeldvoll]

Status report: I currently see 6 failures when
running derbyall:

derbyall/derbyall.fail:lang/altertable.sql
Statement should have been invalidated and recompiled?

derbyall/derbyall.fail:lang/declareGlobalTempTableJava.java
derbyall/derbyall.fail:lang/declareGlobalTempTableJavaJDBC30.java
Temporary table is still there after rollback to savepoint, which
it shouldn't be

derbyall/derbyall.fail:lang/dynamicLikeOptimization.sql
Diff in printed query-plan, even after resetting counters in each open()

derbyall/derbyall.fail:lang/grantRevokeDDL2.sql
Two diffs:
1)
ij(USER1)> -- should fail
delete from t1 where i = 7;
-- Should ALSO report "ERROR: Failed with SQLSTATE 38000" here,
-- but only reports
ERROR: Failed with SQLSTATE 38002

2)
ij(USER2)> -- prepare statement, ok
prepare p1 as 'select * from user1.ttt2';
ij(USER2)> -- ok
execute p1;
I
-----------
8
1 row selected
ij(USER2)> set connection user1;
ij(USER1)> revoke select on ttt2 from user2;
0 rows inserted/updated/deleted
ij(USER1)> set connection user2;
ij(USER2)> -- expect error
execute p1;
I
-----------
8
1 row selected
-- This is incorrect. Should have failed
-- with "ERROR: Failed with SQLSTATE 28508"

derbyall/derbyall.fail:lang/staleplans.sql
Diff in printed query-plan, even after resetting counters in each open()

[Dyre Tjeldvoll]

WRT to the failures in derbyall/derbyall.fail:lang/grantRevokeDDL2.sql:
The first appears to be caused by a

java.sql.SQLException: The external routine is not allowed to execute SQL statem
ents.

which is NOT wrapped in a

java.sql.SQLException: 'The external routine is not a
llowed to execute SQL statements.' was thrown while evaluating an expression.

but this happens for ordianary (not prepared) statements, so I
don't see how re-execution could be causing this.

The second failure is easier to understand. Decompiling the
generated class which fails to properly check authorization
during the second execution we get:

    public ResultSet execute()
throws StandardException
    {
         throwIfClosed("execute");
         startExecution();
         BaseActivation.reinitializeQualifiers(e2);
         return ((resultSet == null) ? fillResultSet() : resultSet);
        // fillResultSet() will only be called once for a ps
    }


    private ResultSet fillResultSet()
throws StandardException
    {
         getLanguageConnectionContext().getAuthorizer().authorize(this , 1);
         // Problem - can't check authorization here, will not get called when
         // ps is re-executed

         return (getResultSetFactory().getScrollInsensitiveResultSet(getResultSetFactory().getIndexRowToBaseRowResultSet((long)960 , 5 , getResultSetFactory().getTableScanResultSet(this , (long)977 , 7 , getMethod("e0") , 2 , getMethod("e1") , 1 , null , -1 , true , e2 , "T1" , null , "SQL070402062141340" , true , false , -1 , -1 , 6 , false , 0 , true , 1.0 , 5.1195) , getMethod("e2") , 1 , "T1" , 1 , 2 , 3 , 4 , null , false , 1.0 , 5.1195) , this , 0 , 2 , getScrollable() , 1.0 , 5.1195));
    }

[Knut Anders Hatlen]

A B, do you think it is OK to commit multiprobe_notTested.patch now that Dyre has verified that the regression tests pass?

[A B]

> A B, do you think it is OK to commit multiprobe_notTested.patch now that Dyre
> has verified that the regression tests pass.

Yes, definitely. Sorry, I was thinking that Dyre would just take the patch and incorporate it into whatever it was he was doing. I didn't realize it was waiting for a commit.

Might be nice to add some explanatory comments to the multiprobe patch (esp. why we need "origProbeValues"), but since Dyre ran the tests (thanks Dyre!) I think it's okay to commit.

Are you (Knut Anders) volunteering to commit, or you asking me to?

Apologies if this has been blocking anything...

[Knut Anders Hatlen]

Thanks, Army! I don't think you have been blocking anything, I just noticed that there was a patch that could be applied independently and wondered if it was considered ready for commit. I have started suites.All/derbyall and can commit it tomorrow, but please feel free to commit it yourself.

[Knut Anders Hatlen]

Committed multiprobe_notTested.patch with revision 528973.

[Dyre Tjeldvoll]

I've looked some more at the failure in lang/altertable.sql:

Dan> I don' t think that''s a correct change, it's strange it
Dan> seems to be fixing something. ConstantActions are constant and
Dan> created once at compile time, and then passed over to the
Dan> execution time.
Dan> Thus activation.getConstantAction() should return the same value.

Dan> Do you know which statement was causing this, basically I
Dan> think the statement should have been invalidated once it was
Dan> executed, thus any future execution should be a recompile?

The statement is 'alter table xxx add check(c2 = 1)'. As an ALTER
TABLE statement it is invalidated immediately after it is
executed, and I have verified that this happens.

The next execute triggers a recompile (also verified), and a new
ConstantAction based on the new (modified) DataDictionary is
created. See GenericStatement.prepMinion(line 473).
In this ConstantAction, table xxx is associated with the
correct Conglomerate number. This new ConstantAction object gets
stored in the GenericPreparedStatement object.

A call to activation.getConstantAction() DOES return the same
object (according to System.identityHashCode) which was stored in
the GenericPreparedStatement during re-prepare.

The ConstantAction reference inside MiscResultSet on the other
hand, still points to the old ConstantAction object:

class [e-mail]() constantAction=class org.apache.derby.impl.sql.execute.AlterTableConstant
Action@28679195 actConstantAction=class org.apache.derby.impl.sql.execute.AlterT
ableConstantAction@8721445
class [e-mail]
eConstantAction() tableName=XXX tableId=5b17c1e6-0111-fa66-3772-00005462a030 tab
leConglomerateId=1568 td=SCHEMA:
[...]
ERROR XSAI2: The conglomerate (1,568) requested does not exist

AlterTableConstantAction@28679195 references 1568 while the new
AlterTableConstantAction@8721445 references 1584 (correct)

So an existing ConstantAction is not valid after re-prepare, at least
not in the current implementation.

[Bryan Pendleton]

It sounds like both the prepared statement and the result set are
keeping a reference to the constant action, and, after re-prepare,
the result set is using its own (old) constant action pointer rather
than picking up the newly-re-prepared constant action from the prepared
statement.

Perhaps the result set should not keep a reference to the constant
action, but should instead always pick up the constant action by
fetching it from the prepared statement. Then after the re-prepare the
result set would get the new constant action.

I guess what I'm suggesting is that perhaps there's no reason for
MiscResultSet to have a constantAction reference, since all it seems
to be doing is keeping a reference to an object which may become stale,
and instead it should just always fetch the constant action from the activation.

[Daniel John Debrunner]

If the statement is being re-compiled how is it picking up an old MiscResultSet?

Bryan's logic on not storing the constant action reference in the MiscResultSet seems good, but I don't see why the old MiscResultSet is even being used.

[Dyre Tjeldvoll]

Thank you Bryan and Dan, for your comments. I like your
suggestion Bryan, but how is it different from what i suggested in my
comment on 27. March (02:02am)?

Dan, the short (and stupid) answer is that the old MiscResultSet
gets returned (or is part of the ResultSet-tree returned) from
the call to Activation.execute(). You probably know better than
anyone what is supposed to happen when calling Activation.execute(), but
what I get from reading decompiled byte code (not from the failed
statement, but I think this part is identical for all
Activations?), is that the code will only create a new
ResultSet (tree) if its resultSet member variable is null. So
unless I'm missing something (quite possible) there is no way for
the execution to know that the statement has been recompiled:

public ResultSet execute()
                throws StandardException
    {
         throwIfClosed("execute");
         startExecution();
         BaseActivation.reinitializeQualifiers(e2);
         return ((resultSet == null) ? fillResultSet() : resultSet);
    }

There is already an interface for clearing the resultSet
parameter in BaseActivation, so I guess the preferred semantic
could be achieved by simply adding a call to

getActivation(lcc,false).clearResultSet()

in GenericPreparedStatement.rePrepare() (inside the
if (!upToDate()) test), but I have not tried this.

[Dyre Tjeldvoll]

Correction: Activation.clearResultSet() extists in trunk, but was removed in derby827_update920.txt

[Daniel John Debrunner]

My assumption is that if the statement is being recompiled then it should get a new activation instance and hence a new result set tree. So with a recompile there should be no relationship to the old activation or the old result set.

[Bryan Pendleton]

Hi Dyre. Indeed, my comment is in agreement with your 27-Mar comment.

Unfortunately, I don't have any ideas to offer about why the recompiled statement is getting the old activation instance.

[Dyre Tjeldvoll]

OK Bryan, thank you for taking the time to look at it. I really
appreciate it :)

I believe Dan's assumption is absolutely correct. A new
ActivationClass (with no ResultSet tree) is indeed created during
re-compile. The problem is, I think, that a new
GenericActivationHolder referencing the new generated class is
NOT created until the next call to
GenericPreparedStatement.getActivation(). Which does not happen,
so the execution is done with the old GenericActivationHolder.

A simple fix for this would be to call getActivation() each time
(seems to work), but this will create a new Holder for each
execution.

One could put the call to getActivation() inside rePrepare (after
verifying that a rePrepare is required), but there you don't have
access to the activation variable (could return the new
ActivationHolder or store it in a member variable).

One could also make getActivation() a bit more intelligent so
that it only returns a new Holder if the ActivationClass has
changed...

[Knut Anders Hatlen]

What about letting PreparedStatement.rePrepare() return a boolean to tell whether the statement was recompiled? If rePrepare() returned a boolean, we could reset EmbedPreparedStatement.activation from EmbedStatement.executeStatement() on each recompile. It seems like most of the other callers of rePrepare() correctly call getActivation() after calling rePrepare().

[Knut Anders Hatlen]

Thinking more about it, I don't think returning a boolean from rePrepare() is the right way to go. A single GenericPreparedStatement can be shared between many EmbedPreparedStatements, and my proposal would only get a new activation in the EmbedPreparedStatement that happened to trigger the recompile. Another approach would be to maintain a version field in GenericPreparedStatement so that we can check whether it has been recompiled since the last time we executed it.

[Dyre Tjeldvoll]

Actually, calling getActivation() for every execute does NOT work. It fixes the stale ConstantAction problem, but altertable.sql shows a number of other errors...

Knut's suggestion about verifying Activations (would that really be ActivationHolders?) against GenericPreparedStatements seems promising, I think.

[Dyre Tjeldvoll]

The failures I saw happened because the prepared statement
parameters weren't copied to the new activation. I get the test
to pass by modifying EmbedPreparedStatement.executeStatement() as
follows:

@@ -1649,7 +1657,22 @@
                checkExecStatus();
                checkIfInMiddleOfBatch();
                clearResultSets();
- return super.executeStatement(a, executeQuery, executeUpdate);
+
+ ExecPreparedStatement ps = a.getPreparedStatement();
+ try {
+ if (ps.getActivationClass() != null) {
+ activation = ps.getActivation(lcc, false);
+ activation.setParameters(a.getParameterValueSet(),
+ ps.getParameterTypes());
+ }
+ } catch (StandardException se) {
+ throw EmbedResultSet.noStateChangeException(se);
+ }
+ catch (Exception e) {
+ e.printStackTrace();
+ }
+
+ return super.executeStatement(activation, executeQuery, executeUpdate);
        }
 
Note that this is just a proof of concept hack. A real solution
should:

- include a test to see if the activationClass has changed and
  only call getActivation() when necessary

- would need to make similar changes to
  EmbedPreparedStatement.executeBatchElement(), or move the logic
  to EmbedStatement

- not use 'false' as the second argument to getActivation()

- have cleaner exception handling

- avoid testing if activationClass is null. This is a kludge to
  avoid calling rePrepare inside getActivation() which fails with
  an NPE

[Knut Anders Hatlen]

I had a look at Dyre's snapshot of his sandbox (derby-827.snapshot.diff) and I think some of the changes could be committed independently. I applied the changes to the close methods of ProjectRestrictResultSet, LastIndexKeyResultSet and InsertResultSet on a clean sandbox, as these changes looked simple and harmless to me, and ran suites.All and derbyall with no failures. (I removed the check for source != null in ProjectRestrictResultSet.close() since its constructor asserts that source is not null.) Attached a new patch containing only these changes (d827-close-cleanup.diff) which I plan to commit unless anyone objects.

[Dyre Tjeldvoll]

I just ran the other derbyall tests that failed (lang/grantRevokeDDL.sql,
lang/declareGlobalTempTableJava, lang/declareGlobalTempTableJavaJDBC30) and they all pass with my hack.

[Dyre Tjeldvoll]

I'm trying to think about how to create a proper solution out of my hack. This is my current understanding:
- any time rePrepare() gets called, any Activation that refers to that prepared statement could be "stale" (meaning that the ActivationHolder refers to an outdated generated class)

- it would be possible to test for this staleness by adding an boolean isValid() method to the Activation interface. Its implementation would be something like (in BaseActivation):
 {
     GeneratedClass curGc = getPreparedStatement().getActivationClass();
     if (curGc == null || curGc == gc) {
          return true;
     }
     return false;
}

- in all places where rePrepare may have been called, one needs to do

if (!activation.isValid()) {
    activation = activation.getPreparedStatement().getActivation(lcc, ...);
}
activation.xxx();

I think this will work, but I would prefer a more automated solution. Maybe a solution where the same ActivationHolder could be used across rePrepares, and automatically detect and fix staleness.

[Bryan Pendleton]

> any Activation that refers to that prepared statement could be "stale"

Is it possible to use the DependencyManager for this? Make the Activation(s)
dependent on the prepared statement?

[Daniel John Debrunner]

I thought that this was handled already by the activation/prepared statement mechanism, I'll try to find some time to look into what isn't happening that should.

[Knut Anders Hatlen]

Committed d827-close-cleanup with revision 530343.

[Knut Anders Hatlen]

I see that many of the result set classes check the isolation level in their constructors and store it in a field. Couldn't this cause problems if a statement is re-executed after changing the isolation level?

[Dyre Tjeldvoll]

I'm afraid I may have introduced some red herrings into the discussion:

Herring 1: The reason why the test were running OK was because I
created a new ActvationHolder on each execution. I thought that a
new Holder would use the old byte code so that if the byte code
hadn't changed I would still reuse the result sets. But since the
ActivationHolder constructor creates a new Activation instance
from the GeneratedClass each time, I was essentially throwing
away the result sets each time. That's why all the tests were
passing.

Herring 2: Even when doing a reprepare you may get the same
GeneratedClass object out of the query tree, so simply testing if
the gc in the ps and the gc in the ActivationHolder is the
same (as I did in my code example) will not be enough. In this
case I think you need to manually set BaseActivation.resultSet
to null to throw away the old result set tree.

[Dyre Tjeldvoll]

Given that Dan is planning to look into this, I'm not going to
push my solution much further. Just wanted to mention that by
adding the versioning proposed by Knut, and adding a rebind()
method to ActivationHolder which uses the version number to see
if it needs to load a new Activation from the
GeneratedClass (which may be the same as before) I got
altertable.sql to pass (and trace printouts show that it is
working as expected). But the other tests still fail, so there
are probably a more issues to look into.

[Daniel John Debrunner]

I think I understand now. GenericActivationHolder already handles the case when the activation generated class changes, but for constant action statements there is no generated activation class. The activation class implmentation is fixed (to avoid re-compiling the same basic class each time) then the check in GenericActivationHolder.execute is never triggered. Of course that works fine at the moment because the result set tree is always regenerated.

Thus I now think that Dyre's original change is correct:

 activation.getConstantAction().executeConstantAction(activation);

because now I understand why it is making the difference. Before I thought that a new activation would have been generated due to the recompile, but it won't because the activation class does not change and there's no need to because the activation class does not change.

Sorry for any confusion, but (at least to my thinking) it's good to understand why a change fixes a problem.

And of course this change (in MiscResultSet) can be made independently of the re-use, it works either way.

[Dyre Tjeldvoll]

Dan> GenericActivationHolder already handles the case when the
Dan> activation generated class changes

Duh! I don't understand how I missed that. I feel silly now. I'll
post my original suggestion as a patch when I've run the tests.

[Knut Anders Hatlen]

Dyre's snapshot patch contains a simple fix for something that clearly looks like a bug in TemporaryRowHolderImpl. Derbyall and suites.All ran cleanly with that single file patched. Committed the fix with revision 530723.

[Dyre Tjeldvoll]

Attaching MiscResultSetConstantAction.diff. derbyall and suites.All pass.

I believe I know the why lang/declareGlobalTempTableJava is failing. It turns out that a call to 'markTempTableAsModifiedInUnitOfWork' gets compiled into the fillResultSet() method when accessing a (global?) temporary table. Since fillResultSet() now is only called on the first execution the temp table isn't marked as modified in later executions, and rollback doesn't clean it up as it should.

[Knut Anders Hatlen]

Committed MiscResultSetConstantAction.diff with revision 531349.

[Dyre Tjeldvoll]

The failure in lang/declareGlobalTempTableJava goes away if the
call to DMLModStatement.generateCodeForTemporaryTable() is
changed as follows:

Index: java/engine/org/apache/derby/impl/sql/compile/InsertNode.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/InsertNode.java (revisio
n 531019)
+++ java/engine/org/apache/derby/impl/sql/compile/InsertNode.java (working
 copy)
@@ -815,7 +815,7 @@
                                                        throws StandardException
        {
                //If the DML is on the temporary table, generate the code to mar
k temporary table as modified in the current UOW
- generateCodeForTemporaryTable(acb, mb);
+ generateCodeForTemporaryTable(acb, /*mb*/acb.getExecuteMethod())
;
 
                /* generate the parameters */
                generateParameterValueSet(acb);

Note that the 'mb' argument references the local variable
'mbWorker' from StatementNode.generate() which refers to the
fillResulSet() method, so currently
generateCodeForTemporaryTable() adds its code to that method.

If this solution is acceptable, I can submit a patch (which can be
committed independently).

[Knut Anders Hatlen]

Attaching a patch (test-isolation.diff) which modifies these test cases in ResultSetsFromPreparedStatementTests:
testScalarAggregateResultSet, testLastIndexKeyResultSet, testDistinctScanResultSet, testDistinctScalarAggregateResultSet, testDistinctGroupedAggregateResultSet, testGroupedAggregateResultSet, testNestedLoopResultSet, testHashTableResultSet, testNestedLoopLeftOuterJoinResultSet, testHashLeftOuterJoinResultSet

They now try to re-execute the statements after dropping and recreating the tables and after changing the transaction isolation level. The test still passes on trunk, but all the modified test cases fail with a lock timeout when derby827_update920.txt is applied.

[Knut Anders Hatlen]

Committed test-isolation.diff with revision 531820.

[Dyre Tjeldvoll]

Since the last time I ran suites.All a bunch of tests have been ported from the old harness. One of them, lang/ScrollCursors1Test fails when reusing lang result sets. The error is caused by 'seenFirst' not being initialized in openCore(). This is fairly easy to fix, but I'm wondering if 'target' (a CursorResultSet) needs some kind of resetting in either close() or openCore() as well? (I don't see any failures that can be tied to this, but just wanted to ask the question).

[Dyre Tjeldvoll]

Attaching a patch (resetMembersScrollInsensitive.diff) which
fixes the failure in lang/ScrollCursors1Test, which can be
applied independently. derbyall and suites.All pass.

[Knut Anders Hatlen]

Committed resetMembersScrollInsensitive.diff with revision 533003.

[Dyre Tjeldvoll]

Attaching a patch (TempTableToExecute.diff) which implements my suggestion for how to "touch" temporary tables in every execution.

[Dyre Tjeldvoll]

Forgot to mention that suites.All and derbyall pass with empTableToExecute.diff.

[Knut Anders Hatlen]

TempTableToExecute.diff looks correct to me. execute() sounds like a more natural place for this code than fillResultSet().
Committed revision 533314.

[Dyre Tjeldvoll]

Attaching a patch (RowChanger.diff). An ASSERT is triggered in
RowChanger.open() when open() is called multiple times without an
intervening close(). This happens in lang/refActions.sql when
re-using result sets. The patch makes two changes:

1) Adds a call to RowChanger.close() in
   DeleteResultSet.cleanUp()

2) In DeleteCascadeResultSet.open() it moves the call to
   cleanUp() (which in turn calls DeleteResultSet.cleanUp()) into
   the finally block so that it gets called even when an
   exception is thrown.

suites.All and derbyall both run without errors. The patch can be
committed independently.

[Knut Anders Hatlen]

Thanks Dyre. The patch looks good. Committed RowChanger.diff with revision 536007.

[Knut Anders Hatlen]

I think the candidate row fix proposed in derby-827.extra.diff is correct, although I think it is better to place it in the base class where candidate is declared and instantiated. I also noticed that HashScanResultSet, TableScanResultSet, LastIndexKeyResultSet and DependentResultSet all use candidate rows the same way and probably all need the same fix. The first three of them share a common super class (ScanResultSet), and the last one looks like it should have been a sub-class of ScanResultSet. I have therefore created a patch (candidate.diff) which makes DependentResultSet extend ScanResultSet and pushes the creation/resetting of the candidate row into the base class.

Also, Dyre mentioned that he would have preferred to use DataValueDescriptor.setToNull() instead of getNewNull(). To avoid the asserts that prevented him from doing it that way, I added a new recycle() method to the DataValueDescriptor interface. This method simply invokes restoreToNull() and returns itself if possible. If it can't set its value to null, it allocates a new object which it returns.

suites.All ran cleanly with this patch. Derbyall has not finished yet. Will report back if it fails.

[Knut Anders Hatlen]

Derbyall passed for candidate.diff. Unless there are objections, I will commit it in a day or so.

[Knut Anders Hatlen]

I have also run run derbyall/suites.All with the combination of candidate.diff and derby827_update920.txt. Now there are no failures in suites.All, and two failures in derbyall (different statistics for dynamicLikeOptimization and staleplans).

[Knut Anders Hatlen]

Committed candidate.diff with revision 537353.

[Knut Anders Hatlen]

Derbyall and suites.All now run cleanly with derby827_update920.txt. Since all known issues with the patch have been fixed and there is a test for re-executing PreparedStatements (thanks, Dyre!), I think it is safe to commit it. If other issues show up because of the patch, I think they can be handled separately in other JIRAs. Committed to trunk with revision 540921. Thanks Dan for contributing the initial patch, and Dyre for all the work on preparing the surrounding code for this commit!

[Myrna van Lunteren]

should this issue be marked fixed? Or is there more work?

[Knut Anders Hatlen]

Marking the issue as resolved in 10.3.0.0 since it seems like all the patches went in before cutting the 10.3 branch. Assigning it to Dyre since he did most of the work to get the fix in (thanks also to Dan for the initial contribution).

[Daniel John Debrunner]

Looking at the code related to this issue I see that ResultSet.finish() is still called once per execution, instead of only when the activation is closed. Examples are:
 
EmbedStatement.executeStatement - line 1267 - resultsToWrap.finish()

EmbedResultSet.close() - line 570 - theResults.finish()

This means that the use of ResultSet.finish() does not match its javadoc. Should these calls be ResultSet.close() instead?

[Knut Anders Hatlen]

I think you are right. It's a bit strange though that there doesn't seem to be any problems with reopening a finished ResultSet. I also noticed that BaseActivation.close() calls ResultSet.finish() and BasicNoPutResultSetImpl.finish() calls Activation.close(), so I think there still is some confusion as to when the different methods are to be called. Perhaps you could open a new JIRA for this?