> 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 <[email protected]> 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 [email protected] >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
