Expose existing auto-generated key functionality through more JDBC APIs in Derby Client.

• Key: DERBY-2653
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Kathey Marsden
• Reporter: A B
• Component/s: JDBC
• Affects Version/s: 10.3.2.1, 10.4.1.3
• Fix Version/s: 10.3.3.0, 10.4.1.3

Description

See DERBY-2631 for details. Desired functionality is the same as for DERBY-2631, except that this issue is specifically for Derby Client (DERBY-2631 only addressed embedded mode).

Comments

[Kathey Marsden]

To implement this for client it seems like the really hard part would be trying to figure out which columns are not valid identity columns so that we can throw an error. I'm not sure how we could do this from the client. Does anyone have any ideas?

I was looking at the javadoc for prepareStatement(String sql, String[] columnNames) and noticed that it says:

"The driver will ignore the array if the SQL statement is not an INSERT statement, or an SQL statement able to return auto-generated keys (the list of such statements is vendor-specific)."

I wonder if this means that instead of throwing an exception if the user specifies a non-identity column or more than one column, we should just ignore those values, not throw an exception. That would certainly make things simpler for client.

[Kathey Marsden]

Attached is a prototype of the change to forward the unimplemented methods to xxx(String sql, int autoGeneratedKeys). It is NOT for commit. The argument checking needs to be added, comments, and tests. I just thought the patch might ease discussion regarding the argument checking problem and make it clear that except for the argument checking these calls are the same as the existing autoGenerate keys methods.

[Kathey Marsden]

I checked the JCC which doesn't support columnIndexes, but does support columnNames and it ignores columns that are not identity columns for columnNames. I suggest we change the embedded driver to do the same and not throw an error if the user specifies columns that are not identity columns. Then the client can do the same.

Kathey

[Kathey Marsden]

Actually I was mistaken, with columnNames with JCC against DB2, it actually returns the insert values for both columns whether they are identity columns or not. For example for:
s.executeUpdate(
"create table t31_AutoGen (c31 int, " +
          "c32 int generated always as identity (increment by 1), " +
"c33 int default 2)");
String[] columnNames = {"C32","C31"};
PreparedStatement ps = conn.prepareStatement("insert into t31_AutoGen values(1,DEFAULT,DEFAULT)",columnNames);

ps.executeUpdate();

ResultSet rs = ps.getGeneratedKeys();

The ResultSet has 1 row with two columns, with values 1, 1.

I don't think that's right actually, but it doesn't support my argument that we ignore non-identity columns. It would be interesting to see how other databases behave.

Kathey

[Daniel John Debrunner]

One approach might be to only accept one column arrays and then document something like:

   The one element column name or position array is ignored currently and the value returned corresponds to the identity column. To ensure compatibility with future changes an application should ensure the column described is the identity column. If the column name or position corresponds to another column or a non-existent column then future changes may result in a value for a different column being returned or an exception being thrown.

[edit - add possibility for an error in the future]

[Kathey Marsden]

Do you think we should make embedded behave the same way or just leave it as it is?

[Daniel John Debrunner]

Isn't embedded doing the logical thing, checking the column is correct? If so why would we regress it?

[Kristian Waagan]

Just wondering what the time frame for this change is, as it may affect my work on implementing statement pooling in the client driver.
The current patch attached to DERBY-3326 is already insufficient, because the call is allowed if the array is null. The current patch will then throw an OperationNotSupportedException.

[Kathey Marsden]

Attached is a partial patch for this issue which implments the following API's for client:

Connection.prepareStatement(String sql, String[] columnNames);

 Statement.execute(String sql, String[] columNames);

Statement.executeUpdate(String sql, String[] columnNames);

The columnIndex calls have not yet been implemented. An argument of null for columNames will return no generated keys. Otherwise client requires an array of length 1 for columnNames. It will actually ignore the value and always return the results of IDENTITY_VAL_LOCAL after the insert.
Users should only ever set columnNames to be the identity column.

Tests are running.

[Kathey Marsden]

Tests ran fine except for jdbcapi/StatementJdbc30Test.java which needed to be updated for the new behavior.

I'll attach an updated patch.

[A B]

Thanks for the patch, Kathey.

From a quick reading it looks like client will throw an error if the specified columnNames array has length zero, is that correct? If so, I wonder if it would make sense to treat a 0-length columnNames array the same as a NO_GENERATED_KEYS argument?

I only mention it because in EmbedStatement.java we treat a 0-length array to mean NO_GENERATED_KEYS. But having said that, it looks like we don't check the array size in EmbedConneciton.java (we just check whether or not it's null), which I think means that a 0-length array *will* return generated keys (if there are any) for the Connection.prepareStatement() API...hmm. The fact that EmbedStatement and EmbedConnection show different behavior is probably a bug (new Jira), but in either case embedded will execute without throwing an error.

I realize that embedded and client are going to have different behaviors with the array arguments, per your earlier comments, but in the specific case of a 0-length array, it seems like it should be possible to keep embedded and client in sync. Do you think it would be worth it to do so?

[Kathey Marsden]

Thanks Army for looking at the patch.

From a quick test it looks like embedded behaves properly and returns null from getGeneratedKeys() for a columnNames array of length 0. I will update the client patch to do the same.

After I resolve this issue I will file a bug for the embedded/client differences.

[Kathey Marsden]

Attached is an updated patch, derby-2653_columnNames2_diff.txt, that handles the empty array case. I added test cases for empty arrays. I disabled the PreparedStatement tests for embedded because of DERBY-3430.

[A B]

Thanks Kathey, this looks good to me.

Very minor nit:

            int genKeys = (columnNames == null ||
                    (columnNames != null && columnNames.length == 0)
                    ? Statement.NO_GENERATED_KEYS:
                    Statement.RETURN_GENERATED_KEYS);

I think that short-circuiting makes the "columnNames != null" check redundant since if it's null, the OR will short-circuit and the right hand side won't (shouldn't) ever get executed. So it could be simplified to:

            int genKeys = (columnNames == null || columnNames.length == 0)
                    ? Statement.NO_GENERATED_KEYS:
                    Statement.RETURN_GENERATED_KEYS);

But as I said, very minor :) Thanks for the additional test fixture, as well!

[Kathey Marsden]

Attached is a patch derby-2653_columnIndexes_diff.txt, which implements the columnIndexes API's for client. The following API's are implemented:

  Connection.prepareStatement(String sql, int[] columnIndexes);
  Statement.execute(String sql, int[] columIndexes);
  Statement.executeUpdate(String sql, int[] columnIndexes);

It was necessary to change a lot of prepared statement constructors and create methods to pass around the columnIndexes value in addition to columnNames. Otherwise the changes are very similar to the changes made for the columnNames API's. The same restrictions apply. To return generated keys, the user must specify an array of columnIndexes of length 1. The actual value of that array is ignored at this time and we always return the generated key for the identity column, regardless of the value of the single element array.

After I check this in I will file a bug against client that it does not reject columns that are not identity columns.

suites.All and derbynetclientsmats passed with IBM 1.5, but I will rerun with 1.6 since some *40* classes changed.

[Kathey Marsden]

Attached is the documentation change for this issue. I just used Dan's wording for the change.

[Kim Haase]

I've been following this issue to check on the possible impact on our JDBC documentation.

If I've understood correctly (not a sure thing), the restriction that "the user must specify an array of columnIndexes of length 1" would be a new one. Currently in http://db.apache.org/derby/docs/dev/ref/rrefjdbcjavasqlconnection30.html and http://db.apache.org/derby/docs/dev/ref/rrefjdbcjavasqlstatement30.html we
say, for Connection.prepareStatement, Statement.execute, and Statement.executeUpdate,

"Every column index in the array must correlate to an auto-increment column within the target table of the INSERT."

(And similarly for the methods that take a columnNames argument.) These statements imply that the array can have more than one element, so people might have to change existing code. The change in the documentation would be no problem, but the change in behavior (if real) might be a concern. What have I misunderstood?

I think a table can have more than one autoincrement column, although that might be an unusual practice -- I don't see any restriction on this in the generated-column-spec topic (http://db.apache.org/derby/docs/dev/ref/rrefsqlj37836.html).

I think the rrefjdbcjavasqlconnection30 and rrefjdbcjavasqlstatement30 topics would need to be changed along with crefjavstateautogen, although if I do all the work described in my DERBY-2388 proposal, the information in them would be folded into other topics.

[Kathey Marsden]

In Derby currently you can only have one identity column per table, so the restriction to a one element array is not a new one. If a user specifies multiple columns, The difference is with the embedded driver will throw an exception that the column is not an identity column (X0X0E, X0X0F) and client will throw an exception that the array is too long (XOXOD).

[Kim Haase]

Oh, I see. No problem, then, as far as behavior goes.

But I looked all over, and I don't think we actually say that explicitly anywhere -- not in the Reference Manual, anyway. I see an error message to that effect, but that's all. Should that information be added to the generated-column-spec topic (http://db.apache.org/derby/docs/dev/ref/rrefsqlj37836.html)? If so, I can file and fix a JIRA issue.

[Kathey Marsden]

Kim asked:
>Should that information be added to the generated-column-spec topic (http://db.apache.org/derby/docs/dev/ref/rrefsqlj37836.html)? If so, I can file and fix a JIRA issue.

That would be great.

Also should I hold off on the doc for this issue and let it get rolled in with DERBY-2388?

[Kim Haase]

Thanks, Kathey, I'll file an issue.

There is no need to hold off on the changes to the crefjavstateautogen topic. That information will be important to have. The only change I was going to make to that topic was to remove the specific reference to JDBC 3.0, because the information applies to all versions of JDBC that we currently support. You could do that yourself if you would like to.

If you wanted to fix the 3.0-specific Connection and Statement topics to correct the information about column indexes and column names, that would be okay too, since I haven't gotten to those and won't for a while. (I'm still waiting for feedback on DERBY-3409 (fixes related to the JDBC 2.0 topics).

[Kathey Marsden]

I checked in the doc change with revision 629580, but only made the changes for DERBY-2653.

[Kathey Marsden]

I received a request to port this change to the 10.3 branch. I was wondering if anyone had any objections to my porting this to 10.3?

[Kathey Marsden]

ported changes to 10.3, marking this issue resolved.