ocaml-community / ocaml-mariadb

OCaml bindings to MariaDB, supporting the nonblocking API
55 stars 18 forks source link

Retrieve statement metadata after statement execution #41

Closed apeschar closed 4 months ago

apeschar commented 3 years ago

Currently, we get the result metadata immediately after preparing the statement. But this metadata is not guaranteed to match the actual result. And in case of INSERT ... RETURNING, it does not.

MySQL documentation mentions (emphasis mine):

If you call mysql_stmt_result_metadata() after mysql_stmt_prepare() but before mysql_stmt_execute(), the column types in the metadata are as determined by the optimizer. If you call mysql_stmt_result_metadata() after mysql_stmt_execute(), the column types in the metadata are as actually present in the result set. In most cases, these should be the same.

And most ≠ all.

So this PR reloads the result metadata after each statement execution.

This fixes #33, and also ensures correctness of the row structure if table or view definitions change after statement preparation. Also see that issue for reproduction steps.

apeschar commented 2 years ago

I've rebased this on master, and taken into account the comments on https://github.com/andrenth/ocaml-mariadb/pull/39.

Specifically mysql_free_result is replaced by mysql_free_result_start and mysql_free_result_cont in the non-blocking implementation.

apeschar commented 2 years ago

Specifically mysql_free_result is replaced by mysql_free_result_start and mysql_free_result_cont in the non-blocking implementation.

I realised this is not needed since freeing the metadata should never block. I've restored the original changes rebased on master.

For the "non-blocking" version (which isn't needed), see: https://github.com/andrenth/ocaml-mariadb/compare/master...apeschar:insert-returning-new

This PR should also fix #29 (at least if leaking the metadata is all) and supplant #39 without causing segfaults.

paurkedal commented 2 years ago

I did some testing with the tweaks to the tests from #29. This fixes the memory leak for blocking_stress_test and it seems to reduce the issue for nonblocking_lwt_stress_test and nonblocking_async_stress_test. There is probably an additional memory leak somewhere, but I think that shouldn't prevent a partial fix. Also, the Caqti test suite runs without issues using this branch for the mariadb driver.

ygrek commented 4 months ago

verified it fixes memory leak thanks a lot for figuring it out sorry took so long :]

apeschar commented 4 months ago

Awesome, thanks!