Project

General

Profile

Bug #5452

eliminate use of ResultSet.CONCUR_UPDATABLE?

Added by Eric Faulhaber about 5 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Ștefan Roman
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
production:
No
env_name:
topics:

History

#1 Updated by Eric Faulhaber about 5 years ago

Ovidiu, while debugging a few days ago, I came across this in SQLQuery.scroll(Session, int, boolean, List<RowStructure>):

      int readOnlyMode = readOnly ? ResultSet.CONCUR_READ_ONLY : ResultSet.CONCUR_UPDATABLE;

I know we carried the readOnly parameter over from the pre-4011 implementation, but this rarely will be set to true, so we almost always will use CONCUR_UPDATABLE. AFAIK, we never update records directly from a ResultSet using the updateXXXX methods, so I think we can drop the readOnly parameter altogether and always make result sets CONCUR_READ_ONLY.

I don't know if there's any practical performance benefit to this change, but it seems like it can't make it worse.

What do you think?

#2 Updated by Ovidiu Maxiniuc about 5 years ago

You are right. Since we always fetch the data from the ResultSet s and use it to hydrate our own Record s there is no point in having the ResultSet s updatable.
Also, I see that DatabaseManager.readOnly is not populated any more so DatabaseManager.isReadOnly() which is used to set the readonly parameter is always returning false.

#3 Updated by Alexandru Lungu about 3 years ago

  • Assignee set to Ștefan Roman

#4 Updated by Alexandru Lungu about 3 years ago

Created 5452a from trunk/rev. 14560.

#5 Updated by Ștefan Roman about 3 years ago

  • % Done changed from 0 to 50
  • Status changed from New to WIP

#6 Updated by Ștefan Roman about 3 years ago

  • % Done changed from 50 to 100
  • Status changed from WIP to Review

Removed readOnly flag from SQLQuery, checked with HotelGUI and everything works fine.
Commited to 5452a revision 14561.

#7 Updated by Alexandru Lungu about 3 years ago

  • % Done changed from 100 to 50
  • Status changed from Review to WIP

I am OK with the current changes.

However, you still need to extend your removal of readOnly. More exactly, there is still a Query.readOnly member we no longer use, as the support for custom ResultSet read-mode is removed.
For that matter, remove Query.setReadOnly. This will require changes in Persistence.getQuery (readOnly flag) and DefaultDirtyShareManager.list (readOnly flag). These will cause changes in other files as well.

#8 Updated by Ștefan Roman about 3 years ago

  • Status changed from WIP to Review
  • % Done changed from 50 to 100

Removed readOnly flag and fixed persistence.list() and getQuery() new signatures, checked with a large project and it works normally.
Commited to 5452a revision 14562.

#9 Updated by Alexandru Lungu almost 3 years ago

I am OK with the changes.
I also committed 5452a rev. 14563 removing readOnly from share manager and multiplexer classes.

I am rebasing now and going to merge in trunk.

#10 Updated by Alexandru Lungu almost 3 years ago

  • Status changed from Review to Test

Branch 5452a was merged to trunk as rev 14704 and archived.

This can be closed.

#11 Updated by Greg Shah almost 3 years ago

  • Status changed from Test to Closed

Also available in: Atom PDF