Hi Bar,
Thanks for review, much appreciated! I addressed most of the comments in the new push https://github.com/MariaDB/server/compare/bb-10.8-wlad-MDEV-26713
>commit 1411d5f40ceaf2eecabbcf68588743439003f4a2
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Fri Nov 19 11:42:46 2021 +0100
>
>
>+ wchar_t wbuf[80];
>+ char to[80*3]; /* there is at most 3 bytes per wchar, in any codepage */
>+ wchar_t *pos=wbuf,*end=wbuf + array_elements(wbuf)-1;
>Isn't it "at most 4 bytes per char"?
You’re right. I missed GB18030, single UTF16 wchar_t can expand to four GB18030 bytes . But for UTF8, single UTF16 wchar_t can’t expand to 4 bytes, if you mean that?
>#############################################################
>
>commit 7af691ca11c95a385aff4889a56870d70843e78d
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>
>- pthread_auto_mutex_decl(my_conio_cs);
...
>
>There was a mutex lock inside my_cgets().
>Why was it needed before the patch and why is it not needed any more?
The old code did more than it had to. We do not need mutex locking for reading line from console window, and we do not use threads in command line client.
>########################################################
>
>commit b9c38470ed6e04185f7f84a056034b7254336d51
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Mon Nov 22 13:22:05 2021 +0100
......
>Should not we add an UTF8 BOM to the beginning of the file,
>so text editors do not break it later.
It might render it unusable for our own stuff, see MySQL Bugs: #67880: mysql 5.6.9 on windows does not start if my.ini file is in utf8 format
I see no reason for having BOM now. Windows’ own editors, Notepad by defaults stores in BOM-less UTF-8 (on systems we target with this work at last), VS Code, Notepad++ . Some Linux tools however would have problems with BOM, in particular “cat” . Anyway, its been a long while since I’ve seen UTF8 being not correctly detected and handled by any editor.
>########################################################
>
>commit 51ce069376fec45d340563a4f7630f8a155afd64
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Fri Nov 26 18:41:35 2021 +0100
>
> MDEV-26713 Introduce --password-codepage client setting
> I'm not sure we need this new options.
> Looks like over-engineering a bit.
Ok,I agree new parameter is not nice. And, found a way to handle that without , using --default-character-set MDEV-26713 allow users with non-UTF8 passwords to login after upgrade. · MariaDB/server@b45c887 (github.com)
I’m firmly convinced, that if we are about to break connectivity, and we know about it, we must to offer something else than “contact your administrator”. That administrator could be the guy who implemented password validation plugin to force at last one non-ASCII character (it is safer, say some people on the internet, harder to break for hackers), and now that poor admin is busy fixing a hundreds of “reset my password” requests. Who knows, I do not think this scenario is entirely unrealistic.
We have no clue how much impact breaking non-ASCII passwords would be. We never warned users, that such passwords would not work, or are “not portable” .It is impossible to run an automated check, whether such passwords were used, prior to upgrade, as we never stored the actual password.
At the end, users are not guilty in the mess we created ourselves by handling passwords as bytes, rather than strings. Thus user should not be punished for something he had not broke. Instead, an explanation, and an easy workaround that allows to connect after upgrade, must be in release notes.
>#####################################################
>
>commit 5c09e9ce4f922fe9674f3f85329742c0c5498a28
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Fri Nov 19 14:03:51 2021 +0100
>
> MDEV-27092 Windows - services that have non-ASCII characters do not
>work with activeCodePage=UTF8
>
>This code:
>
>+ AWSTRDUP(w_ServiceName,lpServiceName);
....
>duplicates two times. Consider wrapping these variables into a class
>and reuse the class in both cases.
It duplicates because those functions are a trivial wrappers for 2 C functions, with similar, but not exact the same signature I considered wrapping, a C++ class, spent some time trying. It turned out looking too artificial, and added more code actually that it removed, which made me believe there is no good abstraction for this case. I’m leaving it as-is.
>########################################################
>commit b155c4036cfb3ffdaa7c272048ac9d32e3182d78
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Mon Nov 29 19:47:36 2021 +0100
>
> MDEV-26713 set console codepage to what user set in
>--default-character-set
>...
>
>+static void adjust_console_codepage()
...<skip>...
>Also, I think this function should only be called when
>default_charset was some explicit name, not MYSQL_AUTODETECT_CHARSET_NAME.
>
>I think the logic should be:
>
>1. If --default-character-set=auto, then detect it from the console.
>2. If --default-character-set=csname, then set the console CP to csname.
>
>Now in case of N1 it seems to detect the character set from the console,
>then sets the console to what has been detected.
>Is the last step really needed in case of auto-detection?
It seems like it would not be needed. However our autodetection is loose, and this has some implications :
Let's say the console codepage is cp437. And we got some old Windows, pre Win10 1903 .
cp437 would be autodetected as MySQL cp850. There are 46 codepoint point differences between cp437 and cp850, (s. https://en.wikipedia.org/wiki/Code_page_850, differences marked yellow), so those codepages are not really very similar, differing in ~20% of the code points.
In the logic I used, console would be is fixed from 437 to 850, thus showing French accented characters for French accented characters in data, rather than random Greek.
So, I’d think last step was fine.
>########################################################
>
>commit b38c341cc392630c798eb754f87338d37ec95cdf
>Author: Vladislav Vaintroub <wlad@mariadb.com >
>Date: Sun Dec 5 17:08:18 2021 +0100
>
>Fix race condition in test.
>
>Can you extend the comment and tell what actually happened
>before this change?
I rebased into into some other commit, but so I'll describe here what happened here.
On the “new” buildbot, Windows host runs mysql-test-run.pl --parallel=64 . It is an “old” version of Windows in terms of this work, Windows 10 build 1803, so it uses legacy codepage detection via console CP. And it runs a test client_charset_win.test, which is
chcp 1257 > NULL & $MYSQL --default-character-set=auto -e "select @@character_set_client"
The machine is US English Windows, with default console codepage is 437 . However test would sometimes, actually most of the times, show cp850 instead of expected cp1257
What happens here, there is that of builbot slave runs perl (MTR), and all MTR subprocesses share the same console object, the one of python process, which is builbot slave.
Now, none of the subprocesses processes would be using console, and there is no visible window even. Output is redirected for all processes, as well as input, where appropriate. Yes, one of the clients managed to change the shared console codepage to 850, in the tiny time window between chcp and the client startup . I’d guess this was fixing of loosely autodetected cp850 on cp437, as I described earlier.
So the fix is avoid changing console codepage, unless there is an actual console output.
Best regards,
Wlad
From: Alexander Barkov
Sent: Friday, 10 December 2021 14:40
To: maria-developers; Vladislav Vaintroub
Subject: Review: bb-10.8-wlad-MDEV-26713
Hi Wlad,
You've done a great piece of work. Thanks and congrats.
..