db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel John Debrunner <...@debrunners.com>
Subject Re: [VOTE][PATCH]updates on forward only resultset using JDBC 2.0 UpdatableResultset APIs
Date Wed, 09 Mar 2005 17:34:35 GMT


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.]


View raw message