Fix blob errors in Oracle backend issue #1141 #1142
Conversation
|
Thanks for the fixes! I don't know much about LOBs in Oracle, but we definitely shouldn't be keeping this code commented out, if we prefer/have to rely on implicit LOB opening, it should be just removed. @Krzmbrzl Any comments? |
It's commented as TODO. It will be good to have an explicit Open/Close for oracle blob in future Could you restart pipelines, please? |
I think we need to either do it now, if it's possible, or remove this completely. Keeping a chunk of code commented out is just never a good idea.
I had already done it and just did it again but they just keep failing, I have no idea what's going on here but will try to look tomorrow if it still doesn't work then. |
|
Do the regular Blob test cases (the Backend independent ones) still work with these changes? I thought I needed explicit opening/closing for consistent-ish behavior across?backends but I don't recall the exact details 👀 |
|
I've deleted commented code |
All existing tests are passed |
|
@Krzmbrzl Do you have any objections to merging this? |
There was a problem hiding this comment.
Formatting is inconsistent with other code parts (the extra space before parenthesis when calling functions for constructing objects is not used anywhere else afaict).
I'll leave it to @vadz to decide whether that needs fixing or not though.
| // append does not work (Oracle bug #886191 ?) | ||
| // b.append(buf, sizeof(buf)); | ||
| // assert(b.get_len() == sizeof(buf) + 10); |
There was a problem hiding this comment.
How can this be? If the common test cases succeed, then that means that
Lines 6563 to 6566 in 8a5ed87
succeeded, which implies that the
append function works 🤔
| { | ||
| blob b ( sql ); | ||
| sql << "select img from soci_test where id = 7", into ( b ); | ||
| // assert(b.get_len() == sizeof(buf) + 10); |
There was a problem hiding this comment.
| // assert(b.get_len() == sizeof(buf) + 10); |
| } | ||
| }; | ||
|
|
||
| TEST_CASE ( "Oracle blob", "[oracle][blob]" ) |
There was a problem hiding this comment.
What exactly is this test case testing that is not covered in the test cases shared across backends? The reasoning for having a Oracle-specific Blob test case should probably be written down here as a comment
There was a problem hiding this comment.
I have restored the code from previous commits. We can remove it and use common test, but we need add test cases using blob before start transaction and after commit/rollback
There was a problem hiding this comment.
we need add test cases using blob before start transaction and after commit/rollback
This would be an Oracle-specific test then as this is undefined behavior in a cross-DB setting (as specified in SOCI docs). So for this specific scenario, a backend-specific test case makes sense
No - as long as the common Blob interface remains functional (which seems to be the case), I'm fine with the changes. |
|
Sorry, just to confirm: is this still needed even with/after #1145? |
I think no |
The LOB opening and closing mechanism in Oracle has some restrictions which makes it implicitly usage is dangerous This commit remove the call OCILobOpen from fetching and add some oracle blob tests