On Wed, 09 Mar 2005 09:34:35 -0800, Daniel John Debrunner
<[EMAIL PROTECTED]> wrote:
>
> +0.8
>
> I think the functionality is correct, but there are some issues in the
> method checksBeforeUpdateOrDelete(). They could be addressed in a
> subsequent patch.
>
> - The code at the end of checksBeforeUpdateOrDelete to get the table
> descriptor and perform column correlation name checks is expensive, and
> seems like it could be done once, rather than on every updateXXX. A
> final solution would ideally address these concerns at compile time and
> not runtime, e.g. part of compilation could be identifying if a
> statement is suitable for updateable result sets.
>
> - Making method behaviour depend on the name of the calling method is
> not a common programming practise in Derby, and again is expensive. Now
> every updateXXX() call has to perform three String comparisions that are
> not needed. Ie. this code.
>
> //the remaining checks only apply to updateXXX methods
> + if (methodName.equals("updateRow") ||
> methodName.equals("deleteRow") || methodName.equals("cancelRowUpdates"))
> + return;
>
> The typical way to address this would be to have the code before this in
> a separate method, have deleteRow, updateRow and cancelRowUpdate methods
> call that new method, and have checksBeforeUpdateOrDelete call that
> method (rather than duplicate code).
>
> - The lack of synchronization for the updateXXX() methods is also an
> area of concern. As coded two threads performing concurrent updateXXX()
> calls on the same result set will mess each other up, leading to missed
> update values or possibly NullPointerException later if cancelRowUpdates
> is called. But synchronization is a performance hit, and multiple
> threads executing concurrent operations on a JDBC object is not
> recommended. So maybe we can survive without it?
> [Putting the synchronization in getDVDforColumnUpdates() would be the
> logical place, not each updateXXX method.]
>
> Dan.
>
>
I have a patch which takes care of first 2 points raised by Dan.
Derby does not support correlation name for updatable columns. This
check was earlier made at runtime everytime an updateXXX was issued. I
have moved the check for correlation name to compile time in
CursorNode.java. If the cursor sql has a correlation name for an
updatable column, compile time code will throw an exception. The new
exception code and message has been added to SQLState.java and
messages_en.properties respectively.
The changes in EmbedResultSet.java is to remove the runtime check for
correlation name and to make the various other checks prior to
updateXXX, updateRow and cancelRowUpdates and deleteRow more
efficient.
In addition, I have changed the existing tests to have couple more
checks for compile time catch of correlation name.
Can a commiter please commit this patch for me?
svn stat
M java\engine\org\apache\derby\impl\sql\compile\CursorNode.java
M java\engine\org\apache\derby\impl\jdbc\EmbedResultSet.java
M java\engine\org\apache\derby\iapi\reference\SQLState.java
M java\engine\org\apache\derby\loc\messages_en.properties
M
java\testing\org\apache\derbyTesting\functionTests\tests\lang\updatableResultSet.java
M
java\testing\org\apache\derbyTesting\functionTests\master\updatableResultSet.out
M
java\testing\org\apache\derbyTesting\functionTests\master\jdk14\updatableResultSet.out
thanks,
Mamta
Index: java/engine/org/apache/derby/impl/sql/compile/CursorNode.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/CursorNode.java
(revision 159525)
+++ java/engine/org/apache/derby/impl/sql/compile/CursorNode.java
(working copy)
@@ -716,6 +716,7 @@
int size = updatableColumns.size();
TableDescriptor tableDescriptor;
String columnName;
+ ResultColumnList rcls = resultSet.getResultColumns();
for (int index = 0; index < size; index++)
{
@@ -723,9 +724,22 @@
tableDescriptor = targetTable.getTableDescriptor();
if ( tableDescriptor.getColumnDescriptor(columnName) ==
null)
{
- throw
StandardException.newException(SQLState.LANG_COLUMN_NOT_FOUND, columnName);
+ throw
StandardException.newException(SQLState.LANG_COLUMN_NOT_FOUND, columnName);
}
+ ResultColumn rc;
+ //make sure that we are not using correlation names for
updatable columns.
+ //eg select c11 as col1, 2, c13 as col3 from t1 for update
of c11, c12
+ //In the eg above, correlation name for c11 will cause
exception because Derby does not support correlation name for updatable columns
+ //But correlation name for c13 is ok because it is a read
only column
+ for (int rclsIndex = 0; rclsIndex < rcls.size();
rclsIndex++) {//look through each column in the resultset for cursor
+ rc = ((ResultColumn)
rcls.elementAt(rclsIndex));
+ if (rc.getSourceTableName() == null)
//continue to look at the next column because this is derived column in the
select list
+ continue;
+ if (rc.getExpression() != null &&
rc.getExpression().getColumnName().equals(columnName) &&
!rc.getName().equals(columnName)) {
+ throw
StandardException.newException(SQLState.LANG_CORRELATION_NAME_FOR_UPDATABLE_COLUMN_DISALLOWED_IN_CURSOR,
columnName);
+ }
+ }
}
}
Index: java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
===================================================================
--- java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java (revision
159525)
+++ java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java (working copy)
@@ -155,8 +155,6 @@
private int fetchDirection;
private int fetchSize;
- //td will be used to ensure the column selected for updateXXX is part of
the table.
- private TableDescriptor td = null;
/**
* This class provides the glue between the Cloudscape
@@ -2038,15 +2036,25 @@
return false;
}
+ //do following few checks before accepting updateXXX resultset api
+ protected void checksBeforeUpdateXXX(String methodName, int
columnIndex) throws SQLException {
+ checksBeforeUpdateOrDelete(methodName, columnIndex);
+
+ //1)Make sure for updateXXX methods, the column position is not out of
range
+ ResultDescription rd = theResults.getResultDescription();
+ if (columnIndex < 1 || columnIndex > rd.getColumnCount())
+ throw
Util.generateCsSQLException(SQLState.LANG_INVALID_COLUMN_POSITION, new
Integer(columnIndex), String.valueOf(rd.getColumnCount()));
+
+ //2)Make sure the column corresponds to a column in the base table and
it is not a derived column
+ if (rd.getColumnDescriptor(columnIndex).getSourceTableName() == null)
+ throw Util.generateCsSQLException(SQLState.COLUMN_NOT_FROM_BASE_TABLE,
methodName);
+ }
+
//do following few checks before accepting updatable resultset api
//1)Make sure this is an updatable ResultSet
//2)Make sure JDBC ResultSet is not closed
//3)Make sure JDBC ResultSet is positioned on a row
//4)Make sure underneath language resultset is not closed
- //5)Make sure for updateXXX methods, the column position is not out of
range
- //6)Make sure the column corresponds to a column in the base table and
it is not a derived column
- //7)Make sure correlation names are not used for base table column
names in updateXXXX. This is because the mapping
- // of correlation name to base table column position is not available
at runtime.
protected void checksBeforeUpdateOrDelete(String methodName, int
columnIndex) throws SQLException {
//1)Make sure this is an updatable ResultSet
@@ -2065,57 +2073,11 @@
//4)Make sure underneath language resultset is not closed
if (theResults.isClosed())
throw Util.generateCsSQLException(SQLState.LANG_RESULT_SET_NOT_OPEN,
methodName);
-
- //the remaining checks only apply to updateXXX methods
- if (methodName.equals("updateRow") || methodName.equals("deleteRow") ||
methodName.equals("cancelRowUpdates"))
- return;
-
- //5)Make sure for updateXXX methods, the column position is not out of
range
- ResultDescription rd = theResults.getResultDescription();
- if (columnIndex < 1 || columnIndex > rd.getColumnCount())
- throw
Util.generateCsSQLException(SQLState.LANG_INVALID_COLUMN_POSITION, new
Integer(columnIndex), String.valueOf(rd.getColumnCount()));
-
- //6)Make sure the column corresponds to a column in the base table and
it is not a derived column
- if (rd.getColumnDescriptor(columnIndex).getSourceTableName() == null)
- throw Util.generateCsSQLException(SQLState.COLUMN_NOT_FROM_BASE_TABLE,
methodName);
-
- //7)Make sure correlation names are not used for base table column names
in updateXXX. This is because the mapping
- // of correlation name to base table column position is not available
at runtime.
- //If can't find the column in the base table, then throw exception. This
will happen if correlation name is used for column names
- if (td == null) getTargetTableDescriptor();
- if
(td.getColumnDescriptor(rd.getColumnDescriptor(columnIndex).getName()) == null)
- throw Util.generateCsSQLException(SQLState.COLUMN_NOT_FROM_BASE_TABLE,
methodName);
}
- //Get the table descriptor for the target table for updateXXX. td will
be used to ensure the column selected for updateXXX
- //is part of the table.
- private void getTargetTableDescriptor() throws SQLException {
- setupContextStack();
- try {
- LanguageConnectionContext lcc =
getEmbedConnection().getLanguageConnection();
- CursorActivation activation =
lcc.lookupCursorActivation(getCursorName());
- ExecCursorTableReference targetTable =
activation.getPreparedStatement().getTargetTable();
- SchemaDescriptor sd = null;
- if (targetTable.getSchemaName() != null)
- sd =
lcc.getDataDictionary().getSchemaDescriptor(targetTable.getSchemaName(),null,
false);
- else
- sd =
lcc.getDataDictionary().getSchemaDescriptor(lcc.getCurrentSchemaName(),null,
false);
-
- if ((sd != null) &&
sd.getSchemaName().equals(SchemaDescriptor.STD_DECLARED_GLOBAL_TEMPORARY_TABLES_SCHEMA_NAME))
- td =
lcc.getTableDescriptorForDeclaredGlobalTempTable(targetTable.getBaseName());
//check if this is a temp table before checking data dictionary
-
- if (td == null) //td null here means it is not a temporary table. Look
for table in physical SESSION schema
- td =
lcc.getDataDictionary().getTableDescriptor(targetTable.getBaseName(), sd);
- } catch (StandardException t) {
- throw noStateChangeException(t);
- } finally {
- restoreContextStack();
- }
- }
-
//mark the column as updated and return DataValueDescriptor for it. It
will be used by updateXXX methods to put new values
protected DataValueDescriptor getDVDforColumnToBeUpdated(int
columnIndex, String updateMethodName) throws StandardException, SQLException {
- checksBeforeUpdateOrDelete(updateMethodName, columnIndex);
+ checksBeforeUpdateXXX(updateMethodName, columnIndex);
if (columnGotUpdated[columnIndex-1] == false) {//this is the first
updateXXX call on this column
//this is the first updateXXX method call on this column. Save the
original content of the column into copyOfDatabaseRow
//The saved copy of the column will be needed if cancelRowUpdates is
issued
@@ -2473,7 +2435,7 @@
*/
public void updateAsciiStream(int columnIndex, java.io.InputStream x,
int length) throws SQLException {
- checksBeforeUpdateOrDelete("updateAsciiStream", columnIndex);
+ checksBeforeUpdateXXX("updateAsciiStream", columnIndex);
int colType = getColumnType(columnIndex);
switch (colType) {
@@ -2519,7 +2481,7 @@
*/
public void updateBinaryStream(int columnIndex, java.io.InputStream x,
int length) throws SQLException {
- checksBeforeUpdateOrDelete("updateBinaryStream", columnIndex);
+ checksBeforeUpdateXXX("updateBinaryStream", columnIndex);
int colType = getColumnType(columnIndex);
switch (colType) {
case Types.BINARY:
@@ -2576,8 +2538,8 @@
int length) throws SQLException {
//If the column type is the right datatype, this method will
eventually call getDVDforColumnToBeUpdated which will check for
//the read only resultset. But for other datatypes, we want to
catch if this updateCharacterStream is being issued
- //against a read only resultset. And that is the reason for
call to checksBeforeUpdateOrDelete here.
- checksBeforeUpdateOrDelete("updateCharacterStream",
columnIndex);
+ //against a read only resultset. And that is the reason for
call to checksBeforeUpdateXXX here.
+ checksBeforeUpdateXXX("updateCharacterStream", columnIndex);
int colType = getColumnType(columnIndex);
switch (colType) {
case Types.CHAR:
@@ -2680,7 +2642,7 @@
* if a database-access error occurs
*/
public void updateObject(int columnIndex, Object x) throws SQLException
{
- checksBeforeUpdateOrDelete("updateObject", columnIndex);
+ checksBeforeUpdateXXX("updateObject", columnIndex);
int colType = getColumnType(columnIndex);
if (colType ==
org.apache.derby.iapi.reference.JDBC20Translation.SQL_TYPES_JAVA_OBJECT) {
try {
@@ -3522,7 +3484,7 @@
* Feature not implemented for now.
*/
public void updateBlob(int columnIndex, Blob x) throws SQLException {
- checksBeforeUpdateOrDelete("updateBlob", columnIndex);
+ checksBeforeUpdateXXX("updateBlob", columnIndex);
int colType = getColumnType(columnIndex);
if (colType != Types.BLOB)
throw dataTypeConversion(columnIndex, "java.sql.Blob");
@@ -3568,7 +3530,7 @@
* Feature not implemented for now.
*/
public void updateClob(int columnIndex, Clob x) throws SQLException {
- checksBeforeUpdateOrDelete("updateClob", columnIndex);
+ checksBeforeUpdateXXX("updateClob", columnIndex);
int colType = getColumnType(columnIndex);
if (colType != Types.CLOB)
throw dataTypeConversion(columnIndex, "java.sql.Clob");
Index: java/engine/org/apache/derby/iapi/reference/SQLState.java
===================================================================
--- java/engine/org/apache/derby/iapi/reference/SQLState.java (revision
159525)
+++ java/engine/org/apache/derby/iapi/reference/SQLState.java (working copy)
@@ -737,6 +737,7 @@
String LANG_CURSOR_UPDATE_MISMATCH =
"42X29";
String LANG_CURSOR_NOT_FOUND =
"42X30";
String LANG_COLUMN_NOT_UPDATABLE_IN_CURSOR =
"42X31";
+ String LANG_CORRELATION_NAME_FOR_UPDATABLE_COLUMN_DISALLOWED_IN_CURSOR
= "42X42";
String LANG_DERIVED_COLUMN_LIST_MISMATCH =
"42X32";
String LANG_DUPLICATE_COLUMN_NAME_DERIVED =
"42X33";
String LANG_PARAM_IN_SELECT_LIST =
"42X34";
Index: java/engine/org/apache/derby/loc/messages_en.properties
===================================================================
--- java/engine/org/apache/derby/loc/messages_en.properties (revision
159525)
+++ java/engine/org/apache/derby/loc/messages_en.properties (working copy)
@@ -443,6 +443,7 @@
42X29=Update table ''{0}'' is not target of cursor ''{1}''.
42X30=Cursor ''{0}'' not found. Verify that autocommit is OFF.
42X31=Column ''{0}'' is not in FOR UPDATE list of cursor ''{1}''.
+42X42=Correlation name not allowed for column ''{0}'' because it is part of
the FOR UPDATE list.
42X32=The number of columns in the derived column list must match the number
of columns in table ''{0}''.
42X33=The derived column list contains a duplicate column name ''{0}''.
42X34=There is a ? parameter in the select list. This is not allowed.
Index:
java/testing/org/apache/derbyTesting/functionTests/tests/lang/updatableResultSet.java
===================================================================
---
java/testing/org/apache/derbyTesting/functionTests/tests/lang/updatableResultSet.java
(revision 159525)
+++
java/testing/org/apache/derbyTesting/functionTests/tests/lang/updatableResultSet.java
(working copy)
@@ -946,18 +946,39 @@
rs.deleteRow();
rs.close();
- System.out.println("Positive Test9b - using correlation
name for column names is not allowed with updateXXX");
+ System.out.println("Positive Test9b - using correlation
name for updatable column name is not allowed");
reloadData();
stmt =
conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE);
System.out.println("Table t1 has following rows");
dumpRS(stmt.executeQuery("select * from t1"));
- rs = stmt.executeQuery("SELECT c1 as col1, c2 as col2
FROM t1 abcde FOR UPDATE of c1");
+ try {
+ System.out.println("attempt to get an updatable
resultset using correlation name for an updatable column");
+ System.out.println("The sql is SELECT c1 as
col1, c2 as col2 FROM t1 abcde FOR UPDATE of c1");
+ rs = stmt.executeQuery("SELECT c1 as col1, c2
as col2 FROM t1 abcde FOR UPDATE of c1");
+ System.out.println("FAIL!!! executeQuery should
have failed");
+ }
+ catch (SQLException e) {
+ System.out.println("SQL State : " +
e.getSQLState());
+ System.out.println("Got expected exception " +
e.getMessage());
+ }
+ System.out.println("attempt to get an updatable
resultset using correlation name for an readonly column. It should work");
+ System.out.println("The sql is SELECT c1, c2 as col2
FROM t1 abcde FOR UPDATE of c1");
+ rs = stmt.executeQuery("SELECT c1, c2 as col2 FROM t1
abcde FOR UPDATE of c1");
rs.next();
- System.out.println("column 1 on this row is " +
rs.getInt(1));
+ rs.updateInt(1,11);
+ rs.updateRow();
+ rs.close();
+ System.out.println("Table t1 after updateRow has
following rows");
+ dumpRS(stmt.executeQuery("select * from t1"));
+
+ System.out.println("Positive Test9c - try to updateXXX
on a readonly column. Should get error");
+ reloadData();
+ rs = stmt.executeQuery("SELECT c1, c2 FROM t1 abcde FOR
UPDATE of c1");
+ rs.next();
+ rs.updateString(2,"bbbb");
try {
- System.out.println("attempt to send updateXXX
on correlation name column will fail");
- rs.updateShort(1, (new
Integer(123)).shortValue());
- System.out.println("FAIL!!! updateXXX should
have failed");
+ rs.updateRow();
+ System.out.println("FAIL!!! updateRow should
have failed");
}
catch (SQLException e) {
System.out.println("SQL State : " +
e.getSQLState());
Index:
java/testing/org/apache/derbyTesting/functionTests/master/updatableResultSet.out
===================================================================
---
java/testing/org/apache/derbyTesting/functionTests/master/updatableResultSet.out
(revision 159525)
+++
java/testing/org/apache/derbyTesting/functionTests/master/updatableResultSet.out
(working copy)
@@ -247,20 +247,31 @@
Positive Test9a - using correlation name for the table in the select sql is
not a problem
column 1 on this row is 1
now try to deleteRow
-Positive Test9b - using correlation name for column names is not allowed with
updateXXX
+Positive Test9b - using correlation name for updatable column name is not
allowed
Table t1 has following rows
C1,C2
-- --
{1,aa }
{2,bb }
{3,cc }
-column 1 on this row is 1
-attempt to send updateXXX on correlation name column will fail
-SQL State : XJ084
-Got expected exception Column does not correspond to a column in the base
table. Cant issue {0} on this column.
+attempt to get an updatable resultset using correlation name for an updatable
column
+The sql is SELECT c1 as col1, c2 as col2 FROM t1 abcde FOR UPDATE of c1
+SQL State : 42X42
+Got expected exception Correlation name not allowed for column 'C1' because it
is part of the FOR UPDATE list.
+attempt to get an updatable resultset using correlation name for an readonly
column. It should work
+The sql is SELECT c1, c2 as col2 FROM t1 abcde FOR UPDATE of c1
Table t1 after updateRow has following rows
C1,C2
-- --
+ {11,aa }
+ {2,bb }
+ {3,cc }
+Positive Test9c - try to updateXXX on a readonly column. Should get error
+SQL State : 42X31
+Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR15'.
+Table t1 after updateRow has following rows
+ C1,C2
+ -- --
{1,aa }
{2,bb }
{3,cc }
@@ -268,7 +279,7 @@
delete using first resultset
attempt to send deleteRow on the same row through a different resultset should
throw an exception
SQL State : XCL08
-Got expected exception Cursor 'SQLCUR16' is not on a row.
+Got expected exception Cursor 'SQLCUR17' is not on a row.
Move to next row in the 2nd resultset and then delete using the second
resultset
Positive Test11 - setting the fetch size to > 1 will be ignored by updatable
resultset. Same as updatable cursors
Notice the Fetch Size in run time statistics output.
@@ -2761,7 +2772,7 @@
{2,bb }
{3,cc }
SQL State : 42X31
-Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR1055'.
+Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR528'.
Make sure the contents of table are unchanged
C1,C2
-- --
Index:
java/testing/org/apache/derbyTesting/functionTests/master/jdk14/updatableResultSet.out
===================================================================
---
java/testing/org/apache/derbyTesting/functionTests/master/jdk14/updatableResultSet.out
(revision 159525)
+++
java/testing/org/apache/derbyTesting/functionTests/master/jdk14/updatableResultSet.out
(working copy)
@@ -247,20 +247,31 @@
Positive Test9a - using correlation name for the table in the select sql is
not a problem
column 1 on this row is 1
now try to deleteRow
-Positive Test9b - using correlation name for column names is not allowed with
updateXXX
+Positive Test9b - using correlation name for updatable column name is not
allowed
Table t1 has following rows
C1,C2
-- --
{1,aa }
{2,bb }
{3,cc }
-column 1 on this row is 1
-attempt to send updateXXX on correlation name column will fail
-SQL State : XJ084
-Got expected exception Column does not correspond to a column in the base
table. Cant issue {0} on this column.
+attempt to get an updatable resultset using correlation name for an updatable
column
+The sql is SELECT c1 as col1, c2 as col2 FROM t1 abcde FOR UPDATE of c1
+SQL State : 42X42
+Got expected exception Correlation name not allowed for column 'C1' because it
is part of the FOR UPDATE list.
+attempt to get an updatable resultset using correlation name for an readonly
column. It should work
+The sql is SELECT c1, c2 as col2 FROM t1 abcde FOR UPDATE of c1
Table t1 after updateRow has following rows
C1,C2
-- --
+ {11,aa }
+ {2,bb }
+ {3,cc }
+Positive Test9c - try to updateXXX on a readonly column. Should get error
+SQL State : 42X31
+Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR15'.
+Table t1 after updateRow has following rows
+ C1,C2
+ -- --
{1,aa }
{2,bb }
{3,cc }
@@ -268,7 +279,7 @@
delete using first resultset
attempt to send deleteRow on the same row through a different resultset should
throw an exception
SQL State : XCL08
-Got expected exception Cursor 'SQLCUR16' is not on a row.
+Got expected exception Cursor 'SQLCUR17' is not on a row.
Move to next row in the 2nd resultset and then delete using the second
resultset
Positive Test11 - setting the fetch size to > 1 will be ignored by updatable
resultset. Same as updatable cursors
Notice the Fetch Size in run time statistics output.
@@ -3035,7 +3046,7 @@
{2,bb }
{3,cc }
SQL State : 42X31
-Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR1191'.
+Got expected exception Column 'C2' is not in FOR UPDATE list of cursor
'SQLCUR536'.
Make sure the contents of table are unchanged
C1,C2
-- --
