Here are answers to Øysteins comments and questions.
A new version of the patch (#3) reflecting those answers has been
uploaded. In this version, some too long lines added or changed have
also been normalized (<= 80 chars wide). I also fixed added or changed
lines to comply with surrounding whitespace styles (tab or not).
I ran derbyall using Solaris 10/x86 Sun JRE 1.4.2 with no untoward
>>>>> "Øystein" == Øystein Grøvlen (JIRA) <firstname.lastname@example.org> wrote:
Øystein> 1. metadata_net.properties:
Øystein> - Is the format and constant values used here documented anywhere?
As part of
DERBY-965, I added some comments explaining the syntax of
the fields I touched, please see the file from line 197 onwards. I
am not aware of any other documentation.
Øystein> 2. DRDAConnThread
Øystein> - parseSQLATTR(): Variable insensitive can be removed entirely. It
Øystein> serves no purpose anymore.
Øystein> 3. NetResultSet/NetResultSet40
Øystein> - Parameters of constructor has comments with possible values. This
Øystein> needs to be updated to include QRYSNSSTC.
Øystein> 4. NetCursor
Øystein> - The writeup says the purpose is to pop warnings, but the code seems
Øystein> to only handle a single warning.
Currently, chained warnings are not being sent over DRDA. I know
Fernanda is working on a patch for that. Eventually is will be a
"pop", right now maximum one warning is being transmitted.
Øystein> - Why do you have to delay the call to setIsUpdataDeleteHole until
Øystein> later and not do it when testing for warnings?
The existing code checked for update/delete holes by relying only
nulldata, which is parsed later than the warning. For SUR, in addition
to nulldata, we send a warning (SQLState.ROW_DELETED) since this is
required by DRDA, cf the write-up section on this. The existing code
is probably non-compliant. Rather than calling setIsUpdataDeleteHole()
in two places, we chose to use the existing code location for calling
it, using the helper variable receivedDeleteHoleWarning in the SUR
case. I agree the code could be simplified by removing the old logic
(relying only on nulldata), but we chose the conservative approach.
Øystein> 5. NetStatementReply
Øystein> - Call to ClientJDBCObjectFactory.newNetResultSet() lists possible
Øystein> values. This needs to be updated to include QRYSNSSTC.
Øystein> 6. Statement
Øystein> - Why have you made warnings_ protected? It seems like it is package
Øystein> private that is needed. Why not keep it private and supply a
Agreed, done. The old code is too not good on encapsulation, so it
leads one astray Introduced a protected method
Statement#getWarnings_, an accessor for warnings_, which is now
Øystein> 7. ResultSet
Øystein> - Why have you made suggestedFetchSize_ public? It seems like it
Øystein> could be protected.
Yes. I made it protected.
Øystein> - Comments for fetchSize_ and suggestedFetchSize_ are a bit brief.
Expanded those comments, included here for your convenience:
// Gets its initial value from the statement when the result set is created.
// It can be modified by setFetchSize and retrieved via getFetchSize.
protected int suggestedFetchSize_;
// Set by the net layer based on suggestedFetchSize_, protocol
// type, scrollability and presence of lobs.
public int fetchSize_;
Øystein> (And if they are to be public, you should supply javadoc).
Øystein> - relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for
Øystein> all result sets?
For SUR, the check is performed by the getAbsoluteRowset.
Øystein> - relativex(): It seems like the code to call getAbsoluteResultSet is
Øystein> duplicated. I do not think the second call will ever be executed.
Yes, you are right. Vestige from a re-write. I removed this.
Øystein> - rowUpdated(): Why not always call getIsRowUpdated? I would
Øystein> think it was valid for all result sets.
This method used to return false for all result sets, since
detectability was not implemented in Derby (for any result set). This
patch implements it for SUR, so getIsRowUpdated is only called for
SUR. I changed this as you suggest, making sure it will return false
in non-SUR cases.
Øystein> - rowUpdated()/rowDeleted(): Thss methods are a bit asymmetric.
Øystein> rowUpdated() calls a get-method while rowDeleted accesses
Øystein> isUpdateDeleteHole_ directly.
The old code had the isUpdateDeleteHole as "public" and I tried to
make patch as small as possible, but I agree it's asymmetric. I
changed the access into using a getter method.
Øystein> - updateRow(): I tried to browse the spec and the javadoc and I did
Øystein> not find anything that said that there is a difference
Øystein> between forward only cursors and scrollable cursors with respect to
Øystein> validity of the current position after and update. Where is this
This is not mandated by JDBC. I remember asking Dan about it once, but
I don't rememeber the rationale for making forward only cursors lose
the position after an updateRow. I checked the SQL 2003 standard just
now, but I could not find it is a requirement for positioned update,
Øystein> - checkForUpdatableResultSet(): What is the difference between this
Øystein> new method and the existing checkUpdatableCursor()? It seems better
Øystein> to change the existing than to make a new method.
Merged these methods into one and updated masters to match the changed
(and better) exceptions.