|
[
Permlink
| « Hide
]
Dag H. Wanvik added a comment - 09/Nov/05 02:28 AM
Proposal text uploaded.
Attaching a writeup for upcoming patch for SUR (files writeup-v1.html and SURChanges-v1.pdf)
This is a first cut at a patch for SUR. It corresponds to the description uploaded earlier as
http://issues.apache.org/jira/secure/attachment/12323122/writeup-v1.html. It passes derbyall, and although the many tests written under more tests will be written. There remains an issue around in-line compress, see the discussion in this mail thread: http://www.nabble.com/conflict-detection-strategies-t1120376.html#a2960144 Comments on writeup-v1.html.
In the conflict section you treat read uncommitted and read-committed as the same, but read-uncommitted isolation level will not get any locks, at least that's what I thought. Should we even allow updateable result sets when the isolation level is read-uncommitted? The read uncommitted/read-committed talks about the row being deleted, but the row being deleted by the same transaction is not discussed in the serializable/repeatable read section. " If the user renavigates to the row, the lock will be reset," - what does reset mean here, is it the lock will be obtained again? I didn't see any explaination how how the update or delete is performed using the RowLocation. The forward only ResultSet uses positioned SQL statements to perform the update, is that the case here? If not, how are you guaranteeing all the associated work is performed on an update or delete, such as firing triggers, enforcing constraints etc? "NOTE: Own changes means changes made by the result set itself, while other changes means changes made by other transactions of other objects in the same transaction. " _ I think you mean 'other transactions OR other objects in the same transaction' "In order to be able to view own changes, whenever the updateRow() method is called, the hashtable has to be updated with the new values for the updated columns" - how is this being implemented, couldn't see any mention of it. I hope this answers to some of your questions:
Read-uncommited isolation level does aquire locks for updatable forward-only result sets, the same behavior has been kept on scrollable insensitve updatable result sets. If a transaction attempts to delete a row that have previously been deleted (either in the same trasaction, or by another transaction) a warning will be added to the result set (for an updateRow or deleteRow call) or to the statement (for positioned SQL statements). The update or delete is performed using positioned SQL statements as it has been done for forward only ResultSets. The RowLocation is used when navigating to position the scan controller at the correct row, and aquiring locks when necessary. Since the scan controller is positioned on the correct row, and the necessary locks are aquired, it is possible to perform a positioned SQL statement for both deletes and updates as it has been done for forward only result sets. Positioned SQL statements are being used so the associated work on update or delete should happen in the same way as for other positioned SQL statements. As mentioned when submitting the patch, more tests will be written for SUR, and checking that the associated work is performed on an update or delete, is part of the tests that are left to be written. Two methods have been added to NoPutResultSet, updateCachedRow(ExecRow row) and markCachedRowAsDeleted(), in order to view own changes. They are called from UpdateResultSet.open() and DeleteResultSet.open() respectively and they propagate the changes done to the current row to ScrollInsensitiveResultSet, so that the hashtable can be updated. "NOTE: Own changes means changes made by the result set itself, while other changes means changes made by other transactions of other objects in the same transaction. " _ I think you mean 'other transactions OR other objects in the same transaction'. That's correct, it is supposed to be 'other transactions OR other objects in the same transaction'. Thanks for the answers, it would be good to update the writeup to reflect your answers.
Fernanda wrote: ---- Two methods have been added to NoPutResultSet, updateCachedRow(ExecRow row) and markCachedRowAsDeleted(), in order to view own changes. They are called from UpdateResultSet.open() and DeleteResultSet.open() respectively and they propagate the changes done to the current row to ScrollInsensitiveResultSet, so that the hashtable can be updated. ---- So, to confirm, this means the updated row in the result set will reflect the state of the row in the database and not the values set by the updateRow() call? Good to state here exactly what the behaviour is expected to be. So if I update a column to value 10 using rs.updateRow(), but some processing in the SQL positioned update (say a fired trigger) results in the column being stored as 20, then will the ResultSet have 20 or 10? Does the answer have any effect on the "own vs others" updates being visible? Dan wrote:
> So, to confirm, this means the updated row in the result set will > reflect the state of the row in the database and not the values set > by the updateRow() call? Good to state here exactly what the > behaviour is expected to be. So if I update a column to value 10 > using rs.updateRow(), but some processing in the SQL positioned > update (say a fired trigger) results in the column being stored as > 20, then will the ResultSet have 20 or 10? No, any further changes resulting from actions of the trigger will not be reflected in the result set in the current implementation. > Does the answer have any effect on the "own vs others" updates being > visible? Yes, I would say so. I tried to see if JDBC had anything to say on whether a trigger action would constitute an "own" change, but could not find anything. I think it is questionable to treat it is an "own" change, since JDBC classifies even changes via a positioned update on the result set's cursor as "others" (although we treat it as "own" in our implementation as stated in the write-up). That is, "others" are not limited to other transactions, but include other changes made through other statement objects. Since a trigger executes a separate SQL statement one could argue that this change happens via another statement object, and thus constitutes an "others" change. But I don't think the answer is obvious. Since SQL doesn't cater for updatable result sets for scrollable insensitive cursors, there is no help to be gleaned from the standard. Will update the write-up for now. > Dag H. Wanvik commented on
> ------------------------------------- > > Dan wrote: > > >>So, to confirm, this means the updated row in the result set will >>reflect the state of the row in the database and not the values set >>by the updateRow() call? Good to state here exactly what the >>behaviour is expected to be. So if I update a column to value 10 >>using rs.updateRow(), but some processing in the SQL positioned >>update (say a fired trigger) results in the column being stored as >>20, then will the ResultSet have 20 or 10? > > > No, any further changes resulting from actions of the trigger will not be > reflected in the result set in the current implementation. My thought here is that the updated row in the ResultSet would then reflect a state of the database that never occurred. This seems in conflict with all isolation levels except read-uncommitted. I can't think of another situation where an application using Derby can see a row in that state (excluding read-uncommitted). > [Dag's useful own vs. others discussion snipped] If "own" means changes made through the ResultSet, then one could say that side-effect changes are "own", since they resulted from the ResultSet's updateRow. Since an UPDATE statement and its side-effects are atomic, then the updateRow must be atomic. Though currently it may not be possible in Derby to have side-effects on an UPDATE statement. Currently triggers
are read-only on the firing table. I thought there might be some possibility of working around this restriction by having a trigger on A that updates B which then fires a trigger to update A. But I think this will just recurse, hit the trigger limit and throw an exception. At some point in the future it will be possible to have side-effects, users have requested before triggers that can modify the NEW values, there is some outstanding issue for allowing procedures in trigger statements, which could open up a lot of possibilities. Maybe there are other ways to get side-effects on an UPDATE? If no side-effects are possible then the proposed behaviour is ok, though the assumption should be clearly stated in the spec. Thanks for your comments!
Dan wrote: > My thought here is that the updated row in the ResultSet would then > reflect a state of the database that never occurred. This seems in > conflict with all isolation levels except read-uncommitted. I can't > think of another situation where an application using Derby can see > a row in that state (excluding read-uncommitted). Hmm, not quite sure what you mean here. Let me sum up my understanding. Other (read-uncommitted) transactions would see all the changes including the (trigger) side-effects. And so would the present transaction when querying the data base via other statement objects than the result set. The question, then, is what changes should be reflected in the result set itself as a consequence of the updateRow (or deleteRow) trigger action. Even if we did reflect the trigger action's changes to rows which are part of the result set, that data set is still not the same as the (uncommitted) view in the database for two reasons: * other changes made in the transaction (via other statement objects) are "other"s and not reflected in the result set. If they were, this would breach JDBC's stated semantics as far as I can understand. * Rows inserted via ResultSet#insertRow are not visible, either, in the present implementation. This is optional, though. JDBC provides metadata calls to allow for variation here. In light of this, I think the data in the (insensitive) result set should be viewed as a convenience, rather the "truth"; insensitive (by design) result sets provide more isolation than the current isolation level requires. But I agree this behavior is not obvious and should be documented clearly. > Though currently it may not be possible in Derby to have > side-effects on an UPDATE statement. Currently triggers are > read-only on the firing table. That would alleviate a user's potential expectation problem. But is this correct? I checked the reference manual for restrictions and could not find this. I also tried it and it seemed to work, cf. sample trigger: st.execute("CREATE TABLE tmpResultSet (id int primary key, name varchar(50))"); st.execute("CREATE TRIGGER trig AFTER UPDATE OF id ON tmpResultSet"+ " REFERENCING NEW AS UPDATEDROW "+ " FOR EACH ROW MODE DB2SQL UPDATE tmpResultSet SET name='triggered' "+ " WHERE UPDATEDROW.id = 10"); An updateRow of a row's 'id' to 10 caused the trigger action here. But maybe I am missing something here. Here is a new version of the write-up and patch.
Dag wrote:
> Dan wrote: >> My thought here is that the updated row in the ResultSet would then >> reflect a state of the database that never occurred. This seems in >> conflict with all isolation levels except read-uncommitted. I can't >> think of another situation where an application using Derby can see >> a row in that state (excluding read-uncommitted). > Hmm, not quite sure what you mean here. > Let me sum up my understanding [Dag's correct understanding snipped] I agree Dag with what you are saying, but in all the cases you describe every individual row represents a state of the database at some point in time. The trouble I have with the SUR updated row is that it may not match a valid state of the database. No user at any time could have issued a select from the database and seen those values. The row may also be self-inconsistent, that is, column values that are inconsistent with each other. I think this becomes important when the database is enforcing business logic through triggers (or any other mechanism that can modify columns on an UPDATE statement). If an old application updates a discount column in a row from 15% to 30% but new database side business logic has been put in place to limit discounts to 25%, then anyone selecting that row from the database sees either 15% or 25%, never 30%. However, with SUR the application has a scrollable ResultSet with a row that has discount 30%, since the result set is scrollable this incorrect value of 30% can continue to be displayed to the end user of the application. This just seems wrong to me. An example for the row self-inconsistent state, is an update of a customer's address. If there is a calculated column from the address, eg. zip code for the US, post code for the UK, sales-region for a sales application, then the updated row in the SUR will have a mismatch. E.g. a Californian zip code for a New York address. Maybe as you say, we can just document this, and that applications should not rely on this behaviour. To, me it seems like the row in the ResultSet should be either not updated or updated with the full effect of the updateRow() operation (see "own" changes). This also seems closely tied to implementing the ResultSet.refreshRow() method, if we continue with the proposed implementation and document it, then a natural work-around would be to tell the application to issue a refreshRow(), but Derby doesn't support it :-(. Of course if refreshRow was implemented, then we possibly wouldn't need to document this as a refreshRow() could be called after an updateRow() to present the correct information. And on the triggers, I guess I remembered incorrectly, I thought I'd seen code that disallowed modifying the triggered table. Dan wrote:
> My thought here is that the updated row in the ResultSet would then > reflect a state of the database that never occurred. This seems in > conflict with all isolation levels except read-uncommitted. I can't > think of another situation where an application using Derby can see > a row in that state (excluding read-uncommitted). A transaction can always see its own uncommitted changes in the database, so it does not conflict with any isolation levels that the ResultSet sees uncommitted data of its own transaction. I think the current approach is quite clear: the ResultSet gets populated with the data that the current transaction can see. Later it is insensitive to changes made by other objects (in its own transaction and by other transactions), however it is sensitive to changes made by own objects. We may choose if the ResultSet should see its own changes or not. If we chose to let the ResultSet see its own changes, we need to determine if the ResultSet should consider triggers which fires because of updateRow() or deleteRow() as "own" changes or "other" changes. I think it is ok to consider these triggers as other objects. Daniel wrote:
_______ This also seems closely tied to implementing the ResultSet.refreshRow() method, if we continue with the proposed implementation and document it, then a natural work-around would be to tell the application to issue a refreshRow(), but Derby doesn't support it :-(. Of course if refreshRow was implemented, then we possibly wouldn't need to document this as a refreshRow() could be called after an updateRow() to present the correct information. ________ I do not think implementing the ResultSet.refreshRow() method would help in this case. We are implementing result sets of type TYPE_SCROLL_INSENSITIVE and according to the "JBDC API Tutorial and Reference, Third Edition" p.759, the refreshRow() mothod does nothing for result sets of type TYPE_SCROLL_INSENSITIVE. Maybe you were thinking of the refreshRow() semantics for sensitive result sets. I think we should decide whether the effects of a trigger (or other mechanism that can modify columns on an UPDATE statement) constitute "own" or "others" changes, and whether we want the result sets of type TYPE_SCROLL_INSENSITIVE to be sensitive to "own" updates and deletes or not. I checked how the trigger situation is handled in Oracle, since they
do support Scrollable updatable insensitive result sets. In essence they take that approach that: - refreshRow will update the result set with the values of the underlying database, even for insensitive result sets. - an updateRow will implicitly do a refreshRow after and thus capture any changes of a row to to a trigger action (plus any other changes made by "others" to that row). This is a well defined semantic and would solve Dan's concern about row internal inconsistency, but is it is not JDBC compliant in my view. JDBC is quite clear on both points: - The refreshRow is stated to do nothing for insensitive result sets, cf. Tutorial book, 3rd ed. p 759: "..it does nothing for those that are TYPE_SCROLL_INSENSITIVE". - JDBC 3.0 spec: "The result set is insensitive to changes made to the underlying data source while it is open. It contains the rows that satisfy the query at either the time the query is executed or as the rows are retrieved." Tutorial, 3rd ed, p. 718, "If updates are visible, calling the appropriate getter method after an updater method has been called will return the new value". The Oracle doc admits that result of triggers are "others" in the sense of JDBC; I quote (Oracle9i JDBC Developer's Guide and Reference Release 2). Note that they use the term "external changes" in the sense of JDBC's "other's" changes: "Note: When you make an internal change that causes a trigger to execute, the trigger changes are effectively external changes. However, if the trigger affects data in the row you are updating, you will see those changes for any scrollable/updatable result set (also for insensitive! -Dag), because an implicit row refetch occurs after the update. They also make a point to distinguish between the insensitivity when scrolling and visibility that is reinstated when doing updates: "Note: Explicit use of the refreshRow() method, described in "Refetching Rows", is distinct from this discussion of visibility. For example, even though external updates are "invisible" to a scroll-insensitive result set, you can explicitly refetch rows in a scroll-insensitive/updatable result set and retrieve external changes that have been made. "Visibility" refers only to the fact that the scroll-insensitive/updatable result set would not see such changes automatically and implicitly. So, in my view, Oracle has tweaked the standard here to get semantics similar to what Dan suggests. We may want to do this for SUR; but I would like to hear what more people think of this before we decide: Should we be strictly JDBC compliant or choose a more "reasonable" behavior? Andreas wrote:
> A transaction can always see its own uncommitted changes in the database, so it does not > conflict with any isolation levels that the ResultSet sees uncommitted data of its own transaction. Agreed, but I see it that in this case the ResultSet does not hold an uncommitted row, it holds a row in a state that never existed on the database, due to server-side logic. No other select can ever return the values seen in the SUR for that row, I don't see this as the same as an uncommitted row in the database. Dag wrote: [ [oracle behaviour snipped] Thanks for investigating that Dag. > Should we be strictly JDBC compliant or choose a more "reasonable" behavior? I need to think a little more about why you think oracle's behaviour is not JDBC compliant (excluding supporting refreshRow with an insensitive rs) Dan wrote:
> Agreed, but I see it that in this case the ResultSet does not hold an uncommitted row, it holds a row in a state that never existed on the database, due to server-side logic. No other select can ever return the values seen in the SUR for that row, I don't see this as the same as an uncommitted row in the database. True. This would also be the situation if: * The ResultSet selects a,b * Another statement updates column a on a row * The ResultSet updates column b on the same row After this, the ResultSet does not reflect the changes made by the other statement, (column a would be the same as it initially was) and it does not reflect a state for the row which ever existed on the database. Andreas wrote:
--- quote True. This would also be the situation if: * The ResultSet selects a,b * Another statement updates column a on a row * The ResultSet updates column b on the same row After this, the ResultSet does not reflect the changes made by the other statement, (column a would be the same as it initially was) and it does not reflect a state for the row which ever existed on the database. --end quote Agreed, it's the same issue in my mind, and allowing such a row in a ResultSet would be wrong. I believe after an update the row in the ResultSet must reflect the state of the row in the database. Hence, like Oracle, Derby should perform the equivalent of a refreshRow. (reposting a mail comment by Øystein here. -Dag)
Daniel John Debrunner (JIRA) wrote: > Agreed, it's the same issue in my mind, and allowing such a row in a ResultSet would be wrong. I believe after an update the row in the ResultSet must reflect the state of the row in the database. Hence, like Oracle, Derby should perform the equivalent of a refreshRow. I think such an implementation kind of blurs the concept of sensitivity. You get a result set that is sensitive to some, but not all, changes performed by other statements in the same transaction. The available metadata for a result set, does not provide a way to describe such behavior. This whole discussion makes me think that the only sensible way to implement an insensitive result set is to not make own changes visible. At least, that would give a clean implementation with a behavior that is easy to understand. -- Øystein (reposting a mail comment by Dan here. -Dag)
Øystein Grøvlen wrote: > Daniel John Debrunner (JIRA) wrote: > >> Agreed, it's the same issue in my mind, and allowing such a row in a >> ResultSet would be wrong. I believe after an update the row in the >> ResultSet must reflect the state of the row in the database. Hence, >> like Oracle, Derby should perform the equivalent of a refreshRow. > > > I think such an implementation kind of blurs the concept of sensitivity. > You get a result set that is sensitive to some, but not all, changes > performed by other statements in the same transaction. The available > metadata for a result set, does not provide a way to describe such > behavior. Not sure I agree, I think insensitive result sets are like that anyway. Taking this from the spec, that Dag quoted: > - JDBC 3.0 spec: "The result set is insensitive to changes made to the > underlying data source while it is open. It contains the rows that > satisfy the query at either the time the query is executed or as the > rows are retrieved." Since a row value can be from the time the query was executed or as the rows are retrieved, I can have a ResultSet where some rows show the effect of a statement, while other rows do not show the effect of the same statement. Not sure an implicit refreshRow() after an updateRow() makes the situation any different. > This whole discussion makes me think that the only sensible > way to implement an insensitive result set is to not make own changes > visible. At least, that would give a clean implementation with a > behavior that is easy to understand. That's one clean option. Dan. (reposting a mail comment by Bernt. -Dag)
>>>>>>>>>>>> Dag H. Wanvik (JIRA) wrote (2006-02-27 15:44:44): > [ http://issues.apache.org/jira/browse/DERBY-690?page=comments#action_12367960 ] > > Dag H. Wanvik commented on > ------------------------------------- > > I checked how the trigger situation is handled in Oracle, since they > do support Scrollable updatable insensitive result sets. In essence > they take that approach that: > > - refreshRow will update the result set with the values of the > underlying database, even for insensitive result sets. The API (1.4.2) spec of refreshRow states: All values are refetched subject to the transaction isolation level and cursor sensitivity. Hence there is a difference in behaviour wrt sensitivity. This can only imply that refreshRow is a null-operation for insensitive resultsets. This is also in agreement with the tutorial as others had pointed out(See refreshRow on page 759). We should also strive to make "insensitivity" as close to the SQL defintion as possible (SQL 2003 p. 96): A change to SQL-data is said to be independent of a cursor CR if and only if it is not made by an <update statement: positioned> or a <delete statement: positioned> that is positioned on CR. A change to SQL-data is said to be significant to CR if and only if it is independent of CR, and, had it been committed before CR was opened, would have caused the table associated with the cursor to be different in any respect. A change to SQL-data is said to be visible to CR if and only if it has an effect on CR by inserting a row in CR, deleting a row from CR, changing the value of a column of a row of CR, or reordering the rows of CR. [...] - If the cursor is insensitive, then significant changes are not visible. Bernt I found the following in the JDBC 2.1 spec:
" 5.8.3 A result set s own changes We have pointed out that the visibility of changes made by others generally depends on a result set s type. A final point that concerns the visibility of changes via an open result set is whether a result set can see its own changes (inserts, updates, and deletes). A JDBC technology application can determine if the changes made by a result set are vis-ible to the result set itself by calling the DatabaseMetaData methods: ownUpdate-sAreVisible, ownDeletesAreVisible, and ownInsertsAreVisible. These methods are needed since this capability can vary between DBMSs and JDBC drivers. One s own updates are visible if an updated column value can be retrieved by calling getXXX() following a call to updateXXX(). " So the visibility of the ResultSets own changes does not have anything to do with the value in the database, it is the values you set when calling updateXXX(). (The values do not go down to the database until updateRow() is called). Has this been changed in JDBC 3 and later ? Dan> I need to think a little more about why you think oracle's
Dan> behaviour is not JDBC compliant (excluding supporting refreshRow Dan> with an insensitive rs) Dan, did you reach any conclusion here? I would like to settle this issue so we can move on with the review of the patch. I do understand your concern about row internal consistency, but I admit I am torn, since to get the effect of a trigger on the row being updated, we would need to refresh the row and this is in violation of the JDBC spec in two ways in my view: a) The definition of visibility of own changes in the specification: "One's own updates are visible if an updated column value can be retrieved by calling getXXX() following a call to updateXXX()." [from the JDBC 2.0 spec, this explanation of own changes has been since been removed, though, but is still in the tutorial book, which purports to expound on JDBC 3.0] Note, this quote does not mandate that updateRow() has been performed yet, so no side effects could be visible at this point; they did not happen yet. That is not to say that the wording precludes that the updateRow could lead to further changes to the column(s), than those resulting from the direct updateXXX actions, but any such change would need to be a result of an "own" change for it to be legal for an insensitive result set. And I would still contend that any actions to the underlying data by the trigger represents an "others" change. b) A refresh would also make visible other changes to the row performed by others (that is other updates, not made by the trigger action). One could possibly filter out such changes, to get just the net effect of any trigger action, but it seems esoteric. It could also be row internally inconsistent, as pointed out before in this thread. That is, to get row internal consistency, we need to make "other's" changes for the row visible (violation in my view). In summary, I think we should either - go for your suggestion, that is, do an implicit refresh after each updateRow (or positioned UPDATE), even if this breaks JDBC semantics a bit, *or* - keep the current implementation (reflect only such changes as are made directly through the updateXXX methods. I guess I am OK with either solution (just as long as we are clear on if/how we break JDBC), but I would like to move on and would appreciate your final conclusion :) I don't want to remove visibility up updates altogether, as Øystein proposed, as I think it makes for a less attractive/useful solution. If anybody else has a considered opinion on this, please raise your voices now! I think one difference is I don't see the argument for triggered actions being "other changes"
Triggered actions are atomic with a statement (in this case a SQL UPDATE). Since ResultSet.updateRow() is logically a SQL UPDATE for Derby, and triggered actions are atomic with the UPDATE, then triggered actions have to be considered "own" changes. They resulted from the ResultSet.updateRow, and if the updateRow fails then the triggered actions are rolled back. That's from a SQL point of view, then from JDBC we have in JDBC 3.0 spec. (section 14.2.4.2, for some reason this is no mention of "other changes" in the update section). --- The method DatabaseMetaData.othersDeletesAreVisible(int type) checks whether deletions made by others (another transaction or another ResultSet object in the same transaction) are visible to ResultSet objects of the specified type. --- I assume a mistake was made here, changes can be made by Statements as well as ResultSet objects in the same transaction. Even with that, triggered actions do *not* fall into either category, they are not made in another transaction and they are not made by other JDBC objects in the same transaction. Thus, triggered actions are not "other" changes. The wording that Andreas found in the JDBC 2.1 spec is another issue, I think it just looks wrong, I think own or other changes is a reflection of changes made at the database engine, not those just proposed in a ResultSet. The fact that this text was removed in subsequent versions might support that. Dag said: ---- b) A refresh would also make visible other changes to the row performed by others (that is other updates, not made by the trigger action). One could possibly filter out such changes, to get just the net effect of any trigger action, but it seems esoteric. It could also be row internally inconsistent, as pointed out before in this thread. That is, to get row internal consistency, we need to make "other's" changes for the row visible (violation in my view). ---- While a refreshRow() does make "other" changes visible, I see that as in-line with JDBC's defintion of insensitive, which is not the same a SQL's [more on that in the other thread :-)] Dag also said: --- [as one option] - go for your suggestion, that is, do an implicit refresh after each updateRow (or positioned UPDATE), even if this breaks JDBC semantics a bit, *or* --- So, I've only ever said the implicit refresh after an updateRow. Doing it after a positioned UPDATE would be wrong as that is an "other" change, it's executed by a different JDBC Statement object. Dan said:
> I think one difference is I don't see the argument for triggered > actions being "other changes" Triggered actions are atomic with a > statement (in this case a SQL UPDATE). Since ResultSet.updateRow() > is logically a SQL UPDATE for Derby, and triggered actions are > atomic with the UPDATE, then triggered actions have to be considered > "own" changes. They resulted from the ResultSet.updateRow, and if > the updateRow fails then the triggered actions are rolled back. Yes, this is the crux of the matter :) Too bad the standard isn't more explicit! > > That's from a SQL point of view, then from JDBC we have in JDBC 3.0 > spec. (section 14.2.4.2, for some reason this is no mention of > "other changes" in the update section). >> The method DatabaseMetaData.othersDeletesAreVisible(int type) checks >> whether deletions made by others (another transaction or another >> ResultSet object in the same transaction) are visible to ResultSet >> objects of the specified type. > I assume a mistake was made here, changes can be made by Statements > as well as ResultSet objects in the same transaction. Even with Agreed, must be a mistake. > that, triggered actions do *not* fall into either category, they are > not made in another transaction and they are not made by other JDBC > objects in the same transaction. Thus, triggered actions are not > "other" changes. Given the vagueness of the definition, it is hard to conclude. I noticed that the Oracle doc explanation called it "external change" (ie "others"). I agree that your interpretation makes sense from a logical perspective, though (atomicity). Iff we assume the 2.1 wording is wrong, I guess we have the latitude we need for accepting it as an "own" change. > The wording that Andreas found in the JDBC 2.1 spec is another > issue, I think it just looks wrong, I think own or other changes is > a reflection of changes made at the database engine, not those just > proposed in a ResultSet. The fact that this text was removed in > subsequent versions might support that. Yes, possibly. > While a refreshRow() does make "other" changes visible, I see that > as in-line with JDBC's defintion of insensitive, which is not the > same a SQL's [more on that in the other thread :-)] If we allow trigger's changes to be "own", I guess we need to swallow this one, too :) I wish JDBC had something like ODBC's keyset driven cursors sets, then we could happily refresh at each repositioning ;) > > Dag also said: > --- [as one option] > - go for your suggestion, that is, do an implicit refresh after > each updateRow (or positioned UPDATE), even if this breaks JDBC > semantics a bit, *or* > So, I've only ever said the implicit refresh after an updateRow. Doing it > after a positioned UPDATE would be wrong as that is an "other" change, > it's executed by a different JDBC Statement object. In our implementation, a positioned UPDATE (for the same result set) is considered an "own" change, cf. the write-up. It would be hard to differentiate since the implementation of updateRow piggybacks the positioned update. If we are all OK with it, let's do the refresh solution. Dag wrote:
> In our implementation, a positioned UPDATE (for the same result set) > is considered an "own" change, cf. the write-up. It would be hard to > differentiate since the implementation of updateRow piggybacks the > positioned update If we decide to refresh the row instead of only seeing the results of updateXXX methods, the reason why we chose to consider positioned updates as "own" changes will no longer be there. Then we can only refresh the row when an updateRow is executed, and stop considering positioned updates as "own" changes. Dan wrote: > [...] Hence, like Oracle, Derby should perform the equivalent of a > refreshRow. Do you mean that we should also, like Oracle, allow users to use the refreshRow() method to get an updated row, or should it still be a no-op when it is not being called from within the updateRow() method? I wrote a testprogram. It verfies the JDBC 2.1 specification definition of own changes in Derby for FORWARD_ONLY, UPDATABLE.
Statement s = testConn.createStatement (ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE); ResultSet rs = s.executeQuery("select * from t1 for update"); rs.next(); rs.updateString(4, "UPDATEDD!!"); System.out.println(rs.getString(4)); Output: UPDATEDD!! So, the changes are visible when you call updateXXX(). updateRow() will do the actual update in the database. Andreas said:
> [sample test program snipped] > > Output: UPDATEDD!! > So, the changes are visible when you call updateXXX(). updateRow() > will do the actual update in the database. Derby's DataBaseMetaData#ownUpdatesAreVisible(TYPE_FORWARD_ONLY) presently returns false (both drivers)... Dag Some comments/questions to the write-up:
* Updatability, 2nd paragraph: I think you should explain why navigations to new rows and previously fetched rows needs to be handled differently. * Isolation level read-uncommitted/read-committed, 2nd paragraph: - 1st sentence: "open" instead of "opened"? - "The result set may overwrite data in an updateRow() ... " I do not understand what you try to say here. Do you mean that updateRow may overwrite data that has been changed after the row was read by the result set? (In other words, you are talking about data in the database, not data in an updateRow.) - Last sentence: I would reformulate to say that only changed columns are written to the database on updateRow(). - Is there really anything particular about result sets in this context? Would not the same discussion be relevant for any multi-statement transaction? I just uploaded v3 of the write-up to address Øystein's comments.
See also answers inlined. Øystein wrote: > Some comments/questions to the write-up: > > * Updatability, 2nd paragraph: I think you should explain why > navigations to new rows and previously fetched rows needs to be > handled differently. This paragraph has been changed, see writeup-v3 > > * Isolation level read-uncommitted/read-committed, 2nd paragraph: > - 1st sentence: "open" instead of "opened"? yes > - "The result set may overwrite data in an updateRow() ... " I do > not understand what you try to say here. Do you mean that > updateRow may overwrite data that has been changed after the row > was read by the result set? (In other words, you are talking > about data in the database, not data in an updateRow.) > - Last sentence: I would reformulate to say that only changed > columns are written to the database on updateRow(). This 2nd paragraph has been changed, see writeup-v3. > - Is there really anything particular about result sets in this > context? Would not the same discussion be relevant for any > multi-statement transaction? No, I agree. I added these paragraphs: Note that these effects will also apply to rows changed by our own transaction if made via separate UPDATE statements, no matter what isolation level is active. If such effects are an issue, the user could increase the isolation level (and/or make own transaction behave!) For Derby, at some point in the future we might want to add updatable sensitive result sets and/or add some syntax to allow extra locking for rows in updatable result set. Dag I have reviewed part of this patch. I have so far reviewed the
changes to ScrollInsensitiveResultSet and some related classes. More will follow. Some of my comments may be more of a request for a general cleanup of the implementation, and not directly related to the changes in this patch. For such cases, I guess we should consider whether to file a separate jira issue. 1. ScrollInsensitiveResultSet: 1.1 Imports a few classes that are not used (TargetResultSet, SQLBlob, SQLVarchar). 1.2 Javadoc for classes: a) Should say something about that the hash table may be stored on disk. b) I miss a description of the layout of the cached rows. 1.3 getPreviousRow() a) I do not understand why this code is added. positionInLastFetchedRow() is called if currentRow==null, but currentRow does not seem to be updated by positionInLastFetchedRow(). Also, it does not seem that the subsequent call to getRowFromHashTable() depend on any data that is set by positionInLastFetchedRow(). 1.4 getNextRowFromSource() a) Comment: I am not able to understand what is going on here based on the comment: - "candidate" is not mention anywhere else in this class, and needs to be defined. - You mention TableScanResultSet, but I do not find any assumption in the code about that being the only possible type of the result set. It is also unclear whether you are talking about target or source here. - Why does these ResultSets need to refer to the same row, and why does it help to set the current row to null? - It is unclear to me what the relation is between the target result set and this result set. Is this explained somewhere? b) Having to look up something by cursor name, indicates to me that you increasing the coupling between modules that by design was not intended to be coupled. Also, this class already has a reference to an Activation. Is this the same or some other Activation? 1.5 addRowToHashTable() a) Javadoc: - key is no longer positionInSource, but currentPosition. - typo: "this methods" - You say that this method "is used in order to store the new values of that row". I think this is a bit misleading since this method only adds row to the hash table; it does not relate to updates. Updates are achieved by the caller who deletes the old version before adding the new. b) I do not like the hard-coding of constants for field numbers and field counts. c) Why are you recalculating extraColumns every time? Will it not be the same value for the entire lifetime of the object? d) Why is there some code in comments for the assignment to hashRowArray[1]? e) I am not quite sure, but would it not be better if position was a parameter to the function? That would make the dependency on the calling code more clear. 1.6 getRowFromHashTable() a) You are creating an object every time you do a lookup in the hash table. Would it be an idea to have a single SQLInteger object that is reused for this purpose? b) Disabled code in comments. Any reason why it is not just removed? 1.7 positionInLastFetchedRow() a) No JavaDoc b) Line exceeding 80 columns (not the only case.) c) Assignment of currentPosition: Could not this be done for all cases at the end (outside the if statement). 1.8 updateCachedRow() a) Are you sure that if a projection is needed that this will be done by the top node of the source tree? Could not there be cases where a projection is needed even if source is of a different type? (E.g., the source of source is a ProjectRestrictResultSet) b) Javadoc: Parameter name is missing (not the only case). 1.9 isDeleted()/isUpdated() a) It does not seem to be necessary to cast the objects stored in the hash table to DataValueDescriptor[] in order to access its contents. (Not doing so, could shorten some lines that are too long :-) 1.10 setRowLocation() a) I am not entirely happen with the name. It sounds more like an attribute is being updated than that the underlying cursor is repositioned. b) Javadoc: Parameter name is missing (not the only case). c) Would not an assert be better than if-test in this case? It seems to me that the code depends on the source being a CursorResultSet. 2. UpdateResultSet/DeleteResultSet a) Why is updateCachedRow()/markCachedRowAsDeleted() made general methods on NoPutResultSet? That requires touching a lot of classes not concerned with scrollable cursors. Would it not be possible to detect that the source is a ScrollInsensitiveResultSet, and do the necessary casting in this case. If you insist on making these general methods, I would suggest calling them updateRow/markRowAsDeleted and let each ResultSet do whatever it needs to do when rows are updated/deleted. b) Is there a reason why you place the calls in open() and not in collectAffectedRows()? c) You make the assumption that only a single row is affected. This may be the case for scrollable cursors, but generally open() may update/delete many rows. I think that needs to be explained in the code. 3. ProjectRestrictResultSet 3.1 setRowlocation() a) Javadoc: Parameter name is missing (not the only case). b) Assert instead of if-test? 3.2 doBaseRowProjection() a) Javadoc: Parameter name is missing (not the only case). b) The test for (source!=null) is not necessary. 4. NoPutResultSet/CursorResultSet a) I have not really understood the relationship between CursorResultSet and NoPutResultSet. Will there be classes that implement CursorResultSet but not NoPutResultSet, and vice versa? b) Originally CursorResultSet has only get...() methods, getRowLocation() and getCurrentRow(). You have added setRowLocation(), but setCurrentRow() is part of NoPutResultSet. This is a bit assymetric. If you had added setRowLocation to NoPutResultSet you would also have avoided a lot casting by callers. My review of the remaining changes to the SQL layer:
5. TableScanResultSet 5.1 General: a) I do not understand why you need to prevent re-qualification. When will you need to read a row again? I thought you were getting the row from the hash table on later accesses. b) Both the write-up and the added comment only talks about qualification. It does not mention anything about currentRowIsValid which does seem to address another issue. 5.2 getRowLoaction()/getCurrentRow() a) It seems like you have changed the behavior of these functions. Should not the JavaDoc be updated to reflect that? (e.g., when null is returned seems to have changed) 5.3 setRowLocation() a) It seems like the comment relates to the testing on isKeyed. This relation could have been more obvious. b) Thinking about B-Tree scans: What if a query does not need to access the base table only; the index. (That is, all selected columns are part of the index.) How does the re-positioning work in this case? 6. IndexRowToBaseRowResultSet 6.1 positionFromRowLocation a) Same question as for TableScanResultSet, when do you need to refetch the row from the heap? b) I would like a comment that says what concept positionFromRowLocation represents. 6.2 getCurrentRow() a) The diff would have meen much smaller if you had structured this as follows: if (positionFromRowLocation) { <new code> return currentRow; } <old code> b) A comment on why your are testing on positionFromRowLocation would be nice. 7. CurrentOfResultSet 7.1 getNextRowCore() a) I can not find that these changes is listed in the list of code changes in the writeup. b) Is skipping the call to target.getCurrentRow() necessary or just an optimization? c) I do not understand the comment for when to add the warning. Why was there not any need for a warning earlier? d) Typo: coursor 7.2 updateCachedRow()/markCachedRowAsDeleted() a) Isn't the cursor already available as an object attribute? b) Another instance of missing parameter name in Javadoc 8. TemporaryRowHolderResultSet 8.1 All new methods: I think the Javadoc should mention that these methods are no-ops. 9. NoPutResultSetImpl 9.1 Imports: Why do you need to import CursorResultSet? 9.2 setCurrentRow() a) Why have you removed 'final'? I cannot find that you have added any method that overrides this method. b) I guess it makes sense to set its currentRow here, but why was it not necessary before? 9.3 updateCachedRow()/markCachedRowAsDeleted() a) I think the Javadoc should mention that these methods are no-ops. 9.4 setRowLocation() a) Another instance of missing parameter name in Javadoc b) Typo: Positiones c) I do not understand the call to targetResultSet. targetResultSet is as far as I can tell always an InsertResultSet which, according to the javadoc, is used to insert rows from a source into a base table. How is this relevant to what you are doing? 10. NormalizeResultSet 10.1 updateCachedRow() a) Another instance of missing parameter name in Javadoc More review comments. This time for the changes to the access layer:
11. ScanController 11.1 fetchWithoutQualify() a) I think the Javadoc should be more verbose and at least list parameters and contain "@see fetch". I also think it would have been nice to be a bit more verbose on what not applying qualifiers mean. b) I would have placed this new method below fetch 11.2 positionAtRowLocation() a) Why can not clients use the existing reopenScanByRowLocation instead? 12. GenericScanController 12.1 reopenScanByRecordHandleAndSetLocks() a) If I have understood things correct, when a scan is initially opened, the first row is not locked. Locking happen on the subsequent next(). Why could not a similar scheme be used here? That is, reopen positions just before the specified row and a subsequent call to next is performed to actually lock it. Looking at fetchRows() and the methods it calls, there seems to already exist code to handle a repositioned scan. (The combination of SCAN_INIT and a set record posisiton). b) I am still a bit uncomfortable with the method names in the following call path: setRowLocation() positionAtRowLocation() reopenScanByRecordHandleAndSetLocks(). The lower levels makes more happen than the names of the high level methods indicates. 13. Scan/SortBufferRowSource/SortScan 13.1 Javadoc a) Javadoc: I think you should at least put a sentence about what the methods does in addition to referring to the methods of ScanController. At least for some of these classes, that seems to be the pattern that has previously been used. b) I think the Javadoc should say that these methods will always throw exceptions. 14. BTreeScan 14.1 fetch()/fetchWithoutQualify() a) I do not think it is good that both methods has the same Javadoc description. 14.2 positionAtRowLocation () a) See comment 13.1 15. HeapScan 15.1 positionAtRowLocation() a) See 13.1 a) 16. DiskHashTable 16.1 Constructor a) It seems strange to me that you need special handling in order to be able to store RowLocation in the hash tables that is not necessary for any other types. Is the problem the getNewNull() is broken for RowLocation? Maybe that problem could be fixed in stead? > 11.2 positionAtRowLocation()
> > a) Why can not clients use the existing reopenScanByRowLocation > instead? > reopenScanByRowLocation(..) let the user reopen the scan and start scanning from the RowLocation specified. After a call to reopenScanByRowLocation(..) the rowLocation is not locked, the page is not latched, and the user need to call next() to get the next record. This will actually be the next record after the rowLocation specified in reopenScanByRowLocation(). positionAtRowLocation(..) positions the scan, and locks the row. > 12. GenericScanController > > 12.1 reopenScanByRecordHandleAndSetLocks() > > a) If I have understood things correct, when a scan is initially > opened, the first row is not locked. Locking happen on the > subsequent next(). Why could not a similar scheme be used > here? That is, reopen positions just before the specified row > and a subsequent call to next is performed to actually lock > it. Looking at fetchRows() and the methods it calls, there > seems to already exist code to handle a repositioned scan. > (The combination of SCAN_INIT and a set record posisiton). > The combination of SCAN_INIT and a set record position will on the next() call move the rowlocation to the next row, not to the set record position. If you position to a rowlocation which points to a previous row, and call next you may risk: * on the next() call you skip the row if it has been deleted and return another row positionAtRowLocation() returns false if the row has been deleted. > b) I am still a bit uncomfortable with the method names in the > following call path: > setRowLocation() > positionAtRowLocation() > reopenScanByRecordHandleAndSetLocks(). > The lower levels makes more happen than the names of the high > level methods indicates. > Remember these are methods in different modules, and we should try to keep the names of the methods within modules consistent with the module. I.e in SQL execution layer, methods for getting RowLocation is called getRowLocation(). The fact that positionAtRowLocation() also sets locks, is natural in the store. Also next() sets locks. You set locks in a scan without mentioning it in the method names. Here is a new version of the patch and write-up. I have chosen to call both the patch and the write-up v4 so that the numbering will match. I have answered to Øysteins comments from 1 to 16.1.
> 1. ScrollInsensitiveResultSet: > > 1.1 Imports a few classes that are not used (TargetResultSet, SQLBlob, > SQLVarchar). > OK Removed > 1.2 Javadoc for classes: > > a) Should say something about that the hash table may be stored on > disk. OK Added some text to javadoc > b) I miss a description of the layout of the cached rows. > OK Added some text to javadoc > 1.3 getPreviousRow() > > a) I do not understand why this code is added. > positionInLastFetchedRow() is called if currentRow==null, but > currentRow does not seem to be updated by > positionInLastFetchedRow(). Also, it does not seem that the > subsequent call to getRowFromHashTable() depend on any data > that is set by positionInLastFetchedRow(). Not needed: Removed. > > 1.4 getNextRowFromSource() > > a) Comment: I am not able to understand what is going on here > based on the comment: > - "candidate" is not mention anywhere else in this class, > and needs to be defined. > - You mention TableScanResultSet, but I do not find any > assumption in the code about that being the only > possible type of the result set. It is also unclear > whether you are talking about target or source here. > - Why does these ResultSets need to refer to the same row, > and why does it help to set the current row to null? > - It is unclear to me what the relation is between the > target result set and this result set. Is this > explained somewhere? > OK Re-written OK The target result set that we get using target = lcc.lookupCursorActivation(getCursorName()).getTargetResultSet(); Is the result set that is closer to the scan. Usually a TableScanResultSet or IndexRowToBaseRowResultSet. > b) Having to look up something by cursor name, indicates to me > that you increasing the coupling between modules that by design > was not intended to be coupled. Also, this class already has a > reference to an Activation. Is this the same or some other > Activation? Using activation now to get the target result set. > > 1.5 addRowToHashTable() > > a) Javadoc: > - key is no longer positionInSource, but currentPosition. > - typo: "this methods" > - You say that this method "is used in order to store the > new values of that row". I think this is a bit > misleading since this method only adds row to the hash > table; it does not relate to updates. Updates are > achieved by the caller who deletes the old version > before adding the new. > OK Javadoc re-written > b) I do not like the hard-coding of constants for field numbers > and field counts. Changed them into constants. > > c) Why are you recalculating extraColumns every time? Will it not > be the same value for the entire lifetime of the object? OK Made extraColumns as private property in the class and set it in the constructor > > d) Why is there some code in comments for the assignment to > hashRowArray[1]? OK Removed! > > e) I am not quite sure, but would it not be better if position was > a parameter to the function? That would make the dependency on > the calling code more clear. OK, added the position as parameter to addRowToHashTable > > 1.6 getRowFromHashTable() > > a) You are creating an object every time you do a lookup in the > hash table. Would it be an idea to have a single SQLInteger > object that is reused for this purpose? OK Created private variable positionInHashTable. > > b) Disabled code in comments. Any reason why it is not just > removed? OK Removed > > 1.7 positionInLastFetchedRow() > > a) No JavaDoc OK Added javadoc. > > b) Line exceeding 80 columns (not the only case.) OK > > c) Assignment of currentPosition: Could not this be done for all > cases at the end (outside the if statement). OK > > 1.8 updateCachedRow() > > a) Are you sure that if a projection is needed that this will be > done by the top node of the source tree? Could not there be > cases where a projection is needed even if source is of a > different type? (E.g., the source of source is a > ProjectRestrictResultSet) The types of result sets you can get in updatable result sets seems to be reduced, since there are restrictions to the type of query that can give an updatable result set. I have not been able to find a case where ProjectRestrict is not the top one. The other types that we have seen in this context are TableScan and IndexRowToBaseRow which would logically come under a ProjectRestrict. > > b) Javadoc: Parameter name is missing (not the only case). OK Found two places. > > 1.9 isDeleted()/isUpdated() > > a) It does not seem to be necessary to cast the objects stored in > the hash table to DataValueDescriptor[] in order to access its > contents. (Not doing so, could shorten some lines that are too > long :-) OK > > 1.10 setRowLocation() > > a) I am not entirely happen with the name. It sounds more like an > attribute is being updated than that the underlying cursor is > repositioned. OK. Renamed to positionScanAtRowLocation(). > > b) Javadoc: Parameter name is missing (not the only case). OK > > c) Would not an assert be better than if-test in this case? It > seems to me that the code depends on the source being a > CursorResultSet. OK Not an issue longer, moved to NoPutResultSet. > > 2. UpdateResultSet/DeleteResultSet > > a) Why is updateCachedRow()/markCachedRowAsDeleted() made general > methods on NoPutResultSet? That requires touching a lot of > classes not concerned with scrollable cursors. Would it not be > possible to detect that the source is a > ScrollInsensitiveResultSet, and do the necessary casting in this > case. If you insist on making these general methods, I would > suggest calling them updateRow/markRowAsDeleted and let each > ResultSet do whatever it needs to do when rows are > updated/deleted. ScrollInsensitiveResultSet is never the source, but the source of the source ... of the source. I have renamed the two methods as you suggested. > > b) Is there a reason why you place the calls in open() and not in > collectAffectedRows()? I have moved the calls to collectAffectedRows() and update/deleteDefferedRows(). I had called them in open so that I would only need to call them once for each update, but if later other types of result sets need to use these methods it is better that they are called after each updated row instead of only once. For positioned updated, it would only be one affected row, so it would not be a problem to call the methods from open(). > > c) You make the assumption that only a single row is affected. > This may be the case for scrollable cursors, but generally > open() may update/delete many rows. I think that needs to be > explained in the code. For SUR we are only interested in positioned updates which would result in one row being updated, but I have moved the method calls so that they can be used for other types of result sets later if necessary. > > 3. ProjectRestrictResultSet > > 3.1 setRowlocation() > > a) Javadoc: Parameter name is missing (not the only case). Changed to see NoPutResultSet.setRowLocation and added specific comments to those implementations that needed it. > b) Assert instead of if-test? Not needed anymore since it is a part of NoPutResultSet. > 3.2 doBaseRowProjection() > > a) Javadoc: Parameter name is missing (not the only case). OK > b) The test for (source!=null) is not necessary. Agree. Changed. > > 4. NoPutResultSet/CursorResultSet > > a) I have not really understood the relationship between > CursorResultSet and NoPutResultSet. Will there be classes that > implement CursorResultSet but not NoPutResultSet, and vice > versa? There are result sets that are NoPut but not Cursor, the opposite does not happen. > b) Originally CursorResultSet has only get...() methods, > getRowLocation() and getCurrentRow(). You have added > setRowLocation(), but setCurrentRow() is part of NoPutResultSet. > This is a bit assymetric. If you had added setRowLocation to > NoPutResultSet you would also have avoided a lot casting by > callers. Moved to NoPutResultSet. >5. TableScanResultSet > >5.1 General: > > a) I do not understand why you need to prevent re-qualification. > When will you need to read a row again? I thought you were > getting the row from the hash table on later accesses. Right before an positioned update/delete is performed, CurrentOfResultSet is used to get the current row. This will re-read the row, and re-qualify before it is updated. That's the moment when we do not want to re-qualify, since changes can have been done to the row that makes it no longer qualify and we don't want that to happen, since according to our chosen semantics for insensitive result sets, rows will remain in the result, in the same place, even if they no longer qualify according to the original query. > b) Both the write-up and the added comment only talks about > qualification. It does not mention anything about > currentRowIsValid which does seem to address another issue. currentRowIsValid is set to the result of positioning at the rowLocation, if positioning at rowLocation returs false, the row has been deleted under our feet. Whenenver currentRowIsValid is false it means that the row has been deleted. Write-up updated. >5.2 getRowLoaction()/getCurrentRow() > > a) It seems like you have changed the behavior of these functions. > Should not the JavaDoc be updated to reflect that? (e.g., when > null is returned seems to have changed) OK The javadoc was not updated from before for getCurrentRow(). Updated both javadocs. >5.3 setRowLocation() > > a) It seems like the comment relates to the testing on isKeyed. > This relation could have been more obvious. OK. Changed the comment. > b) Thinking about B-Tree scans: What if a query does not need to > access the base table only; the index. (That is, all selected > columns are part of the index.) How does the re-positioning > work in this case? The plan is to remove that optimization if the result set is of type scrollable insensitive updatable. This seems to also be a problem for forward-only result set, jira issue >6. IndexRowToBaseRowResultSet > >6.1 positionFromRowLocation > > a) Same question as for TableScanResultSet, when do you need to > refetch the row from the heap? Same answer as for 5.1 > b) I would like a comment that says what concept > positionFromRowLocation represents. Ok, comment added. >6.2 getCurrentRow() > > a) The diff would have meen much smaller if you had structured > this as follows: > > if (positionFromRowLocation) { > <new code> > return currentRow; > } > <old code> OK > b) A comment on why your are testing on positionFromRowLocation > would be nice. OK >7. CurrentOfResultSet > >7.1 getNextRowCore() > > a) I can not find that these changes is listed in the list of code > changes in the writeup. write-up updated. > b) Is skipping the call to target.getCurrentRow() necessary or > just an optimization? Not necessary, but if the rowLocation is NULL we know that the row has been deleted. > c) I do not understand the comment for when to add the warning. > Why was there not any need for a warning earlier? The cursor result set for a forward-only updatable result set will be sensitive to changes. The warning was not needed before because if the row had been deleted under our feet, the cursor result set would return NULL for cursorRow and an exception would be thrown. Now that the cursor result set is a ScrollInsensitiveResultSet, if a row is deleted under our feet, the ScrollInsensitiveResultSet will still return the cached row from the hash table (not NULL) while the target result set will return a currentRow that is NULL, this is the situation that causes the warning. > d) Typo: coursor OK >7.2 updateCachedRow()/markCachedRowAsDeleted() > > a) Isn't the cursor already available as an object attribute? Yes, OK. > b) Another instance of missing parameter name in Javadoc OK >8. TemporaryRowHolderResultSet > >8.1 All new methods: I think the Javadoc should mention that these > methods are no-ops. Ok >9. NoPutResultSetImpl > >9.1 Imports: Why do you need to import CursorResultSet? OK. Removed >9.2 setCurrentRow() > > a) Why have you removed 'final'? I cannot find that you have > added any method that overrides this method. OK. Put final back. > b) I guess it makes sense to set its currentRow here, but why was > it not necessary before? The change was done so that from now on we do not need to do this: currentRow = XXX; setCurrentRow(XXX); you can just do: setCurrentRow(XXX); >9.3 updateCachedRow()/markCachedRowAsDeleted() > > a) I think the Javadoc should mention that these methods are > no-ops. > OK >9.4 setRowLocation() > > a) Another instance of missing parameter name in Javadoc OK > b) Typo: Positiones OK > c) I do not understand the call to targetResultSet. > targetResultSet is as far as I can tell always an > InsertResultSet which, according to the javadoc, is used to > insert rows from a source into a base table. How is this > relevant to what you are doing? Never reached, removed. >10. NormalizeResultSet > >10.1 updateCachedRow() > > a) Another instance of missing parameter name in Javadoc > OK >11. ScanController > >11.1 fetchWithoutQualify() > > a) I think the Javadoc should be more verbose and at least list > parameters and contain "@see fetch". I also think it would > have been nice to be a bit more verbose on what not applying > qualifiers mean. OK. Added more comments. > b) I would have placed this new method below fetch OK. Moved >11.2 positionAtRowLocation() > > a) Why can not clients use the existing reopenScanByRowLocation > instead? Part of 1067 now, Andreas will comment on it. >12. GenericScanController > >12.1 reopenScanByRecordHandleAndSetLocks() > > a) If I have understood things correct, when a scan is initially > opened, the first row is not locked. Locking happen on the > subsequent next(). Why could not a similar scheme be used > here? That is, reopen positions just before the specified row > and a subsequent call to next is performed to actually lock > it. Looking at fetchRows() and the methods it calls, there > seems to already exist code to handle a repositioned scan. > (The combination of SCAN_INIT and a set record posisiton). > > b) I am still a bit uncomfortable with the method names in the > following call path: > setRowLocation() > positionAtRowLocation() > reopenScanByRecordHandleAndSetLocks(). > The lower levels makes more happen than the names of the high > level methods indicates. Part of 1067 now, Andreas will comment on it. >13. Scan/SortBufferRowSource/SortScan > >13.1 Javadoc > > a) Javadoc: I think you should at least put a sentence about what > the methods does in addition to referring to the methods of > ScanController. At least for some of these classes, that > seems to be the pattern that has previously been used. OK. Added a line about what it is supposed to do even though not implemented > b) I think the Javadoc should say that these methods will always > throw exceptions. OK. Added a line that the method always throws exception >14. BTreeScan > >14.1 fetch()/fetchWithoutQualify() > > a) I do not think it is good that both methods has the same > Javadoc description. OK. Specified on WithoutQualify that it does not apply qualifiers. > >14.2 positionAtRowLocation () > > a) See comment 13.1 > Part of 1067 now, Andreas will comment on it. >15. HeapScan > >15.1 positionAtRowLocation() > > a) See 13.1 a) Part of 1067 now, Andreas will comment on it. >16. DiskHashTable > >16.1 Constructor > > a) It seems strange to me that you need special handling in order > to be able to store RowLocation in the hash tables that is not > necessary for any other types. Is the problem the > getNewNull() is broken for RowLocation? Maybe that problem > could be fixed in stead? OK. Fixed getNewNUll() for HeapRowLocation. I have found some more information to complement my answer to 1.4. I also decided to keep the way to get the targetResultSet in ScrollInsensitiveResultSet.java equal to how it is done in CurrentOfResultSet (1.4.b), so I am attaching a new patch.
From ActivationClassBuilder.java line 350: /** * Updatable cursors * need to add a field and its initialization * for use in BaseActivation to access the result set that * identifies target rows for a positioned update or delete. * <p> * The code that is generated is: * <pre><verbatim> * private CursorResultSet targetResultSet; * * </verbatim></pre> * * The expression that is generated is: * <pre><verbatim> * (ResultSet) (targetResultSet = (CursorResultSet) #expression#) * </verbatim></pre> * */ And from line 309: /** * Updatable cursors * need to add a getter method for use in BaseActivation to access * the result set that identifies target rows for a positioned * update or delete. * <p> * The code that is generated is: * <pre><verbatim> * public CursorResultSet getTargetResultSet() { * return targetResultSet; * } * * public CursorResultSet getCursorResultSet() { * return cursorResultSet; * } * </verbatim></pre> * */ Thanks for addressing my comments. I have only looked at code that is
related to earlier comments. If other code has been added in the later patches, please, point me to it. Basically, the changes looks good, but I still have a few questions which is probably more about my understanding of this. Fernanda Pizzorno (JIRA) wrote: >> 1. ScrollInsensitiveResultSet: >> I would like Javadoc for all the attributes you have added to the class. >> 1.4 getNextRowFromSource() >> >> a) Comment: I am not able to understand what is going on here >> based on the comment: >> - "candidate" is not mention anywhere else in this class, >> and needs to be defined. >> - You mention TableScanResultSet, but I do not find any >> assumption in the code about that being the only >> possible type of the result set. It is also unclear >> whether you are talking about target or source here. >> - Why does these ResultSets need to refer to the same row, >> and why does it help to set the current row to null? >> - It is unclear to me what the relation is between the >> target result set and this result set. Is this >> explained somewhere? >> > > OK Re-written > > OK The target result set that we get using > target = lcc.lookupCursorActivation(getCursorName()).getTargetResultSet(); > Is the result set that is closer to the scan. Usually a TableScanResultSet > or IndexRowToBaseRowResultSet. > This confuses me. I thought source was the result set closer to the scan. At least, it is where you get the rows from. Or are you saying that the target may be closer to the scan than the source? Why is then called target? If I understand you correctly, getTargetResultSet does not return a TargetResultSet. This naming is very confusing. (I know it is not your fault.) If I understand the comments from ActivationClassBuilder correctly, the target result is the result set used for position updates. In that case, I do not understand why you need to change the position of the source result set when repositioning. It seems to me that is only used to populate the hash table. > >> b) Having to look up something by cursor name, indicates to me >> that you increasing the coupling between modules that by design >> was not intended to be coupled. Also, this class already has a >> reference to an Activation. Is this the same or some other >> Activation? > > Using activation now to get the target result set. I see you reverted this in the latest patch. Why cannot the local activation be used? Is it not the same activation? > >> 1.5 addRowToHashTable() >> >> a) Javadoc: >> - key is no longer positionInSource, but currentPosition. >> - typo: "this methods" >> - You say that this method "is used in order to store the >> new values of that row". I think this is a bit >> misleading since this method only adds row to the hash >> table; it does not relate to updates. Updates are >> achieved by the caller who deletes the old version >> before adding the new. >> > > OK Javadoc re-written You have not adressed the last comment. > >> b) I do not like the hard-coding of constants for field numbers >> and field counts. > > Changed them into constants. Good, but there are still some hard-coding in other methods. I also think you can remove the comments on the assignments since using constants the code becomes self-explanatory. >> 1.7 positionInLastFetchedRow() >> >> a) No JavaDoc > > OK Added javadoc. Typos: Positiones, proviously, >> 1.8 updateCachedRow() >> >> a) Are you sure that if a projection is needed that this will be >> done by the top node of the source tree? Could not there be >> cases where a projection is needed even if source is of a >> different type? (E.g., the source of source is a >> ProjectRestrictResultSet) > > The types of result sets you can get in updatable result sets seems to be > reduced, since there are restrictions to the type of query that can give > an updatable result set. I have not been able to find a case where > ProjectRestrict is not the top one. The other types that we have seen in > this context are TableScan and IndexRowToBaseRow which would logically > come under a ProjectRestrict. This might well be the case. Is there some way of verifying this? And how easy will it be to detect the problem if new "top nodes" are introduced? And would it require a major rewrite to handle that? > >> b) Javadoc: Parameter name is missing (not the only case). > > OK Found two places. I have not verified that this is fixed everywhere for the latest patch. Could you please, generate javadoc and check warnings? >> 5. TableScanResultSet >> >> 5.1 General: >> >> a) I do not understand why you need to prevent re-qualification. >> When will you need to read a row again? I thought you were >> getting the row from the hash table on later accesses. > > Right before an positioned update/delete is performed, CurrentOfResultSet > is used to get the current row. This will re-read the row, and re-qualify > before it is updated. That's the moment when we do not want to re-qualify, > since changes can have been done to the row that makes it no longer > qualify and we don't want that to happen, since according to our chosen > semantics for insensitive result sets, rows will remain in the result, in > the same place, even if they no longer qualify according to the original > query. I still does not see how this impact the insensitivity since I would think the row will still be present in the hash table. Will not the update be performed if the row does not qualify? How does the CurrentOfResultSet refetch the current row? Does it involve the ScrollInsensitiveResultSet at all in order to get it? > >> b) Both the write-up and the added comment only talks about >> qualification. It does not mention anything about >> currentRowIsValid which does seem to address another issue. > > currentRowIsValid is set to the result of positioning at the rowLocation, > if positioning at rowLocation returs false, the row has been deleted > under our feet. Whenenver currentRowIsValid is false it means that the > row has been deleted. > > Write-up updated. Please, add a comment to the declaration of the attribute, too. >> 5.3 positionScanAtRowLocation() (was: setRowLocation) NEW: c) I do not understand the purpose of resumeRead. The description in JavaDoc (NoPutResultSet) is a bit confusing. Is there some way to make this a bit more general to other types of NoPutResultSet? >> 6. IndexRowToBaseRowResultSet >> >> 6.1 positionFromRowLocation >> >> b) I would like a comment that says what concept >> positionFromRowLocation represents. > > Ok, comment added. Typo: otherwite >> 7. CurrentOfResultSet >> >> 7.1 getNextRowCore() >> >> b) Is skipping the call to target.getCurrentRow() necessary or >> just an optimization? > > Not necessary, but if the rowLocation is NULL we know that the row > has been deleted. But is it worth the extra complexity? > >> c) I do not understand the comment for when to add the warning. >> Why was there not any need for a warning earlier? > > The cursor result set for a forward-only updatable result set will > be sensitive to changes. The warning was not needed before because > if the row had been deleted under our feet, the cursor result set > would return NULL for cursorRow and an exception would be > thrown. Now that the cursor result set is a > ScrollInsensitiveResultSet, if a row is deleted under our feet, the > ScrollInsensitiveResultSet will still return the cached row from the > hash table (not NULL) while the target result set will return a > currentRow that is NULL, this is the situation that causes the > warning. I think my problem is that I have not quite understood what a target result set is, and especially the reasoning behind its name. >> 8. TemporaryRowHolderResultSet >> >> 8.1 All new methods: I think the Javadoc should mention that these >> methods are no-ops. > > Ok > Not done for positionScanAtRowLocation. >> 13. Scan/SortBufferRowSource/SortScan >> >> 13.1 Javadoc >> >> a) Javadoc: I think you should at least put a sentence about what >> the methods does in addition to referring to the methods of >> ScanController. At least for some of these classes, that >> seems to be the pattern that has previously been used. > > OK. Added a line about what it is supposed to do even though not implemented > Not addressed in Scan. >> b) I think the Javadoc should say that these methods will always >> throw exceptions. > > OK. Added a line that the method always throws exception I would prefer that it also mentioned the SQLState for the exception. Here are my review comments to the JDBC changes. Generally, there are
also in this part of the code several Javdoc-@param without parameter name. 17. EmbedResultSet 17.1 rowUpdated()/rowDeleted() a) Why is checkIfClosed removed? 17.2 addWarning() a) I would like javadoc for this method 18. EmbedDatabaseMetaData 18.1 othersUpdatesAreVisible() a) The added comment was placed a bit strange. Since it supposed to cover all three methods, either put it above the Javadoc for the first method, or duplicate withing Javadoc for all. 19. EmbedConnection 19.1 setResultSetConncurrency() a) Is there any point in keeping this method around? It just returns one of it input parameters. 20. Added SqlState a) Is it necessary to prefix the message with "WARNING:"? I would think that would be possible to determined from the context. If kept, capitalize after colon. b) According to the ODBC guide I have access to, this SqlState is for "A positioned UPDATE or a positioned DELETE SQL statement was specified, and no row or more than one row was updated or deleted." As far as I can tell, you will also use this warning when one reposition to a deleted row. Is that according to the spec? c) For the text of this message you have used the "subcondition" specified in the SQL spec. Is that a requirement? I thought the spec only put a requirement on the state and not the associated text. I think something more descriptive, like what I found in the ODBC guide would be helpful to users. Thank you for looking into the patch Øystein. Here is a new patch a answer to Øystein's comments.
>>>1. ScrollInsensitiveResultSet: >>> > >I would like Javadoc for all the attributes you have added to the >class. OK. Added comment for all new attributes. >>>1.4 getNextRowFromSource() >>> >>> a) Comment: I am not able to understand what is going on here >>> based on the comment: >>> - "candidate" is not mention anywhere else in this class, >>> and needs to be defined. >>> - You mention TableScanResultSet, but I do not find any >>> assumption in the code about that being the only >>> possible type of the result set. It is also unclear >>> whether you are talking about target or source here. >>> - Why does these ResultSets need to refer to the same row, >>> and why does it help to set the current row to null? >>> - It is unclear to me what the relation is between the >>> target result set and this result set. Is this >>> explained somewhere? >>> >>OK Re-written > > >>OK The target result set that we get using >>target = lcc.lookupCursorActivation(getCursorName()).getTargetResultSet(); >>Is the result set that is closer to the scan. Usually a TableScanResultSet >>or IndexRowToBaseRowResultSet. >> > >This confuses me. I thought source was the result set closer to the >scan. At least, it is where you get the rows from. Or are you saying >that the target may be closer to the scan than the source? Why is then >called target? If I understand you correctly, getTargetResultSet does >not return a TargetResultSet. This naming is very confusing. (I know >it is not your fault.) > >If I understand the comments from ActivationClassBuilder correctly, >the target result is the result set used for position updates. In >that case, I do not understand why you need to change the position of >the source result set when repositioning. It seems to me that is only >used to populate the hash table. We have a hierarchy of result sets of different types. When the top result set needs to get a new row, it will get it from its source (immediately under in the hierarchy), which will get it from its source ... and so on, until the result set at the bottom of the hierarchy fetches the row from the scan. The target result set is the one that is at the bottom of the hierarchy, but if a result set accesses its source result set which accesses its source ... and so on, it will end up indirectly accessing the target result set. >>> b) Having to look up something by cursor name, indicates to me >>> that you increasing the coupling between modules that by design >>> was not intended to be coupled. Also, this class already has a >>> reference to an Activation. Is this the same or some other >>> Activation? >> >>Using activation now to get the target result set. > > >I see you reverted this in the latest patch. Why cannot the local >activation be used? Is it not the same activation? > Yes it is. I had forgotten to check for updatable, that's why I removed it. Read-only result sets do not have a target. > >>>1.5 addRowToHashTable() >>> >>> a) Javadoc: >>> - key is no longer positionInSource, but currentPosition. >>> - typo: "this methods" >>> - You say that this method "is used in order to store the >>> new values of that row". I think this is a bit >>> misleading since this method only adds row to the hash >>> table; it does not relate to updates. Updates are >>> achieved by the caller who deletes the old version >>> before adding the new. >>> >>OK Javadoc re-written > > > >You have not adressed the last comment. > OK, re-written once more. > >>> b) I do not like the hard-coding of constants for field numbers >>> and field counts. >> >>Changed them into constants. > > >Good, but there are still some hard-coding in other methods. OK. I believe I got all of them now. >I also think you can remove the comments on the assignments since >using constants the code becomes self-explanatory. OK. Removed >>>1.7 positionInLastFetchedRow() >>> >>> a) No JavaDoc >> >>OK Added javadoc. > > >Typos: Positiones, proviously, OK. Corrected >>>1.8 updateCachedRow() >>> >>> a) Are you sure that if a projection is needed that this will be >>> done by the top node of the source tree? Could not there be >>> cases where a projection is needed even if source is of a >>> different type? (E.g., the source of source is a >>> ProjectRestrictResultSet) >> >>The types of result sets you can get in updatable result sets seems to be >>reduced, since there are restrictions to the type of query that can give >>an updatable result set. I have not been able to find a case where >>ProjectRestrict is not the top one. The other types that we have seen in >>this context are TableScan and IndexRowToBaseRow which would logically >>come under a ProjectRestrict. > > >This might well be the case. Is there some way of verifying this? >And how easy will it be to detect the problem if new "top nodes" are >introduced? And would it require a major rewrite to handle that? If we added the doBaseRowProjection as a public method in all NoPutResultSet, and those that are not of type ProjectRestrictRS would return the results of the execution of this method at their source result set, the ProjectRestrictRS would no longer need to be the top one. I do not think that it is a major rewrite. >>> b) Javadoc: Parameter name is missing (not the only case). >> >>OK Found two places. > > >I have not verified that this is fixed everywhere for the latest >patch. Could you please, generate javadoc and check warnings? Done. Fixed all warnings concerning this patch. > >>>5. TableScanResultSet >>> >>>5.1 General: >>> >>> a) I do not understand why you need to prevent re-qualification. >>> When will you need to read a row again? I thought you were >>> getting the row from the hash table on later accesses. >> >>Right before an positioned update/delete is performed, CurrentOfResultSet >>is used to get the current row. This will re-read the row, and re-qualify >>before it is updated. That's the moment when we do not want to re-qualify, >>since changes can have been done to the row that makes it no longer >>qualify and we don't want that to happen, since according to our chosen >>semantics for insensitive result sets, rows will remain in the result, in >>the same place, even if they no longer qualify according to the original >>query. > > >I still does not see how this impact the insensitivity since I would >think the row will still be present in the hash table. Will not the >update be performed if the row does not qualify? > >How does the CurrentOfResultSet refetch the current row? Does it >involve the ScrollInsensitiveResultSet at all in order to get it? There are several levels of ResultSets in between Update/DeleteResultSet and CurrentOfResultSet, but the row to be updated/deleted comes from a call to CurrentOfResultSet.getNextRowCore(). CurrentOfResultSet.getNextRowCore() will read a cursorRow and a currentRow. The cursorRow comes from the CursorResultSet which in our case is a ScrollInsensitiveResultSet, and currentRow comes from the TargetResultSet which can be TableScan or IndexRowToBaseRow RS. CurrentOfResultSet uses to cursorRow to throw an exception if the cursor is not on a row and the current row is the one returned to the result set on top of CurrentOfResultSet. The hierarchy will usually be: 1. Update/DeleteRS 2. DMLWriteRS 3. ProjectRestrictRS 4. CurrentOfRS The CurrentOfRS will return the currentRow from its targetRS. ProjectRestrictRS will use the returned row returned by CurrentOfRS to build a new row with the information that is necessary for Update/DeleteRS to update/delete the row. DMLWrite does not do anything to the row, just returns it to Update/DeleteRS. If the row has been updated to values that not longer qualify, for example, if I have a result set generated with the following statement: "SELECT a, b FROM t1 WHERE a < 5", and I run the following code: rs.next() rs.updateInt(1, 10); rs.updateRow(); rs.next(); rs.previous(); rs.updateInt(1, 15); rs.updateRow(); when we get to the second update row, the value of a in the database and result set will be 10, so it will be > 5 and therefore not qualify. When UpdateResultSet gets the row to be updated, the targetResultSet on the CurrentOfResultSet, will return null as current row, because the current row has a > 10 and no longer qualifies. That will make the CurrentOfResultSet return null as currentRow and the row won't be updated, instead the application will get a "Cursor operation conflict" warning. It is our design choice that once the row has been a part of the result set, it will continue to be a part of it even though its values are changed so that they no longer qualify. We chose not to have update holes in the result set, and therefore, we need to keep this row in the result set even though a > 10. That's why we want to result set to qualify only when getting a new row and not to qualify again the getting the current row. >>> b) Both the write-up and the added comment only talks about >>> qualification. It does not mention anything about >>> currentRowIsValid which does seem to address another issue. >> >>currentRowIsValid is set to the result of positioning at the rowLocation, >>if positioning at rowLocation returs false, the row has been deleted >>under our feet. Whenenver currentRowIsValid is false it means that the >>row has been deleted. >> >>Write-up updated. > > >Please, add a comment to the declaration of the attribute, too. OK. Comment added >>>5.3 positionScanAtRowLocation() (was: setRowLocation) > > >NEW: c) I do not understand the purpose of resumeRead. The >description in JavaDoc (NoPutResultSet) is a bit confusing. Is there >some way to make this a bit more general to other types of >NoPutResultSet? Removed. Moved the logic to TableScanResultSet instead. >>>6. IndexRowToBaseRowResultSet >>> >>>6.1 positionFromRowLocation >>> >>> b) I would like a comment that says what concept >>> positionFromRowLocation represents. >> >>Ok, comment added. > > >Typo: otherwite Ok, corrected. >>>7. CurrentOfResultSet >>> >>>7.1 getNextRowCore() >>> >>> b) Is skipping the call to target.getCurrentRow() necessary or >>> just an optimization? >> >>Not necessary, but if the rowLocation is NULL we know that the row >>has been deleted. > > >But is it worth the extra complexity? Ok. Removed > >>> c) I do not understand the comment for when to add the warning. >>> Why was there not any need for a warning earlier? >> >>The cursor result set for a forward-only updatable result set will >>be sensitive to changes. The warning was not needed before because >>if the row had been deleted under our feet, the cursor result set >>would return NULL for cursorRow and an exception would be >>thrown. Now that the cursor result set is a >>ScrollInsensitiveResultSet, if a row is deleted under our feet, the >>ScrollInsensitiveResultSet will still return the cached row from the >>hash table (not NULL) while the target result set will return a >>currentRow that is NULL, this is the situation that causes the >>warning. > > >I think my problem is that I have not quite understood what a target >result set is, and especially the reasoning behind its name. > > >>>8. TemporaryRowHolderResultSet >>> >>>8.1 All new methods: I think the Javadoc should mention that these >>> methods are no-ops. >> >>Ok >> > >Not done for positionScanAtRowLocation. Done. >>>13. Scan/SortBufferRowSource/SortScan >>> >>>13.1 Javadoc >>> >>> a) Javadoc: I think you should at least put a sentence about what >>> the methods does in addition to referring to the methods of >>> ScanController. At least for some of these classes, that >>> seems to be the pattern that has previously been used. >> >>OK. Added a line about what it is supposed to do even though not implemented >> > >Not addressed in Scan. Added javadoc to GenericScanController too. > >>> b) I think the Javadoc should say that these methods will always >>> throw exceptions. >> >>OK. Added a line that the method always throws exception > > >I would prefer that it also mentioned the SQLState for the exception. Ok >17. EmbedResultSet > >17.1 rowUpdated()/rowDeleted() > > a) Why is checkIfClosed removed? Probably a merge problem since checkIfClosed() was commited after the first patch was submitted. >17.2 addWarning() > > a) I would like javadoc for this method Added. >18. EmbedDatabaseMetaData > >18.1 othersUpdatesAreVisible() > > a) The added comment was placed a bit strange. Since it supposed > to cover all three methods, either put it above the Javadoc > for the first method, or duplicate withing Javadoc for all. Agreed. Moved above the Javadoc for the first method. >19. EmbedConnection > >19.1 setResultSetConncurrency() > > a) Is there any point in keeping this method around? It just > returns one of it input parameters. Removed. >20. Added SqlState > > a) Is it necessary to prefix the message with "WARNING:"? I would > think that would be possible to determined from the context. > If kept, capitalize after colon. Not really necessary. > b) According to the ODBC guide I have access to, this SqlState is > for "A positioned UPDATE or a positioned DELETE SQL statement > was specified, and no row or more than one row was updated or > deleted." As far as I can tell, you will also use this warning > when one reposition to a deleted row. Is that according to the > spec? The warning is added in CurrentOfResultSet. This result set is only used for positioned update/delete operations. Therefore you will not get a warning unless you attempt to delete/update a previously deleted row either by a positioned update/delete or by calling ResultSet.updateRow() or ResultSet.deleteRow() methods. > c) For the text of this message you have used the "subcondition" > specified in the SQL spec. Is that a requirement? I thought > the spec only put a requirement on the state and not the > associated text. I think something more descriptive, like what > I found in the ODBC guide would be helpful to users. Message changed to: "An attempt to update or delete an already deleted row was made: no row was updated or deleted." Thank you very much for the explanation on CurrentOfResultSet works
and interacts with other result sets. It would have been really nice to have had this (preferrably together with a figure), when I started reviewing the patch. One tiny detail. In the new message, I suggest the word after colon be capiltalized: > Message changed to: "An attempt to update or delete an already deleted row > was made: no row was updated or deleted." All my comments so far has been addressed. I plan to do a quick walk-through of the test changes. Thank you for reviewing the patch Øystein. I am attaching a new patch that addresses Øystein's last comment.
Øystein wrote: [...] One tiny detail. In the new message, I suggest the word after colon be capiltalized: > Message changed to: "An attempt to update or delete an already deleted row > was made: no row was updated or deleted." [...] I have changed the message from "... no row was updated or deleted" to "... No row was updated or deleted". I have successfully run derbyall. >Comment by Øystein Grøvlen [20/Mar/06 10:15 AM]
>All my comments so far has been addressed. I plan to do a quick >walk-through of the test changes. I have gone through the test changes, and they are ok. The tests for SUR have already been committed, and the changes in The patch has been reveiewed and all comments have been addressed. Can someone please commit this patch?
Thank you in advance! The patch has been reveiewed and all comments have been addressed. Can someone please commit this patch?
Thank you in advance! New patch in which we solved the merge conflicts.
Committed revision 389202.
Toggling off the "release note needed" flag since I'm already tracking this for the Release Notes and expect to cull a description from the 10.2 snapshot wiki.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Minor
Add scrollable, updatable, insensitive result sets
