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