maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

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?

> 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);

> 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.
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?

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,

 - Kristian.