Skip to content

Improve fix for #80783 #6842

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

Closed
wants to merge 1 commit into from
Closed

Conversation

remicollet
Copy link
Member

@remicollet remicollet commented Apr 8, 2021

Working on new tests introduced in previous fix bccca0b
I notice some driver may return SQL_SUCCESS instead of SQL_SUCCESS_WITH_INFO, but with fetch len > 255

Tried with unixODBC 2.3.9 and [MySQL][ODBC 8.0(w) Driver][mysqld-5.5.5-10.4.18-MariaDB]

P.S. issue only affects binary columuns, not char ones, so bug80783.phpt, not bug80783a.phpt

@remicollet remicollet requested a review from cmb69 April 8, 2021 09:07
@remicollet
Copy link
Member Author

@cmb69 can you please check, as you are the author of previous fix.

@remicollet
Copy link
Member Author

Also open https://bugzilla.redhat.com/1947353 for discussion with unixODBC maintainer for Fedora

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Oh, I see. This patch looks good to me. Thanks!

Generally, most of odbc_stmt_get_col() looks fishy to me. First, we should not read binary data as SQL_C_CHAR, and secondly, we should not read in small chunks (while a comment states that this would usually be more efficient, I doubt so, and the required tinkering is prone to error). Changing the latter should better not be done in a stable release, and changing the former would be a BC break …

@remicollet
Copy link
Member Author

and secondly, we should not read in small chunks

I agree, sounds strange to allocate per small 256 steps, when we know the need size...

@remicollet
Copy link
Member Author

merged as 25f5a1b

@remicollet remicollet closed this Apr 8, 2021
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