search

SOCI / soci

Official repository of the SOCI - The C++ Database Access Library
http://soci.sourceforge.net/
Boost Software License 1.0
1.42k stars 478 forks source link

Fix blob errors in Oracle backend issue #1141 #1142

Closed avpalienko closed 2 months ago

avpalienko commented 7 months ago

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

vadz commented 7 months ago

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?

avpalienko commented 7 months ago

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.

It's commented as TODO. It will be good to have an explicit Open/Close for oracle blob in future Oracle says "These functions allow an application to mark the beginning and end of a series of LOB operations so that specific processing (e.g., updating indexes, etc.) can be performed when a LOB is closed."

Could you restart pipelines, please?

vadz commented 7 months ago

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.

It's commented as TODO. It will be good to have an explicit Open/Close for oracle blob in future Oracle says "These functions allow an application to mark the beginning and end of a series of LOB operations so that specific processing (e.g., updating indexes, etc.) can be performed when a LOB is closed."

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.

Could you restart pipelines, please?

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.

Krzmbrzl commented 7 months ago

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 👀

avpalienko commented 7 months ago

I've deleted commented code

avpalienko commented 7 months ago

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 👀

All existing tests are passed

vadz commented 7 months ago

@Krzmbrzl Do you have any objections to merging this?

Krzmbrzl commented 7 months ago

Do you have any objections to merging this?

No - as long as the common Blob interface remains functional (which seems to be the case), I'm fine with the changes.

vadz commented 2 months ago

Sorry, just to confirm: is this still needed even with/after #1145?

avpalienko commented 2 months ago

Sorry, just to confirm: is this still needed even with/after #1145?

I think no