THREESCALE-12061: Monkey patch ruby-oci8 to fix memory bloat (master) #4176
Conversation
| module OCI8CursorMemoryPatch | ||
| private | ||
|
|
||
| def define_one_column(pos, param) | ||
| @fetch_array_size = nil # disable array fetching anytime | ||
| super # call original | ||
| end | ||
| end | ||
|
|
||
| OCI8::Cursor.prepend(OCI8CursorMemoryPatch) |
There was a problem hiding this comment.
I think we can just move this down within the if System::Database.oracle? block and avoid the custom oci check.
There was a problem hiding this comment.
Well, that's what I thought first. But there is an issue. That .oracle? check requires ActiveRecord::DatabaseConfigurations to be loaded, and I think loading ActiveRecord first prevents the patch from being applied correctly (I guess because it might be pulling ruby-oci8 as a depencency).
So, I decided to opt for a safer check, that doesn't require ActiveRecord to be loaded.
There was a problem hiding this comment.
I think that being already loaded is not a problem. It is not something that we read at startup and then stays unchanged. It is just changing the runtime method behavior and the time it happens is not relevant.
There was a problem hiding this comment.
I'm not sure... I just know that when the patch was inside the block
ActiveSupport.on_load(:active_record) do
if System::Database.oracle?
the QE tests were still showing the regression.
But I was also using another approach (not a module prepend that you suggested), in case it might have any effect.
|
@akostadinov So, do you want to tweak this in some way, or shall we keep it as it is? |
|
I'd like to avoid the dual check. But looking at othert hings right now. Will look at this hopeuflly soon. update: There might actually be a very simple trick to solve For all problems, that will require oracle-enhanced fixes. Update: we need both - the monkey patch and this mapping update for ruby-oci8 + the oracle-enhanced cursor clean-up fixes and improvements. |
|
Included this into #4187 lets continue the discussion there. Want to test it before merging. |
What this PR does / why we need it:
Same as #4173, but for master
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-12061
Verification steps
Memory usage should be more or less stable in QE tests.
Special notes for your reviewer: