Skip to content

Update ma_charsets #2

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

Merged

Conversation

better0fdead
Copy link

@better0fdead better0fdead commented Oct 5, 2022

Updated compiled_charset_list prior to tarantool/mysql#47 in tarantool/mysql repo.

9EOR9 and others added 4 commits October 5, 2022 11:24
Added the character sets and collations described in  CONC-299.
No additional test added, since charset/test_conc33 checks if all character sets and collations from server are supported.

(cherry picked from commit fe129ed)
According to https://msdn.microsoft.com/en-us/library/windows/desktop/dd317756(v=vs.85).aspx code page for latin1 is 1252, and not 850. The same for some other collations.
Changed "auto" charset detection to use only GetACP() on windows.
Since I guess C/ODBC is the only user of that feature, and because GetConsoleCP() returns OEM codepage, and not the default system codepage, which is used by DM as source codepage for recoding to unicode.

(cherry picked from commit 9a50a7d)
(cherry picked from commit 61ecc3b)
(cherry picked from commit e9a2c9e)
@Totktonada
Copy link
Member

Totktonada commented Oct 7, 2022

Did you consider to update the base downstream version to the latest upstream release? (Asked in PR #1, but didn't get an answer.)

Anyway, backporting is okay for me too. I just want to understand how the decision was made.

@better0fdead
Copy link
Author

Did you consider to update the base downstream version to the latest upstream release? (Asked in PR #1, but didn't get an answer.)

Anyway, backporting is okay for me too. I just want to understand how the decision was made.

I thought about that, but it breaks tarantool/mysql, I think it's worth another task. For that particular issue just updating ma_charset is enough.

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Can you please provide a draft PR for mysql with charset test based on tarantool/mysql#47 ? It is rather hard to verify these changes by reviewing the code.

@better0fdead
Copy link
Author

better0fdead commented Oct 9, 2022

Thank you for the PR. Can you please provide a draft PR for mysql with charset test based on tarantool/mysql#47 ? It is rather hard to verify these changes by reviewing the code.

Yes, u can see it here, i dele ted workaround from ci for this bug, so u can see that ci is green without workaround.

@LeonidVas LeonidVas merged commit 54cdb25 into 3.0.1-beta-38-gb4681a2-tarantool Oct 10, 2022
@LeonidVas LeonidVas deleted the better0fdead/gh-47-charsets branch October 10, 2022 14:56
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.

7 participants