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.

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

Only prepare for describe if statement type is 'SELECT' for Oracle backend #1119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iqbal-hasprime
Copy link

@iqbal-hasprime iqbal-hasprime commented Jan 9, 2024

In my use-case I always call statement::exchange_for_rowset, even if the query for the statement does not return any result, since type of query used to create the statement is unknown beforehand.

OCIStmtExecute call with OCI_DESCRIBE_ONLY in oracle_statement_backend::prepare_for_describe will fail (with "ORA-24333: zero iteration count" error) for non-SELECT queries, since there's nothing to describe.

My proposed change to the library is to the call to OCIStmtExecute if the statement type is OCI_STMT_SELECT.

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.

Thanks, I agree that it's harmless to avoid this error, but I'd just like to check what should happen in case of an error?

src/backends/oracle/statement.cpp Show resolved Hide resolved
src/backends/oracle/statement.cpp Outdated Show resolved Hide resolved
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, thanks!

I'll merge this soon.

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.

Oops, sorry, I spoke too soon and didn't notice that a test failed.

It looks like this change breaks some other uses of SOCI, so it can't be applied as is... Sorry for missing it originally.

@iqbal-hasprime
Copy link
Author

Oops, sorry, I spoke too soon and didn't notice that a test failed.

It looks like this change breaks some other uses of SOCI, so it can't be applied as is... Sorry for missing it originally.

No worries. I should have checked as well. I'll try and fix the failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants