maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

Just a couple of comments, see below:

On Oct 16, Alexander Barkov wrote:
> revision-id: a0e3bd09251 (mariadb-10.4.4-419-ga0e3bd09251)
> parent(s): 22b645ef529
> author: Alexander Barkov <bar@xxxxxxxxxxx>
> committer: Alexander Barkov <bar@xxxxxxxxxxx>
> timestamp: 2019-10-16 16:26:29 +0400
> message:
> 
> Part1: MDEV-20837 Add MariaDB_FUNCTION_PLUGIN
> 
> - Defining MariaDB_FUNCTION_PLUGIN
> - Changing the code in /plugins/type_inet/ and /plugins/type_test/
>   to use MariaDB_FUNCTION_PLUGIN instead of MariaDB_FUNCTION_COLLECTION_PLUGIN.

> diff --git a/include/mysql/plugin_function.h b/include/mysql/plugin_function.h
> new file mode 100644
> index 00000000000..4ce416612e9
> --- /dev/null
> +++ b/include/mysql/plugin_function.h
> @@ -0,0 +1,58 @@
> +#ifndef MARIADB_PLUGIN_FUNCTION_INCLUDED
> +#define MARIADB_PLUGIN_FUNCTION_INCLUDED
> +/* Copyright (C) 2019, Alexander Barkov and MariaDB
> +
> +   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 St, Fifth Floor, Boston, MA 02110-1335  USA */
> +
> +/**
> +  @file
> +
> +  Function Plugin API.
> +
> +  This file defines the API for server plugins that manage functions.
> +*/
> +
> +#ifdef __cplusplus
> +
> +#include <mysql/plugin.h>
> +
> +/*
> +  API for function plugins. (MariaDB_FUNCTION_PLUGIN)
> +*/
> +#define MariaDB_FUNCTION_INTERFACE_VERSION (MYSQL_VERSION_ID << 8)
> +
> +
> +class Plugin_function
> +{
> +  int m_interface_version;
> +  Create_func *m_builder;
> +public:
> +  Plugin_function(int interface_version, Create_func *builder)
> +   :m_interface_version(interface_version),
> +    m_builder(builder)
> +  { }

Why do you need this ^^^ constructor?

> +  Plugin_function(Create_func *builder)
> +   :m_interface_version(MariaDB_FUNCTION_INTERFACE_VERSION),
> +    m_builder(builder)
> +  { }
> +  Create_func *create_func()
> +  {
> +    return m_builder;
> +  }
> +};
> +
> +
> +#endif /* __cplusplus */
> +
> +#endif /* MARIADB_PLUGIN_FUNCTION_INCLUDED */
> diff --git a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result
> index 4663ae485e2..a9422f2e4fd 100644
> --- a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result
> +++ b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result
> @@ -15,14 +16,94 @@ PLUGIN_LICENSE,
>  PLUGIN_MATURITY,
>  PLUGIN_AUTH_VERSION
>  FROM INFORMATION_SCHEMA.PLUGINS
> -WHERE PLUGIN_TYPE='FUNCTION COLLECTION'
> -    AND PLUGIN_NAME='func_inet';
> -PLUGIN_NAME	func_inet
> +WHERE PLUGIN_TYPE='FUNCTION'
> +  AND PLUGIN_NAME IN
> +('inet_aton',
> +'inet_ntoa',
> +'inet6_aton',
> +'inet6_ntoa',
> +'is_ipv4',
> +'is_ipv6',
> +'is_ipv4_compat',
> +'is_ipv4_mapped')
> +ORDER BY PLUGIN_NAME;
> +----	----
> +PLUGIN_NAME	inet6_aton
>  PLUGIN_VERSION	1.0
>  PLUGIN_STATUS	ACTIVE
> -PLUGIN_TYPE	FUNCTION COLLECTION
> +PLUGIN_TYPE	FUNCTION
>  PLUGIN_AUTHOR	MariaDB Corporation
> -PLUGIN_DESCRIPTION	Function collection test
> +PLUGIN_DESCRIPTION	Function INET6_ATON()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental

INET* functions should probably be Alpha, not Experimental.
You expect them to be GA one day, don't you?

> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	inet6_ntoa
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function INET6_NTOA()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	inet_aton
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function INET_ATON()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	inet_ntoa
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function INET_NTOA()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	is_ipv4
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function IS_IPV4()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	is_ipv4_compat
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function IS_IPV4_COMPAT()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	is_ipv4_mapped
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function IS_IPV4_MAPPED()
> +PLUGIN_LICENSE	GPL
> +PLUGIN_MATURITY	Experimental
> +PLUGIN_AUTH_VERSION	1.0
> +----	----
> +PLUGIN_NAME	is_ipv6
> +PLUGIN_VERSION	1.0
> +PLUGIN_STATUS	ACTIVE
> +PLUGIN_TYPE	FUNCTION
> +PLUGIN_AUTHOR	MariaDB Corporation
> +PLUGIN_DESCRIPTION	Function IS_IPV6()
>  PLUGIN_LICENSE	GPL
>  PLUGIN_MATURITY	Experimental
>  PLUGIN_AUTH_VERSION	1.0
> diff --git a/sql/item_create.cc b/sql/item_create.cc
> index e8eb76dfc12..e316723cedf 100644
> --- a/sql/item_create.cc
> +++ b/sql/item_create.cc
> @@ -5704,12 +5657,26 @@ void item_create_cleanup()
>  {
>    DBUG_ENTER("item_create_cleanup");
>    my_hash_free(& native_functions_hash);
> -#ifdef HAVE_SPATIAL
> -  plugin_function_collection_geometry.deinit();
> -#endif
>    DBUG_VOID_RETURN;
>  }
>  
> +
> +static Create_func *
> +function_plugin_find_native_function_builder(THD *thd, const LEX_CSTRING &name)
> +{
> +  plugin_ref plugin;
> +  if ((plugin= my_plugin_lock_by_name(thd, &name, MariaDB_FUNCTION_PLUGIN)))
> +  {
> +    Create_func *builder=
> +      reinterpret_cast<Plugin_function*>(plugin_decl(plugin)->info)->
> +        create_func();
> +    plugin_unlock(thd, plugin);
> +    return builder;
> +  }
> +  return NULL;

So, function plugins cannot be unlocked either?

> +}
> +
> +
>  Create_func *
>  find_native_function_builder(THD *thd, const LEX_CSTRING *name)
>  {
> @@ -5724,16 +5691,10 @@ find_native_function_builder(THD *thd, const LEX_CSTRING *name)
>    if (func && (builder= func->builder))
>      return builder;
>  
> -  if ((builder= Plugin_find_native_func_builder_param(*name).find(thd)))
> +  if ((builder= function_plugin_find_native_function_builder(thd, *name)))

this is what I mean by "special code path for plugins" and want to get
rid of. But not in this commit, I agree.

>      return builder;
>  
> -#ifdef HAVE_SPATIAL
> -  if (!builder)
> -    builder= plugin_function_collection_geometry.
> -               find_native_function_builder(thd, *name);
> -#endif
> -
> -  return builder;
> +  return NULL;
>  }
>  
>  Create_qfunc *

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx