Oleksandr Byelkin <sanja@montyprogram.com> writes:
https://jira.mariadb.org/browse/MDEV-10559
Original problem was that database drivers in Python & Perl (probably other) do not call mysql_close for MYSQL object if there was no connection happing (memory leak).
MySQL has solution for this - freeing option on failed connection if user did not asked something else (we even has the same flag, but it did not work). So I took MySQL solution. Hm, so let me see if I understand correctly: This is actually MDEV-10455? Which is a regression introduced by b803b46dac40d2417e41c778938d97951ce4ec88 "Client attributes"?
And you want to make a change (in stable 10.0?) to re-activate the CLIENT_REMEMBER_OPTIONS flag, _off_ by default? yes, that is what needed by Python & Perl.
But it appeared not so compatible with our async wrappers. Frankly speaking the code is difficult to read and debug because everything made over macros nor I have found decent description/overview of how it is done (thank you if somebody point to it) so I can not say that understand what it is doing, and all my suggestion based on how I understood it. There is a mailing list thread discussing this:
https://lists.launchpad.net/maria-developers/msg04324.html
And there is the knowledgebase, of course, but its mostly about how to use it from application code:
https://mariadb.com/kb/en/mariadb/non-blocking-client-library/
Basically, each wrapper runs the corresponding base function in a co-routine, with its own stack stored in mysql->options.extension (similar to running in a separate thread). So if a base function were to de-allocate mysql->options.extension, it will destroy its own CPU stack, which I think explains crashes and lack of stack traces...
I think the essential point here is that all use of async functions requires first setting the MYSQL_OPT_NONBLOCK option, otherwise they will crash:
mysql_options(&mysql, MYSQL_OPT_NONBLOCK, 0); Huh! Interesting finding. actually test which crash to not set the flag, so do not use async operations, but wrapper is in act (by global setup
Hi, Kristian! On 16.08.2016 09:07, Kristian Nielsen wrote: probably).
So what we have: we allocate memory for some options set between mysql_init and mysql_real_connect, instead of "reall connect" called some wrapper which store some pointers to the options then call mysql_real_connect and if "real_connect" free them it fail on exit. Might fail even earlier, as real_connect would be freeing the memory of its own CPU stack, which can never be a good idea...
I think that best solution is that wrapper
1) store current state of CLIENT_REMEMBER_OPTIONS
2) set the flag
3) call mysql_real_connect
4) if connect fail and there was not "remember options" then wrapper free options by itself The problem with this is that this will probably break existing programs.
Exactly, that was my concern, that is why I start asking.
If I understand you correctly, this will effectively unset the MYSQL_OPT_NONBLOCK option on a failed mysql_real_connect() ? If so, then any further use of an async function will immediately crash anyway. Like mysql_close_start()...
It would basically be required for the application to immediately re-activate the MYSQL_OPT_NONBLOCK option after every failed connect. It seems very likely that there could be existing programs that do not do this. So it does not seem a possible route?
If you read Monty's original review comments for the async feature, you will find this:
https://lists.launchpad.net/maria-developers/msg04324.html
"The reason for not using mysql->options.extension was only becasue mysql_real_connect() freed the options on failure. This code is not needed as they will be freed by mysql_close() anyway.
So I suggest you remove the code:
if (!(client_flag & CLIENT_REMEMBER_OPTIONS)) mysql_close_free_options(mysql);
from mysql_real_connect()"
So that is the reason for the original removal/disabling of CLIENT_REMEMBER_OPTIONS. I'm wondering if it is really a good idea to re-introduce it, and even worse, do so in a stable release? Do you have other solution for memory leak on failed connect? Of course one could rewrite python & perl, but who will?
It seems the dilemma here is that on the one hand, not supporting CLIENT_REMEMBER_OPTIONS creates incompatibility with the MySQL version of the library. On the other hand, supporting it breaks async-using code. The documentation (MySQL one) does not say that mysql_close() can be omitted on failed connect, though neither does it say that it cannot.
Elena wrote that the original regressions was introduced by a patch for "Client attributes". What is the reason for that? I'm thinking perhaps "Client attributes" made it so that mysql->options.extension is _always_ allocated? Then missing mysql_close() will leak the options memory, while before the patch, programs that did not set options themselves would not leak? If this is how things are, then another option could be to fix the "Client attributes" patch to not allocate the extra memory by default (if that is possible).
Other possible options could be to clear all options _except_ the MYSQL_OPT_NONBLOCK. Or maybe force CLIENT_REMEMBER_OPTIONS on if MYSQL_OPT_NONBLOCK is set. Both these options seem unattractive, since they make the semantics even more complicated of a part of the API that is already too magic.
In any case, I think before discussing the technical solution, it is necessary to first carefully consider and decide on the semantics of how things should work. Changing behaviour of the client library is very delicate, there are a _lot_ of possibilities for breaking backwards compatibility.
Hope this helps,
Thank you a lot that really helps. So as I see it: bringing in act the wrappers makes it incompatible with the flag, but we need it to non-leaking slippery written code of connect in other cases. IMHO we just can enforce the flag in case of wrappers, so client either writes something on his knee or use async operation (where some rules must be obeyed). Making something more I think do not worth it because: 1) in 10.2 C/C will replace the library and it has no the problem 2) we should no make huge changes before 10.2