maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx>]

Hi, Kristian!

On 16.08.2016 09:07, Kristian Nielsen wrote:
> Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx> 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 
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