Re: [Maria-developers] b248988c8b6: MDEV-19275 Provide SQL service to plugins.
Hi, Alexey! see questions below On Sep 07, Alexey Botchkov wrote:
revision-id: b248988c8b6 (mariadb-10.6.1-74-gb248988c8b6) parent(s): 42ad4fa3464 author: Alexey Botchkov committer: Alexey Botchkov timestamp: 2021-09-07 14:08:13 +0400 message:
MDEV-19275 Provide SQL service to plugins.
SQL service added. It provides the limited set of client library functions to be used by plugin.
diff --git a/include/mysql/service_sql.h b/include/mysql/service_sql.h new file mode 100644 index 00000000000..5050793f655 --- /dev/null +++ b/include/mysql/service_sql.h @@ -0,0 +1,99 @@ +/* Copyright (C) 2021 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1301 USA */ + +#if defined(MYSQL_SERVER) && !defined MYSQL_SERVICE_SQL +#define MYSQL_SERVICE_SQL + +#include <mysql.h> + +/** + @file + SQL service + + Interface for plugins to execute SQL queries on the local server. + + Functions of the service are the 'server-limited' client library: + mysql_init + mysql_real_connect_local + mysql_real_connect + mysql_errno + mysql_error + mysql_real_query + mysql_affected_rows + mysql_num_rows + mysql_store_result + mysql_free_result + mysql_fetch_row + mysql_close +*/ + + +#ifdef __cplusplus +extern "C" { +#endif + +extern struct sql_service_st { + MYSQL * STDCALL (*mysql_init)(MYSQL *mysql); + MYSQL *(*mysql_real_connect_local)(MYSQL *mysql, + const char *host, const char *user, const char *db, + unsigned long clientflag); + MYSQL * STDCALL (*mysql_real_connect)(MYSQL *mysql, const char *host, + const char *user, const char *passwd, const char *db, unsigned int port, + const char *unix_socket, unsigned long clientflag); + unsigned int STDCALL (*mysql_errno)(MYSQL *mysql); + const char * STDCALL (*mysql_error)(MYSQL *mysql); + int STDCALL (*mysql_real_query)(MYSQL *mysql, const char *q, + unsigned long length); + my_ulonglong STDCALL (*mysql_affected_rows)(MYSQL *mysql); + my_ulonglong STDCALL (*mysql_num_rows)(MYSQL_RES *res); + MYSQL_RES * STDCALL (*mysql_store_result)(MYSQL *mysql); + void STDCALL (*mysql_free_result)(MYSQL_RES *result); + MYSQL_ROW STDCALL (*mysql_fetch_row)(MYSQL_RES *result); + void STDCALL (*mysql_close)(MYSQL *sock); +} *sql_service; + +#ifdef MYSQL_DYNAMIC_PLUGIN + +#define mysql_init sql_service->mysql_init +#define mysql_real_connect_local sql_service->mysql_real_connect_local +#define mysql_real_connect sql_service->mysql_real_connect +#define mysql_errno(M) sql_service->mysql_errno(M) +#define mysql_error(M) sql_service->mysql_error(M) +#define mysql_real_query sql_service->mysql_real_query +#define mysql_affected_rows sql_service->mysql_affected_rows +#define mysql_num_rows sql_service->mysql_num_rows +#define mysql_store_result sql_service->mysql_store_result +#define mysql_free_result sql_service->mysql_free_result +#define mysql_fetch_row sql_service->mysql_fetch_row +#define mysql_close sql_service->mysql_close + +#else + +MYSQL *mysql_real_connect_local(MYSQL *mysql, + const char *host, const char *user, const char *db, + unsigned long clientflag);
normally the service file is the authoritative source of the service documentation, everything there is to know about the service should be there. In this case it's mostly the normal client C API, so no need to document it. But mysql_real_connect_local is new, so please, add a comment here, explaining what it is. For example, I'd think it connects to the current server, internally. But then, why does it need host and user? The comment should cover it.
+ +/* The rest of the function declarations mest be taken from the mysql.h */ + +#endif /*MYSQL_DYNAMIC_PLUGIN*/ + + +#ifdef __cplusplus +} +#endif + +#endif /*MYSQL_SERVICE_SQL */ + + diff --git a/plugin/test_sql_service/CMakeLists.txt b/plugin/test_sql_service/CMakeLists.txt index aa9ecfe685e..7c61a1c3c7a 100644 --- a/plugin/test_sql_service/CMakeLists.txt +++ b/plugin/test_sql_service/CMakeLists.txt @@ -15,4 +15,5 @@
SET(SOURCES test_sql_service.c)
-MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY RECOMPILE_FOR_EMBEDDED) +ADD_DEFINITIONS(-DMYSQL_SERVER)
why?
+MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY) diff --git a/plugin/test_sql_service/test_sql_service.c b/plugin/test_sql_service/test_sql_service.c index 062f10fce58..e1155a98c40 100644 --- a/plugin/test_sql_service/test_sql_service.c +++ b/plugin/test_sql_service/test_sql_service.c @@ -14,71 +14,113 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */
-#define PLUGIN_VERSION 0x100 -#define PLUGIN_STR_VERSION "1.0.0" - -#define _my_thread_var loc_thread_var +#define PLUGIN_VERSION 0x20000 +#define PLUGIN_STR_VERSION "2.0"
#include <my_config.h> -#include <assert.h> #include <my_global.h> #include <my_base.h> -#include <typelib.h> -//#include <mysql_com.h> /* for enum enum_server_command */ -#include <mysql/plugin.h> #include <mysql/plugin_audit.h> -//#include <string.h> - +#include <mysql.h>
shouldn't be needed
-LEX_STRING * thd_query_string (MYSQL_THD thd); -unsigned long long thd_query_id(const MYSQL_THD thd); -size_t thd_query_safe(MYSQL_THD thd, char *buf, size_t buflen); -const char *thd_user_name(MYSQL_THD thd); -const char *thd_client_host(MYSQL_THD thd); -const char *thd_client_ip(MYSQL_THD thd); -LEX_CSTRING *thd_current_db(MYSQL_THD thd); -int thd_current_status(MYSQL_THD thd); -enum enum_server_command thd_current_command(MYSQL_THD thd); - -int maria_compare_hostname(const char *wild_host, long wild_ip, long ip_mask, - const char *host, const char *ip); -void maria_update_hostname(const char **wild_host, long *wild_ip, long *ip_mask, - const char *host);
/* Status variables for SHOW STATUS */ static long test_passed= 0; +static char *sql_text_local, *sql_text_global; +static char qwe_res[1024]= ""; + static struct st_mysql_show_var test_sql_status[]= { {"test_sql_service_passed", (char *)&test_passed, SHOW_LONG}, + {"test_sql_query_result", qwe_res, SHOW_CHAR}, {0,0,0} };
static my_bool do_test= TRUE; -static void run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, - void *var_ptr, const void *save); -static MYSQL_SYSVAR_BOOL(run_test, do_test, PLUGIN_VAR_OPCMDARG, - "Perform the test now.", NULL, run_test, FALSE); +static int run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); +static int run_sql_local(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); +static int run_sql_global(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); + +static void noop_update(MYSQL_THD thd, struct st_mysql_sys_var *var, + void *var_ptr, const void *save); + +static MYSQL_SYSVAR_BOOL(run_test, do_test, + PLUGIN_VAR_OPCMDARG, + "Perform the test now.", + run_test, NULL, FALSE); + +static MYSQL_SYSVAR_STR(execute_sql_local, sql_text_local, + PLUGIN_VAR_OPCMDARG, + "Create the new local connection, execute SQL statement with it.", + run_sql_local, noop_update, FALSE); + +static MYSQL_SYSVAR_STR(execute_sql_global, sql_text_global, + PLUGIN_VAR_OPCMDARG, + "Execute SQL statement using the global connection.", + run_sql_global, noop_update, FALSE);
Why did you test local and global connections? What difference in behavior can one expect?
+ static struct st_mysql_sys_var* test_sql_vars[]= { MYSQL_SYSVAR(run_test), + MYSQL_SYSVAR(execute_sql_local), + MYSQL_SYSVAR(execute_sql_global), NULL };
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index ef6f40778fe..8a941a68e53 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -5064,6 +5075,9 @@ static int init_server_components()
tc_log= 0; // ha_initialize_handlerton() needs that
+ if (ddl_log_initialize()) + unireg_abort(1);
why?
+ if (plugin_init(&remaining_argc, remaining_argv, (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) diff --git a/sql/sql_class.h b/sql/sql_class.h index e569fcd32d6..964626be3d4 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 09ad632dd98..4725855c130 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4857,6 +4870,7 @@ Prepared_statement::execute_server_runnable(Server_runnable *server_runnable) if (!(lex= new (mem_root) st_lex_local)) return TRUE;
+ thd->set_time();
why? May be it should be executed using the timestamp of the top-level statement?
thd->set_n_backup_statement(this, &stmt_backup); thd->set_n_backup_active_arena(this, &stmt_backup); thd->stmt_arena= this; @@ -6153,23 +6163,193 @@ static my_bool loc_read_query_result(MYSQL *mysql) + extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql, - const char *host, const char *user, const char *passwd, const char *db) + const char *host, const char *user, const char *db,
you aren't using `host` or `db` for anything. And I don't understand what you're using `user` for.
+ unsigned long clientflag) { - //char name_buff[USERNAME_LENGTH]; - + THD *thd_orig= current_thd; + THD *new_thd; + Protocol_local *p; DBUG_ENTER("mysql_real_connect_local");
/* Test whether we're already connected */ diff --git a/sql/thread_pool_info.cc b/sql/thread_pool_info.cc index 90ac6871784..e3ffd160a11 100644 --- a/sql/thread_pool_info.cc +++ b/sql/thread_pool_info.cc @@ -14,9 +14,9 @@ along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111 - 1301 USA*/
#include <mysql_version.h> -#include <mysql/plugin.h>
#include <my_global.h> +#include <mysql/plugin.h>
shouldn't be needed, mysql/plugin.h should not need my_global.h
#include <sql_class.h> #include <sql_i_s.h> #include <mysql/plugin.h>
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
For example, I'd think it connects to the current server, internally. But then, why does it need host and user?
Initially i thought it can be put in the thread's security_ctx so functions like CURRENT_USER return these values. After some more meditation i decided that it doesn't seem to be useful and just removed these arguments.
+ADD_DEFINITIONS(-DMYSQL_SERVER) Why?
Couldn't figure out how to get rid of it. Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h helps. I wasn't bold enough to mention the particular service in such an exposed file as mysql.h :)
+#include <mysql.h> shouldn't be needed My idea was that those not using the SQL service don't have to see the 'mysql.h' declarations. Now all the plugins see all these mysql_xxx() functions and related structures. Not sure if it's good.
Why did you test local and global connections? What difference in behavior can one expect?
Shouldn't be any difference in results. I test it though as the 'global' and the 'local' connection work differently. The 'global' does the mysql_real_connect() in the xxx_plugin_init() and disconnects in the plugin_deinit(), while the 'local' inited and closed in one function.
+ if (ddl_log_initialize()) + unireg_abort(1);
why?
So that call was moved to be before the 'plugin_init()' call. The 'CREATE TABLE' query in the test_sql_service_init() fails as the log is not initialized. The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f. Best regards. HF On Tue, Sep 7, 2021 at 5:58 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Alexey!
see questions below
On Sep 07, Alexey Botchkov wrote:
revision-id: b248988c8b6 (mariadb-10.6.1-74-gb248988c8b6) parent(s): 42ad4fa3464 author: Alexey Botchkov committer: Alexey Botchkov timestamp: 2021-09-07 14:08:13 +0400 message:
MDEV-19275 Provide SQL service to plugins.
SQL service added. It provides the limited set of client library functions to be used by plugin.
diff --git a/include/mysql/service_sql.h b/include/mysql/service_sql.h new file mode 100644 index 00000000000..5050793f655 --- /dev/null +++ b/include/mysql/service_sql.h @@ -0,0 +1,99 @@ +/* Copyright (C) 2021 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1301 USA */ + +#if defined(MYSQL_SERVER) && !defined MYSQL_SERVICE_SQL +#define MYSQL_SERVICE_SQL + +#include <mysql.h> + +/** + @file + SQL service + + Interface for plugins to execute SQL queries on the local server. + + Functions of the service are the 'server-limited' client library: + mysql_init + mysql_real_connect_local + mysql_real_connect + mysql_errno + mysql_error + mysql_real_query + mysql_affected_rows + mysql_num_rows + mysql_store_result + mysql_free_result + mysql_fetch_row + mysql_close +*/ + + +#ifdef __cplusplus +extern "C" { +#endif + +extern struct sql_service_st { + MYSQL * STDCALL (*mysql_init)(MYSQL *mysql); + MYSQL *(*mysql_real_connect_local)(MYSQL *mysql, + const char *host, const char *user, const char *db, + unsigned long clientflag); + MYSQL * STDCALL (*mysql_real_connect)(MYSQL *mysql, const char *host, + const char *user, const char *passwd, const char *db, unsigned int port, + const char *unix_socket, unsigned long clientflag); + unsigned int STDCALL (*mysql_errno)(MYSQL *mysql); + const char * STDCALL (*mysql_error)(MYSQL *mysql); + int STDCALL (*mysql_real_query)(MYSQL *mysql, const char *q, + unsigned long length); + my_ulonglong STDCALL (*mysql_affected_rows)(MYSQL *mysql); + my_ulonglong STDCALL (*mysql_num_rows)(MYSQL_RES *res); + MYSQL_RES * STDCALL (*mysql_store_result)(MYSQL *mysql); + void STDCALL (*mysql_free_result)(MYSQL_RES *result); + MYSQL_ROW STDCALL (*mysql_fetch_row)(MYSQL_RES *result); + void STDCALL (*mysql_close)(MYSQL *sock); +} *sql_service; + +#ifdef MYSQL_DYNAMIC_PLUGIN + +#define mysql_init sql_service->mysql_init +#define mysql_real_connect_local sql_service->mysql_real_connect_local +#define mysql_real_connect sql_service->mysql_real_connect +#define mysql_errno(M) sql_service->mysql_errno(M) +#define mysql_error(M) sql_service->mysql_error(M) +#define mysql_real_query sql_service->mysql_real_query +#define mysql_affected_rows sql_service->mysql_affected_rows +#define mysql_num_rows sql_service->mysql_num_rows +#define mysql_store_result sql_service->mysql_store_result +#define mysql_free_result sql_service->mysql_free_result +#define mysql_fetch_row sql_service->mysql_fetch_row +#define mysql_close sql_service->mysql_close + +#else + +MYSQL *mysql_real_connect_local(MYSQL *mysql, + const char *host, const char *user, const char *db, + unsigned long clientflag);
normally the service file is the authoritative source of the service documentation, everything there is to know about the service should be there.
In this case it's mostly the normal client C API, so no need to document it. But mysql_real_connect_local is new, so please, add a comment here, explaining what it is.
For example, I'd think it connects to the current server, internally. But then, why does it need host and user? The comment should cover it.
+ +/* The rest of the function declarations mest be taken from the mysql.h */ + +#endif /*MYSQL_DYNAMIC_PLUGIN*/ + + +#ifdef __cplusplus +} +#endif + +#endif /*MYSQL_SERVICE_SQL */ + + diff --git a/plugin/test_sql_service/CMakeLists.txt b/plugin/test_sql_service/CMakeLists.txt index aa9ecfe685e..7c61a1c3c7a 100644 --- a/plugin/test_sql_service/CMakeLists.txt +++ b/plugin/test_sql_service/CMakeLists.txt @@ -15,4 +15,5 @@
SET(SOURCES test_sql_service.c)
-MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY RECOMPILE_FOR_EMBEDDED) +ADD_DEFINITIONS(-DMYSQL_SERVER)
why?
+MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY) diff --git a/plugin/test_sql_service/test_sql_service.c b/plugin/test_sql_service/test_sql_service.c index 062f10fce58..e1155a98c40 100644 --- a/plugin/test_sql_service/test_sql_service.c +++ b/plugin/test_sql_service/test_sql_service.c @@ -14,71 +14,113 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */
-#define PLUGIN_VERSION 0x100 -#define PLUGIN_STR_VERSION "1.0.0" - -#define _my_thread_var loc_thread_var +#define PLUGIN_VERSION 0x20000 +#define PLUGIN_STR_VERSION "2.0"
#include <my_config.h> -#include <assert.h> #include <my_global.h> #include <my_base.h> -#include <typelib.h> -//#include <mysql_com.h> /* for enum enum_server_command */ -#include <mysql/plugin.h> #include <mysql/plugin_audit.h> -//#include <string.h> - +#include <mysql.h>
shouldn't be needed
-LEX_STRING * thd_query_string (MYSQL_THD thd); -unsigned long long thd_query_id(const MYSQL_THD thd); -size_t thd_query_safe(MYSQL_THD thd, char *buf, size_t buflen); -const char *thd_user_name(MYSQL_THD thd); -const char *thd_client_host(MYSQL_THD thd); -const char *thd_client_ip(MYSQL_THD thd); -LEX_CSTRING *thd_current_db(MYSQL_THD thd); -int thd_current_status(MYSQL_THD thd); -enum enum_server_command thd_current_command(MYSQL_THD thd); - -int maria_compare_hostname(const char *wild_host, long wild_ip, long
ip_mask,
- const char *host, const char *ip); -void maria_update_hostname(const char **wild_host, long *wild_ip, long *ip_mask, - const char *host);
/* Status variables for SHOW STATUS */ static long test_passed= 0; +static char *sql_text_local, *sql_text_global; +static char qwe_res[1024]= ""; + static struct st_mysql_show_var test_sql_status[]= { {"test_sql_service_passed", (char *)&test_passed, SHOW_LONG}, + {"test_sql_query_result", qwe_res, SHOW_CHAR}, {0,0,0} };
static my_bool do_test= TRUE; -static void run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, - void *var_ptr, const void *save); -static MYSQL_SYSVAR_BOOL(run_test, do_test, PLUGIN_VAR_OPCMDARG, - "Perform the test now.", NULL, run_test, FALSE); +static int run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); +static int run_sql_local(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); +static int run_sql_global(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, + struct st_mysql_value *value); + +static void noop_update(MYSQL_THD thd, struct st_mysql_sys_var *var, + void *var_ptr, const void *save); + +static MYSQL_SYSVAR_BOOL(run_test, do_test, + PLUGIN_VAR_OPCMDARG, + "Perform the test now.", + run_test, NULL, FALSE); + +static MYSQL_SYSVAR_STR(execute_sql_local, sql_text_local, + PLUGIN_VAR_OPCMDARG, + "Create the new local connection, execute SQL statement with it.", + run_sql_local, noop_update, FALSE); + +static MYSQL_SYSVAR_STR(execute_sql_global, sql_text_global, + PLUGIN_VAR_OPCMDARG, + "Execute SQL statement using the global connection.", + run_sql_global, noop_update, FALSE);
Why did you test local and global connections? What difference in behavior can one expect?
+ static struct st_mysql_sys_var* test_sql_vars[]= { MYSQL_SYSVAR(run_test), + MYSQL_SYSVAR(execute_sql_local), + MYSQL_SYSVAR(execute_sql_global), NULL };
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index ef6f40778fe..8a941a68e53 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -5064,6 +5075,9 @@ static int init_server_components()
tc_log= 0; // ha_initialize_handlerton() needs that
+ if (ddl_log_initialize()) + unireg_abort(1);
why?
+ if (plugin_init(&remaining_argc, remaining_argv, (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) diff --git a/sql/sql_class.h b/sql/sql_class.h index e569fcd32d6..964626be3d4 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 09ad632dd98..4725855c130 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4857,6 +4870,7 @@ Prepared_statement::execute_server_runnable(Server_runnable *server_runnable) if (!(lex= new (mem_root) st_lex_local)) return TRUE;
+ thd->set_time();
why? May be it should be executed using the timestamp of the top-level statement?
thd->set_n_backup_statement(this, &stmt_backup); thd->set_n_backup_active_arena(this, &stmt_backup); thd->stmt_arena= this; @@ -6153,23 +6163,193 @@ static my_bool loc_read_query_result(MYSQL *mysql) + extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql, - const char *host, const char *user, const char *passwd, const char *db) + const char *host, const char *user, const char *db,
you aren't using `host` or `db` for anything. And I don't understand what you're using `user` for.
+ unsigned long clientflag) { - //char name_buff[USERNAME_LENGTH]; - + THD *thd_orig= current_thd; + THD *new_thd; + Protocol_local *p; DBUG_ENTER("mysql_real_connect_local");
/* Test whether we're already connected */ diff --git a/sql/thread_pool_info.cc b/sql/thread_pool_info.cc index 90ac6871784..e3ffd160a11 100644 --- a/sql/thread_pool_info.cc +++ b/sql/thread_pool_info.cc @@ -14,9 +14,9 @@ along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111 - 1301 USA*/
#include <mysql_version.h> -#include <mysql/plugin.h>
#include <my_global.h> +#include <mysql/plugin.h>
shouldn't be needed, mysql/plugin.h should not need my_global.h
#include <sql_class.h> #include <sql_i_s.h> #include <mysql/plugin.h>
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexey! On Sep 10, Alexey Botchkov wrote:
For example, I'd think it connects to the current server, internally. But then, why does it need host and user?
Initially i thought it can be put in the thread's security_ctx so functions like CURRENT_USER return these values. After some more meditation i decided that it doesn't seem to be useful and just removed these arguments.
What security context are you using now? root? whatever happen to be in the THD? What if you create a new THD?
+ADD_DEFINITIONS(-DMYSQL_SERVER) Why? Couldn't figure out how to get rid of it. Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h helps. I wasn't bold enough to mention the particular service in such an exposed file as mysql.h :)
Feel free to replace it with any other macro that is guaranteed to be set for the server code (including plugins and anything that doesn't define MYSQL_SERVER, but still is in the server). Any plugin related define should work, I think.
+#include <mysql.h> shouldn't be needed My idea was that those not using the SQL service don't have to see the 'mysql.h' declarations. Now all the plugins see all these mysql_xxx() functions and related structures. Not sure if it's good.
I simply mean that you already include mysql.h in the service_sql.h. If you wouldn't, then yes, a separate include would be needed.
The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f.
Thanks. One unanswered question below:
diff --git a/sql/sql_class.h b/sql/sql_class.h index e569fcd32d6..964626be3d4 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 09ad632dd98..4725855c130 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4857,6 +4870,7 @@ Prepared_statement::execute_server_runnable(Server_runnable *server_runnable) if (!(lex= new (mem_root) st_lex_local)) return TRUE;
+ thd->set_time();
why? May be it should be executed using the timestamp of the top-level statement?
thd->set_n_backup_statement(this, &stmt_backup); thd->set_n_backup_active_arena(this, &stmt_backup); thd->stmt_arena= this;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
What security context are you using now? root? whatever happen to be in the THD? What if you create a new THD?
The thd->security_ctx->skip_grants() is done for the created THD. Otherwise yes, the current context is used. Going to fix that with the local empty context.
Feel free to replace it with any other macro that is guaranteed to be set for the server code. Since at the moment it only affects the SQL service plugin, i'd keep it as you did. Just adding the comment. If something else starts to depend on it, then we'll better add a specific define for it.
+ thd->set_time();
why? May be it should be executed using the timestamp of the top-level statement?
When that can be desirable? I did that thinking 'what if the set_time() for the current thread wasn't called at all'. Though removed this for now. So the patch with the abovementioned changes. 9ad45b41d385fca8215880cce1757ebe7f2396eb Best regards. HF On Fri, Sep 10, 2021 at 7:54 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Alexey!
On Sep 10, Alexey Botchkov wrote:
For example, I'd think it connects to the current server, internally. But then, why does it need host and user?
Initially i thought it can be put in the thread's security_ctx so functions like CURRENT_USER return these values. After some more meditation i decided that it doesn't seem to be useful and just removed these arguments.
What security context are you using now? root? whatever happen to be in the THD? What if you create a new THD?
+ADD_DEFINITIONS(-DMYSQL_SERVER) Why? Couldn't figure out how to get rid of it. Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h helps. I wasn't bold enough to mention the particular service in such an exposed file as mysql.h :)
Feel free to replace it with any other macro that is guaranteed to be set for the server code (including plugins and anything that doesn't define MYSQL_SERVER, but still is in the server). Any plugin related define should work, I think.
+#include <mysql.h> shouldn't be needed My idea was that those not using the SQL service don't have to see the 'mysql.h' declarations. Now all the plugins see all these mysql_xxx() functions and related structures. Not sure if it's good.
I simply mean that you already include mysql.h in the service_sql.h.
If you wouldn't, then yes, a separate include would be needed.
The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f.
Thanks. One unanswered question below:
diff --git a/sql/sql_class.h b/sql/sql_class.h index e569fcd32d6..964626be3d4 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 09ad632dd98..4725855c130 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4857,6 +4870,7 @@ Prepared_statement::execute_server_runnable(Server_runnable *server_runnable) if (!(lex= new (mem_root) st_lex_local)) return TRUE;
+ thd->set_time();
why? May be it should be executed using the timestamp of the top-level statement?
thd->set_n_backup_statement(this, &stmt_backup); thd->set_n_backup_active_arena(this, &stmt_backup); thd->stmt_arena= this;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexey! Looks good to me, thanks. Please tell Sanja to rebase on top of that, push the result into a preview-* branch, and update the TODO.
So the patch with the abovementioned changes. 9ad45b41d385fca8215880cce1757ebe7f2396eb
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei. Unfortunately the taken approach led to compiler errors on Windows and (seemingly) the rpl_shutdown_wait_slaves test failure. So i made the change - to preserve the way we do the includes. 76c4616c2cc47ecc85362d0c5a2bffd9495e4753 Best regards. HF On Mon, Sep 13, 2021 at 12:42 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Alexey!
Looks good to me, thanks. Please tell Sanja to rebase on top of that, push the result into a preview-* branch, and update the TODO.
So the patch with the abovementioned changes. 9ad45b41d385fca8215880cce1757ebe7f2396eb
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexey! I suggest a different fix: ====================================================================== diff --git a/plugin/type_mysql_json/type.cc b/plugin/type_mysql_json/type.cc index 61507a24d92..8b79e039106 100644 --- a/plugin/type_mysql_json/type.cc +++ b/plugin/type_mysql_json/type.cc @@ -14,8 +14,8 @@ along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ -#include <mysql/plugin_data_type.h> #include <my_global.h> +#include <mysql/plugin_data_type.h> #include <sql_type.h> #include <field.h> #include <mysqld_error.h> diff --git a/storage/example/ha_example.cc b/storage/example/ha_example.cc index 30484a41087..868d30d2081 100644 --- a/storage/example/ha_example.cc +++ b/storage/example/ha_example.cc @@ -98,7 +98,7 @@ #pragma implementation // gcc: Class implementation #endif -#include <my_config.h> +#include <my_global.h> #include <mysql/plugin.h> #include "ha_example.h" #include "sql_class.h" ====================================================================== Let's just accept that *if* one wants to include my_global.h, it *has* to be included first. As Monty wants to be the case anyway. On Sep 14, Alexey Botchkov wrote:
Hi, Sergei.
Unfortunately the taken approach led to compiler errors on Windows and (seemingly) the rpl_shutdown_wait_slaves test failure. So i made the change - to preserve the way we do the includes. 76c4616c2cc47ecc85362d0c5a2bffd9495e4753
Best regards. HF
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexey Botchkov -
Sergei Golubchik