maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

Sorry for the delayed reply (but better late than never).

>>>>> "Kristian" == Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx> writes:

Kristian> Hi Monty,
Kristian> While fixing test failures in the MariaDB 5.5 tree, I encountered problems in
Kristian> sql_string, related to changes you did there in 5.2. I managed to come up with
Kristian> some solution, but please see below patch and see if you can suggest something
Kristian> better.

Kristian> Two issues:

Kristian> 1. You added realloc_with_extra() / realloc_with_extra_if_needed(), and
Kristian> used them to replace calls to realloc(). But realloc() as part of its
Kristian> operation puts a terminating '\0' at the end of the allocation; with your
Kristian> change this is now at a different (and not very useful) position in the
Kristian> buffer. My change tries to put it in the original place, but it is not
Kristian> terribly elegant, and perhaps your original change needs to be rethought.

The \0 should not have been required at the end; In some cases it's
good that it's there as we can avoid an extra realloc.

Code that assumes that there is always an extra \0 at end is wrong
code and need to be fixed;  There is many different usage of the
library that can cause the code not to end with \0 even before.

When adding the above functions I did find some cases in MySQL where I
had to change to use ->c_ptr_safe() instead of just ->ptr() but all
those cases was a potential bug in the original code.

Conclusion:

Don't fix realloc. Instead fix the wrong call that is using the code!

Kristian> 2. I found a terrible mess (IMHO): We have two distict, subtly differing,
Kristian> sql_string.{h,cc}, one in /sql/ and one in /client/. We even seem to have a
Kristian> requirement for ABI compatibility between the two, since mysql_embedded uses
Kristian> client/sql_string.h with sql/sql_string.cc :-( And in fact your change seems
Kristian> to break such ABI compatibility, so I suspect mysql_embedded might be subtly
Kristian> broken in 5.2.

Sergei did fix the above by making the sql_string.cc files identical.


Kristian> But why do we need to have two sql_string in the first place? Especially given
Kristian> that they are apparently sufficiently close to be ABI compatible ... can't we
Kristian> eliminate on of them to get at least a little sanity into the sql_string
Kristian> story?

The reason was originally that sql_string.cc did at some point call
other server code that was not in the client. This seams to not be the
case anymore so now we can use identical versions of sql_string.cc

Regards,
Monty