Result set holdability defined inside stored procedures is ignored by server/client

• Key: DERBY-498
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Deepa Remesh
• Reporter: A B
• Component/s: Network Client, Network Server
• Affects Version/s: 10.1.2.1, 10.2.1.6
• Fix Version/s: 10.1.2.1, 10.2.1.6

Description

Assume I have a Java stored procedure that returns one or more result sets, and the holdability of those result sets is specified as part of the createStatement() method within the procedure definition (see below for an example).

If I execute this procedure against Derby embedded, the holdability of each result set matches that of the statement-specific holdability that is defined within the stored procedure. However, if I run the procedure against the Network Server using the Derby client, the holdability of _all_ result sets is the same, and it is based on the holdability of the statement that _executed_ the procedure--i.e. the statement-specific holdability that is defined within the procedure is ignored.

Ex: If I create a stored procedure that corresponds to the following method:

public static void p2(ResultSet[] rs1, ResultSet[] rs2,
    ResultSet[] rs3) throws Exception
{
    Connection conn = DriverManager.getConnection(
        "jdbc:default:connection");

    Statement st1 = conn.createStatement(
        ResultSet.TYPE_FORWARD_ONLY,
        ResultSet.CONCUR_READ_ONLY,
        ResultSet.HOLD_CURSORS_OVER_COMMIT);
    rs1[0] = st1.executeQuery("select * from testtable1");

    Statement st2 = conn.createStatement(
        ResultSet.TYPE_FORWARD_ONLY,
        ResultSet.CONCUR_READ_ONLY,
        ResultSet.CLOSE_CURSORS_AT_COMMIT);
    rs2[0] = st2.executeQuery("select * from testtable2");

    Statement st3 = conn.createStatement(
        ResultSet.TYPE_FORWARD_ONLY,
        ResultSet.CONCUR_READ_ONLY,
        ResultSet.HOLD_CURSORS_OVER_COMMIT);
    rs3[0] = st3.executeQuery("select * from testtable3");

    return;

    }
}

Then with Derby embedded, if I have a JDBC Statement that executes a call to this procedure, rs1 and rs3 will behave with HOLD_CURSORS holdability and rs2 will behave with CLOSE_CURSORS holdability--and that will be the case regardless of the holdability on the Statement that executed the call. That seems correct to me.

But if I do the same thing with Network Server, all of the result sets (rs1, rs2, and rs3) will have the same holdability as the JDBC Statement that executed the call. It doesn't matter what the holdabilities used within the procedure definition are: they will all be over-ridden by the holdability of the Statement that made the call.

Comments

[A B]

Attaching a program to demonstrate the problem. To run against embedded (for reference), do

java d498

To run against the server (which is where the problem is), start the Network Server on port 1527 and then do

java d498 server

We expect the server's behavior to match that of Derby embedded (because embedded works correctly), but a look at the output from this program shows that it does not. Instead, the holdabilities that are set within the stored procedure are ignored when running against the server.

[Deepa Remesh]

The attached patch 'derby-498.diff' changes network server to use statement holdability set within stored procedures. The patch does the following:

1. For callable statements, the execute method in DRDAStatement gets holdability from the statement that produced the resultset.
2. Added getResultSetHoldability method which takes a resultset and returns holdability.
3. execute method passes this holdability to addResultSet method, which sets DRDAResultSet.withHoldCursor with this value.
4. writeOPNQRYRM method in DRDAConnThread is changed to use the holdability of the current DRDAResultSet for setting SQLCSRHLD.
5. Added tests in lang/holdCursorJava.java. Created a new master file for DerbyNetClient.

Ran derbyall on WinXP Sun jdk1.4.2. No failures. However, in a previous run of derbyall I got failures in few encryption tests. The failures did not seem related to my change. So I ran the encryption suites again and they passed. Then ran derbyall again and all tests passed.

While working on this patch, I found some changes were affecting XA tests. I think the patch I uploaded does not have any side effects. But I am new to this and would greatly appreciate if someone can review and give me feedback. Once this is reviewed/committed, I would like to merge this to 10.1 branch too.

I applied the patch to my local codeline and ran the repro as well as the new holdCursorJava.java test, and it all works correctly. I also ran the new test cases without the server changes and the test failed as expected:

21c21
< 21, testtable2-one
---
> EXPECTED:ResultSet is null
38c38
< EXPECTED:ResultSet is null
---
> 11, testtable1-one

So that's good.

I'm not familiar enough with the XA code to say for sure if there are any concerns in that area, but I did review the changes overall and they look good to me. My one (very minor) comment is that the code path for NON-callable statements changes ever-so-slightly with this patch: in the "addResultSet' method, the value of "newDrdaRs.withHoldCursor" wasn't getting set before (so far as I can tell); now, it's set to the current DRDAStatement's "withHoldCursor" value (which is passed in from DRDAStatement.execute()). I didn't see anything wrong with this when I looked at the code--and in fact this change seems more "correct" to me--but it did make me think twice while I was reviewing it, so I thought I'd mention it.. If this is intentional and okay, then I vote +1 with the hope that someone more familiar with XA can review this patch, as well...

[Deepa Remesh]

