Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix row::move_as behavior breaking reusability #1145

Closed
wants to merge 4 commits into from
Closed

Fix row::move_as behavior breaking reusability #1145

wants to merge 4 commits into from

Conversation

Krzmbrzl
Copy link
Contributor

For the time being, this is merely a sketch of how I think the fix has to work. The current implementation fails at the unavailability of a session object at the row scope...

Once this is finished, this PR fixes #1144

Krzmbrzl
Krzmbrzl commented Apr 21, 2024
View reviewed changes
src/core/row.cpp Outdated Show resolved Hide resolved
@vadz vadz added this to the 4.1.0 milestone Apr 26, 2024
@Krzmbrzl Krzmbrzl force-pushed the fix-row-move-as branch from 4d4e959 to 795ab44 Compare May 1, 2024 13:59
@Krzmbrzl
Copy link
Contributor Author

Krzmbrzl commented May 1, 2024

@avpalienko could you debug where the SegFault for Oracle is coming from? You seem to have good access to an Oracle installation, whereas for me it is always super tedious to get something to work.

@avpalienko
Copy link

@avpalienko could you debug where the SegFault for Oracle is coming from? You seem to have good access to an Oracle installation, whereas for me it is always super tedious to get something to work.

I've debugged on windows and have no segfault. But test fails:

BLOB
Blob binding
move_as

D:\CMA\Dev\SOCI\soci_githab\tests\common-tests.h(6784)
...............................................................................

D:\CMA\Dev\SOCI\soci_githab\tests\common-tests.h(6784): FAILED:
{Unknown expression after the reported line}
due to unexpected exception with message:
soci error: Invalid handle while fetching data from "select b from soci_test
where id=:id" with :1=42.

===============================================================================
test cases: 2 | 1 passed | 1 failed
assertions: 162 | 161 passed | 1 failed

I think it's a result of the move_as. Lob locator binds to the statement in define_by_pos():

bbe->reset();
ociData_ = bbe->get_lob_locator();

and then it initialized by OCIDefineByPos(). But in row::move_as() we call dtor ~oracle_blob_backend() with OCIDescriptorFree inside. On the next fetch in the post_fetch we call bbe->set_lob_locator(). Inside it we set locator in the old released value and call OCILobIsOpen which returns OCI_INVALID_HANDLE

@Krzmbrzl
Fix blob fetching semantics in Oracle
f110c69 
Previously, the into-type would take the blob object's LOB locator
object and bind that in the query. That works as long as there is only a
single Blob object being fetched from the database. However, if multiple
values are fetched consecutively, there would only be a single locator
object involved. In the best case, this leads to every new fetch
overriding the previous result and in the worst case, that previous
result has been turned into an independent blob object that has gone out
of scope, which leads to freeing of this shared locator.

With this commit, the into-type object will create its own locator
object and use that. When assigning the fetched result to a blob object,
the locator is copied such that the into-type and the blob keep
independent locator objects, i.e. the locator is no longer shared.
@Krzmbrzl
Copy link
Contributor Author

Krzmbrzl commented Sep 1, 2024 •
edited
Loading

Finally! I think with my latest commit I have found a way to fix the underlying issue 🚀

@avpalienko sorry that I took so long to look into this again. Your debug insights were still very valuable though, so thanks for having taken the time 🙏

@Krzmbrzl Krzmbrzl marked this pull request as ready for review September 1, 2024 07:57
@Krzmbrzl Krzmbrzl requested a review from vadz September 1, 2024 07:57
vadz
vadz approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for fixing this!

Will merge soon.

@vadz vadz mentioned this pull request Sep 3, 2024
Closed
@vadz vadz closed this in 63835ff Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avpalienko avpalienko avpalienko left review comments

@vadz vadz vadz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

Using row::move_as while iterating rowset yields segmentation fault
3 participants
@Krzmbrzl @avpalienko @vadz