maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxx>]

> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx
>