[Maria-developers] A mess with sql_alloc() ?
I need to use sql_string.* and sql_list.* in MYSQL_CLIENT context (mysqlbinlog.cc). sql_list.h refers to sql_alloc() function which originally has "THD dependent" implementation: mysql_priv.h: #ifndef MYSQL_CLIENT ... (1) void *sql_alloc(size_t); ... #endif thr_malloc.cc: void *sql_alloc(size_t Size) { MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC); return alloc_root(root,Size); } So I redefined sql_alloc() in a client context: void *sql_alloc(size_t size) { ... } ... #include "sql_string.h" #include "sql_list.h" And this appeared to run into conflict with sql/sql_string.cc: (2) extern uchar* sql_alloc(unsigned size); Moreover I found the following declaration client\sql_string.cc: (3) extern void sql_alloc(size_t size); Isn't it a mess: compare (1), (2) and (3) ? Actually, looks like both (2) & (3) are reducible: - I found no direct usage of sql_alloc() in sql_string.* files; - After commenting (2) & (3) out the rebuild was successfull. If (2) & (3) shouldn't be deleted, then they should be brought in correspondence with (1). If not, what should I do to redefine sql_alloc()? Looks like along with sql_alloc(), we can delete similar declarations (2a) & (3a) of sql_element_free() (and actually all occurences of this function): mysql_priv.h #ifndef MYSQL_CLIENT ... (1a) void sql_element_free(void *ptr); ... #endif thr_malloc.cc void sql_element_free(void *ptr __attribute__((unused))) {} /* purecov: deadcode */ sql/sql_string.cc (2a) extern void sql_element_free(void *ptr); client/sql_string.cc (3a) extern void sql_element_free(void *ptr); Indeed, besides pointed above we find only the following occurences of sql_element_free(): client/mysql.cc void* sql_alloc(unsigned size); void sql_element_free(void *ptr); ... void *sql_alloc(size_t Size) { return my_malloc(Size,MYF(MY_WME)); } void sql_element_free(void *ptr) { my_free(ptr,MYF(0)); } So sql_element_free() is called nowhere (note also that this function has a do-nothing body).
Alexi1952 <Alexi1952@yandex.ru> writes:
mysql_priv.h:
(1) void *sql_alloc(size_t);
So I redefined sql_alloc() in a client context:
void *sql_alloc(size_t size) { ... } ... #include "sql_string.h" #include "sql_list.h"
(Any reason you put the definition before the #include's ? Usually one writes things the other way around)
sql/sql_string.cc:
(2) extern uchar* sql_alloc(unsigned size);
(3) extern void sql_alloc(size_t size);
- I found no direct usage of sql_alloc() in sql_string.* files; - After commenting (2) & (3) out the rebuild was successfull. If (2) & (3) shouldn't be deleted, then they should be brought
Seems like you should just delete them. In any case extern declarations like this should be avoided, much better to only have the declaration in one place in some *.h file.
Looks like along with sql_alloc(), we can delete similar declarations (2a) & (3a) of sql_element_free() (and actually all occurences of this function):
mysql_priv.h
(1a) void sql_element_free(void *ptr);
thr_malloc.cc
void sql_element_free(void *ptr __attribute__((unused))) {} /* purecov: deadcode */
sql/sql_string.cc
(2a) extern void sql_element_free(void *ptr);
client/sql_string.cc
(3a) extern void sql_element_free(void *ptr);
Indeed, besides pointed above we find only the following occurences of sql_element_free():
client/mysql.cc
void* sql_alloc(unsigned size); void sql_element_free(void *ptr); ... void *sql_alloc(size_t Size) { return my_malloc(Size,MYF(MY_WME)); } void sql_element_free(void *ptr) { my_free(ptr,MYF(0)); }
So sql_element_free() is called nowhere (note also that this function has a do-nothing body).
Again I think the extern declarations should be deleted. Not sure about the sql_element_free(). My guess would be that they were an attempt to be able to use sql_list etc. from outside the server without memory leaks, but the attempt was never completed. The issue with sql_alloc() is that it uses a mem_root that will automatically free all memory at end of a statement (for example). Maybe someone wanted to have explicit calls to sql_element_free() (which would work in client but do nothing in server), but they stopped half-way? We might consider deleting them, on the other hand we should maybe better leave them to reduce risk of conflicts when merging with MySQL. I have no strong opinion on this one. - Kristian.
Thanks for your comments!
In any case extern declarations like this should be avoided, much better to only have the declaration in one place in some *.h file.
I agree. Currently, we have: mysql_priv.h: #ifndef MYSQL_CLIENT ... void *sql_alloc(size_t); ... #endif thr_malloc.cc: void *sql_alloc(size_t Size) { MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC); return alloc_root(root,Size); } This states that sql_alloc() *should not* be used in MYSQL_CLIENT context (hence all these extern declarations). In my opinion, it's more correctly to have: mysql_priv.h: void *sql_alloc(size_t); thr_malloc.cc: #ifndef MYSQL_CLIENT void *sql_alloc(size_t Size) { MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC); return alloc_root(root,Size); } #endif which states instead that sql_alloc() *can be* used in a client context but should be supplied with *another definition*. Question: May I do these changes in mysql_priv.h and thr_malloc.cc ? On the one hand, it is resonable. On the other hand, I am confused a bit with "mysql_priv.h" name intended to mean that content of this file belongs to mysql internals (though this file is included in mysqlbinlog.cc) - Alex 09.10.09, 15:15, "Kristian Nielsen" <knielsen@knielsen-hq.org>:
mysql_priv.h: (1) void *sql_alloc(size_t); So I redefined sql_alloc() in a client context:
void *sql_alloc(size_t size) { ... } ... #include "sql_string.h" #include "sql_list.h" (Any reason you put the definition before the #include's ? Usually one writes
sql/sql_string.cc:
(2) extern uchar* sql_alloc(unsigned size); (3) extern void sql_alloc(size_t size); - I found no direct usage of sql_alloc() in sql_string.* files; - After commenting (2) & (3) out the rebuild was successfull. If (2) & (3) shouldn't be deleted, then they should be brought Seems like you should just delete them. In any case extern declarations like this should be avoided, much better to only have the declaration in one place in some *.h file. Looks like along with sql_alloc(), we can delete similar declarations (2a) & (3a) of sql_element_free() (and actually all occurences of this function): mysql_priv.h (1a) void sql_element_free(void *ptr); thr_malloc.cc
void sql_element_free(void *ptr __attribute__((unused))) {} /* purecov: deadcode */ sql/sql_string.cc
(2a) extern void sql_element_free(void *ptr);
client/sql_string.cc
(3a) extern void sql_element_free(void *ptr);
Indeed, besides pointed above we find only the following occurences of sql_element_free():
client/mysql.cc
void* sql_alloc(unsigned size); void sql_element_free(void *ptr); ... void *sql_alloc(size_t Size) { return my_malloc(Size,MYF(MY_WME)); } void sql_element_free(void *ptr) { my_free(ptr,MYF(0)); }
So sql_element_free() is called nowhere (note also that this function has a do-nothing body). Again I think the extern declarations should be deleted. Not sure about the sql_element_free(). My guess would be that they were an attempt to be able to use sql_list etc. from outside the server without memory leaks, but the attempt was never completed. The issue with sql_alloc() is that it uses a mem_root that will automatically free all memory at end of a statement (for example). Maybe someone wanted to have explicit calls to sql_element_free() (which would work in client but do nothing in server), but
Alexi1952 writes: things the other way around) they stopped half-way? We might consider deleting them, on the other hand we should maybe better leave them to reduce risk of conflicts when merging with MySQL. I have no strong opinion on this one. - Kristian. _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- Почта в порядке находится здесь: http://mail.yandex.ru/promo/new/order
Alexi1952 <Alexi1952@yandex.ru> writes:
Thanks for your comments!
In any case extern declarations like this should be avoided, much better to only have the declaration in one place in some *.h file.
I agree. Currently, we have:
mysql_priv.h:
#ifndef MYSQL_CLIENT ... void *sql_alloc(size_t); ... #endif
thr_malloc.cc:
void *sql_alloc(size_t Size) { MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC); return alloc_root(root,Size); }
This states that sql_alloc() *should not* be used in MYSQL_CLIENT context (hence all these extern declarations). In my opinion, it's more correctly to have:
mysql_priv.h:
void *sql_alloc(size_t);
thr_malloc.cc:
#ifndef MYSQL_CLIENT void *sql_alloc(size_t Size) { MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC); return alloc_root(root,Size); } #endif
which states instead that sql_alloc() *can be* used in a client context but should be supplied with *another definition*.
Yes, agree.
Question: May I do these changes in mysql_priv.h and thr_malloc.cc ? On the one hand, it is resonable. On the other hand, I am confused a bit with "mysql_priv.h" name intended to mean that content of this file belongs to mysql internals (though this file is included in mysqlbinlog.cc)
I think this is ok to do (I will check them again when I see the patch for review). It is true about mysql_priv.h being more internal, but then mysqlbinlog already goes deep deep into internals (the replication event stuff). - Kristian.
Hi!
"Kristian" == Kristian Nielsen <knielsen@knielsen-hq.org> writes:
Kristian> Alexi1952 <Alexi1952@yandex.ru> writes:
mysql_priv.h:
(1) void *sql_alloc(size_t);
So I redefined sql_alloc() in a client context:
void *sql_alloc(size_t size) { ... } ... #include "sql_string.h" #include "sql_list.h"
Kristian> (Any reason you put the definition before the #include's ? Usually one writes Kristian> things the other way around)
sql/sql_string.cc:
(2) extern uchar* sql_alloc(unsigned size);
(3) extern void sql_alloc(size_t size);
- I found no direct usage of sql_alloc() in sql_string.* files; - After commenting (2) & (3) out the rebuild was successfull. If (2) & (3) shouldn't be deleted, then they should be brought
Kristian> Seems like you should just delete them. Yes, you should just delete it; This is something that has been left in the code from old times or some. Same goes for sql_element_free(). I have deleted them from my version of MariaDB and will be in the code next time I push. (This is normal cleanup we have to do, even at the risk to get a few merge conflicts with MySQL) Kristian> In any case extern declarations like this should be avoided, much better to Kristian> only have the declaration in one place in some *.h file. Agree. Regards, Monty
participants (3)
-
Alexi1952 -
Kristian Nielsen -
Michael Widenius