Thanks Army for reviewing the patch. The use of "newDrdaRs.withHoldCursor" is intentional. Reasoning is:

With this change, the result set(s) generated by a callable statement can have different holdabilities. So the SQLCSRHLD value in OPNQRYRM reply message can be different for each result set. Hence I cannot use the holdability of statement in general but need to get it for each result set . For this, I am using the variable "withHoldCursor" in DRDAResultSet. This variable existed before but was not being used anywhere. Now it is used in DRDAConnThread's writeOPNQRYRM and set in DRDAStatement's addResultSet method:

check in writeOPNQRYRM before:
//pass the SQLCSRHLD codepoint only if statement has hold cursors over commit set
if (stmt.withHoldCursor == JDBC30Translation.HOLD_CURSORS_OVER_COMMIT)
writer.writeScalar1Byte(CodePoint.SQLCSRHLD, CodePoint.TRUE);

check in writeOPNQRYRM after:
//pass the SQLCSRHLD codepoint only if statement producing the ResultSet has
//hold cursors over commit set. In case of stored procedures which use server-side
//JDBC, the holdability of the ResultSet will be the holdability of the statement
//in the stored procedure, not the holdability of the calling statement.
if (stmt.getCurrentDrdaResultSet().withHoldCursor == JDBC30Translation.HOLD_CURSORS_OVER_COMMIT)
writer.writeScalar1Byte(CodePoint.SQLCSRHLD, CodePoint.TRUE);

call to addResultSet execute method in DRDAStatement:
        ...
//For callable statement, get holdability of statement generating the result set
if(isCallable)
addResultSet(rs,getResultSetHoldability(rs));
else
addResultSet(rs,withHoldCursor);
        ...

For callable statements, I set "withHoldCursor" in DRDAResultSet with the holdability of statement within procedure which generated the result set. For other statements, I set it to the holdability of the DRDAStatement (same as what was being checked in writeOPNQRYRM before this change).

[Kathey Marsden]

Deepa what were the affects on the XA tests that you saw with this patch?

[Deepa Remesh]

To solve this problem, initially I was using a different approach, where I was changing the holdability in PKGNAMCSN for each resultset returned by network server. With this approach, jdbcapi/xaSimplePositive.sql and jdbcapi/xaStateTran.sql were failing in derbynetclientmats. Also, I had to change a lot on the client side and still things were not working fully. So I reworked my patch to update OPNQRYRM reply message with the holdability for each resultset. With this approach, I did not have to change anything on client side. And my new testcase and all tests in derbyall passed.

The failures with my first approach made me think if there could be any other effect on XA. Also, I was going through some mails in derby-dev which talk about XA and holdability. So I requested someone who has worked in this area to look at this patch.
http://mail-archives.apache.org/mod_mbox/db-derby-dev/200506.mbox/[e-mail]%3E
http://issues.apache.org/jira/browse/DERBY-346

[Kathey Marsden]

Well all that XA trouble sounds like it was due to a not so good tip that I put in this bug (sorry about that). Your fix looks fine except that I wonder about the reflection to get the holdability with xa connections and jdk131 and whether the method will be available.

If we don't have one already, I think it would be good to add a procedure call that returns statements, both inside and outside of an xa transaction to xaSimplePositive.sql. You can't specify HOLD_CURSORS_OVER_COMMIT within an xa transaction (it should throw an error), but even statements in procedures with CLOSE_CURSORS_AT_COMMIT should go through the new code path.

There are probably existing procedure definitions in functionTests/util/ProcedureTest that could be called from one of the xa sql tests.

[Deepa Remesh]

With derby-498.diff, I ran derbynetmats and derbynetclientmats suites using Sun jdk13. All tests passed.

Also attaching an additional patch "xa_proc_test.diff" for xa tests. It does the following:
1. Adds procedure test to jdbcapi/xaSimplePositive.sql.
2. Updates master files.
This patch is independent of the changes for DERBY-498. I ran the test jdbcapi/xaSimplePositive.sql successfully in embedded and derbynetclientmats framework before and after applying derby-498.diff. (This test is excluded from derbynetmats)

I will open a new JIRA issue with repro for the problem found in xa holdability.

[Kathey Marsden]

Checked into the trunk

Date: Wed Oct 19 14:47:26 2005
New Revision: 326718

URL: http://svn.apache.org/viewcvs?rev=326718&view=rev

[Deepa Remesh]

I merged this patch to my 10.1 codeline and ran derbyall with Sun jdk 1.4.2 on Windows XP. 2 tests failed. Failures are not related to this change.
derbyall/derbyall.fail:lang/ConcurrentImplicitCreateSchema.java (I ran this test again and it passed)
derbyall/derbyall.fail:i18n/iepnegativetests_ES.sql (master update)

The merge command is:
svn merge -r 326717:326718 https://svn.apache.org/repos/asf/db/derby/code/trunk

If someone could commit this to 10.1 branch, it would be great. Thanks

[Kathey Marsden]

Checked this into 10.1
New Revision: 327002

URL: http://svn.apache.org/viewcvs?rev=327002&view=rev

[Deepa Remesh]

Verified by running the attached repro against trunk and 10.1 branch.

[A B]

Fix has been in trunk and 10.1 since last October, so I'm closing the issue.