Hi, Alexander!

On Sep 16, Alexander Barkov wrote:
> revision-id: 3f38da9145c (mariadb-10.4.4-251-g3f38da9145c)
> parent(s): e6ff3f9d1c8
> author: Alexander Barkov <b...@mariadb.com>
> committer: Alexander Barkov <b...@mariadb.com>
> timestamp: 2019-07-12 07:53:55 +0400
> message:
> 
> MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN

> diff --git a/include/mysql/plugin.h b/include/mysql/plugin.h
> index 85e52a247af..92703b626ac 100644
> --- a/include/mysql/plugin.h
> +++ b/include/mysql/plugin.h
> @@ -611,6 +612,22 @@ struct handlerton;
>     int interface_version;
>   };
>  
> +/*
> +  API for data type plugin. (MariaDB_DATA_TYPE_PLUGIN)
> +*/
> +#define MariaDB_DATA_TYPE_INTERFACE_VERSION (MYSQL_VERSION_ID << 8)
> +
> +/**
> +   Data type plugin descriptor
> +*/
> +#ifdef __cplusplus
> +struct st_mariadb_data_type
> +{
> +  int interface_version;
> +  const class Type_handler *type_handler;
> +};
> +#endif

Plugin-wise it'd be better to have a separate plugin_data_type.h
and put Type_handler definition there.

So that as much of the API as possible gets into the .pp
file and we could detect when it changes.

> +
>  /*************************************************************************
>    st_mysql_value struct for reading values from mysqld.
>    Used by server variables framework to parse user-provided values.
> diff --git a/plugin/type_test/CMakeLists.txt b/plugin/type_test/CMakeLists.txt
> new file mode 100644
> index 00000000000..b85168d1bd2
> --- /dev/null
> +++ b/plugin/type_test/CMakeLists.txt
> @@ -0,0 +1,17 @@
> +# Copyright (c) 2016, MariaDB corporation. All rights reserved.
> +# 
> +# 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  02110-1335  
> USA
> +
> +MYSQL_ADD_PLUGIN(type_test plugin.cc RECOMPILE_FOR_EMBEDDED
> +                 MODULE_ONLY COMPONENT Test)

Add at least two new plugins, please.
May be in separate commits, as you like.
But at least two.
And practical, not just to test the API

> diff --git a/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test 
> b/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test
> new file mode 100644
> index 00000000000..30e313481d6
> --- /dev/null
> +++ b/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test
> @@ -0,0 +1,11 @@
> +--echo #
> +--echo # MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
> +--echo #
> +
> +SET SESSION debug_dbug="+d,frm_data_type_info";
> +
> +CREATE TABLE t1 (a TEST_INT8);
> +SHOW CREATE TABLE t1;
> +DROP TABLE t1;
> +
> +SET SESSION debug_dbug="-d,frm_data_type_info";

don't do that, always save old debug_dbug value instead and restore it later.

 SET @old_debug_dbug=@@debug_dbug;
 SET @@debug_dbug="+d,frm_data_type_info";
 ...
 SET @@debug_dbug=@old_debug_dbug;

because after "+d,xxx" you have one keyword enabled, like in
@@debug_dbug="d,xxx".  and after "-d,xxx" you have no keywords enabled,
like in @debug_dbug="d" which means "all keywords enabled" for dbug.

> diff --git a/plugin/type_test/mysql-test/type_test/type_test_int8.result 
> b/plugin/type_test/mysql-test/type_test/type_test_int8.result
> new file mode 100644
> index 00000000000..758f94904c1
> --- /dev/null
> +++ b/plugin/type_test/mysql-test/type_test/type_test_int8.result
> @@ -0,0 +1,144 @@
> +#
> +# MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
> +#
> +SELECT
> +PLUGIN_NAME,
> +PLUGIN_VERSION,
> +PLUGIN_STATUS,
> +PLUGIN_TYPE,
> +PLUGIN_AUTHOR,
> +PLUGIN_DESCRIPTION,
> +PLUGIN_LICENSE,
> +PLUGIN_MATURITY,
> +PLUGIN_AUTH_VERSION
> +FROM INFORMATION_SCHEMA.PLUGINS
> +WHERE PLUGIN_TYPE='DATA TYPE'
> +    AND PLUGIN_NAME='TEST_INT8';
> +PLUGIN_NAME  TEST_INT8
> +PLUGIN_VERSION       1.0
> +PLUGIN_STATUS        ACTIVE
> +PLUGIN_TYPE  DATA TYPE
> +PLUGIN_AUTHOR        MariaDB
> +PLUGIN_DESCRIPTION   Data type TEST_INT8
> +PLUGIN_LICENSE       GPL
> +PLUGIN_MATURITY      Alpha
> +PLUGIN_AUTH_VERSION  1.0
> +CREATE TABLE t1 (a TEST_INT8);
> +SHOW CREATE TABLE t1;
> +Table        Create Table
> +t1   CREATE TABLE `t1` (
> +  `a` test_int8 DEFAULT NULL

is the type name a reserved word? or an arbitrary identifier?
should we support (and print as)

    `a` `test_int8` DEFAULT NULL

?

> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> +SELECT CAST('100' AS TEST_INT8) AS cast;
> +cast
> +100
> +BEGIN NOT ATOMIC
> +DECLARE a TEST_INT8 DEFAULT 256;
> +SELECT HEX(a), a;
> +END;
> +$$
> +HEX(a)       a
> +100  256
> +CREATE FUNCTION f1(p TEST_INT8) RETURNS TEST_INT8 RETURN 1;
> +SHOW CREATE FUNCTION f1;
> +Function     sql_mode        Create Function character_set_client    
> collation_connection    Database Collation
> +f1   
> STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
>        CREATE DEFINER=`root`@`localhost` FUNCTION `f1`(p TEST_INT8) RETURNS 
> test_int8
> +RETURN 1     latin1  latin1_swedish_ci       latin1_swedish_ci

use enable_metadata for a part (or for a whole) of this test

and show how mysql --column-type-info handles it.

> +SELECT f1(10);
> +f1(10)
> +1
> +DROP FUNCTION f1;
> +CREATE TABLE t1 (a TEST_INT8);
> +CREATE TABLE t2 AS SELECT a FROM t1;
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `a` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +CREATE TABLE t2 AS SELECT COALESCE(a,a) FROM t1;
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `COALESCE(a,a)` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +CREATE TABLE t2 AS SELECT LEAST(a,a) FROM t1;
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `LEAST(a,a)` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +INSERT INTO t1 VALUES (1),(2);
> +CREATE TABLE t2 AS SELECT MIN(a), MAX(a) FROM t1;
> +SELECT * FROM t2;
> +MIN(a)       MAX(a)
> +1    2
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `MIN(a)` test_int8 DEFAULT NULL,
> +  `MAX(a)` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (id INT, a TEST_INT8);
> +INSERT INTO t1 VALUES (1,1),(1,2),(2,1),(2,2);
> +CREATE TABLE t2 AS SELECT id, MIN(a), MAX(a) FROM t1 GROUP BY id;
> +SELECT * FROM t2;
> +id   MIN(a)  MAX(a)
> +1    1       2
> +2    1       2
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `id` int(11) DEFAULT NULL,
> +  `MIN(a)` test_int8 DEFAULT NULL,
> +  `MAX(a)` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +INSERT INTO t1 VALUES (1),(2);
> +CREATE TABLE t2 AS SELECT DISTINCT a FROM t1;
> +SELECT * FROM t2;
> +a
> +1
> +2
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `a` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +INSERT INTO t1 VALUES (1);
> +CREATE TABLE t2 AS SELECT (SELECT a FROM t1) AS c1;
> +SELECT * FROM t2;
> +c1
> +1
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `c1` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;
> +CREATE TABLE t1 (a TEST_INT8);
> +CREATE TABLE t2 AS SELECT a FROM t1 UNION SELECT a FROM t1;
> +SHOW CREATE TABLE t2;
> +Table        Create Table
> +t2   CREATE TABLE `t2` (
> +  `a` test_int8 DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t2;
> +DROP TABLE t1;

CREATE LIKE, ALTER TABLE, SHOW COLUMNS, I_S.COLUMNS

Does it support indexing?

> diff --git a/plugin/type_test/plugin.cc b/plugin/type_test/plugin.cc
> new file mode 100644
> index 00000000000..ea70c70f786
> --- /dev/null
> +++ b/plugin/type_test/plugin.cc
> @@ -0,0 +1,120 @@
> +/*
> +   Copyright (c) 2000, 2015, Oracle and/or its affiliates.
> +   Copyright (c) 2009, 2019, 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-1301  
> USA */
> +
> +#include <my_global.h>
> +#include <sql_class.h>          // THD
> +#include <mysql/plugin.h>
> +#include "sql_type.h"
> +
> +
> +class Field_test_int8 :public Field_longlong
> +{
> +public:
> +  Field_test_int8(const LEX_CSTRING &name, const Record_addr &addr,
> +                  enum utype unireg_check_arg,
> +                  uint32 len_arg, bool zero_arg, bool unsigned_arg)
> +    :Field_longlong(addr.ptr(), len_arg, addr.null_ptr(), addr.null_bit(),
> +                    Field::NONE, &name, zero_arg, unsigned_arg)
> +  {}
> +  void sql_type(String &res) const
> +  {
> +    CHARSET_INFO *cs= res.charset();
> +    res.length(cs->cset->snprintf(cs,(char*) res.ptr(),res.alloced_length(),
> +               "test_int8"));

isn't there an easier way of setting a String to a specific value?
Like, res.copy() or something?

By the way, why do you need to do it at all, if the parent's Field::sql_type
method could set the value from type_handler()->name() ?

> +    // UNSIGNED and ZEROFILL flags are not supported by the parser yet.
> +    // add_zerofill_and_unsigned(res);
> +  }
> +  const Type_handler *type_handler() const;
> +};
> +
> +
> +class Type_handler_test_int8: public Type_handler_longlong
> +{
> +public:
> +  const Name name() const override
> +  {
> +    static Name name(STRING_WITH_LEN("test_int8"));

I'd prefer this being picked up automatically from the plugin name.
Like it's done for engines, I_S tables, auth plugins, ft parsers, etc.

> +    return name;
> +  }
> +  bool Column_definition_data_type_info_image(Binary_string *to,
> +                                              const Column_definition &def)
> +                                              const override
> +  {
> +    return to->append(Type_handler_test_int8::name().lex_cstring());
> +  }

Not sure, why this has to be overridden by the derived class.

> +  Field *make_table_field(MEM_ROOT *root,
> +                          const LEX_CSTRING *name,
> +                          const Record_addr &addr,
> +                          const Type_all_attributes &attr,
> +                          TABLE *table) const override
> +  {
> +    return new (root)
> +           Field_test_int8(*name, addr, Field::NONE,
> +                           attr.max_char_length(),
> +                           0/*zerofill*/,
> +                           attr.unsigned_flag);
> +  }
> +
> +  Field *make_table_field_from_def(TABLE_SHARE *share, MEM_ROOT *root,
> +                                   const LEX_CSTRING *name,
> +                                   const Record_addr &rec, const Bit_addr 
> &bit,
> +                                   const Column_definition_attributes *attr,
> +                                   uint32 flags) const override
> +  {
> +    return new (root)
> +      Field_test_int8(*name, rec, attr->unireg_check,
> +                      (uint32) attr->length,
> +                      f_is_zerofill(attr->pack_flag) != 0,
> +                      f_is_dec(attr->pack_flag) == 0);
> +  }
> +};

Why do you need two different methods? I'd expect one
that does new Field_test_int8() to be enough.

The second can be in the parent class, calling make_table_field().
Or both could be in the parent class, calling the only
virtual method that actually does new (root) Field_test_int8(...)

> +
> +static Type_handler_test_int8 type_handler_test_int8;
> +
> +
> +const Type_handler *Field_test_int8::type_handler() const
> +{
> +  return &type_handler_test_int8;
> +}
> +
> +
> +/*************************************************************************/
> +
> +static struct st_mariadb_data_type data_type_test_plugin=
> +{
> +  MariaDB_DATA_TYPE_INTERFACE_VERSION,
> +  &type_handler_test_int8
> +};

It's be more interesting to have a distinct type, not just an alias
for BIGINT. E.g. 7-byte integer.

As an example, it's a pretty empty plugin, it doesn't show
why this API was ever created. Add something non-trivial to it please.

And some real, non-test, plugin in a separate commit.

> +
> +
> +maria_declare_plugin(type_geom)
> +{
> +  MariaDB_DATA_TYPE_PLUGIN,     // the plugin type (see 
> include/mysql/plugin.h)
> +  &data_type_test_plugin,       // pointer to type-specific plugin descriptor
> +  "TEST_INT8",                  // plugin name
> +  "MariaDB",                    // plugin author

MariaDB Corporation ?

> +  "Data type TEST_INT8",        // the plugin description
> +  PLUGIN_LICENSE_GPL,           // the plugin license (see 
> include/mysql/plugin.h)
> +  0,                            // Pointer to plugin initialization function
> +  0,                            // Pointer to plugin deinitialization 
> function
> +  0x0100,                       // Numeric version 0xAABB means AA.BB veriosn
> +  NULL,                         // Status variables
> +  NULL,                         // System variables
> +  "1.0",                        // String version representation
> +  MariaDB_PLUGIN_MATURITY_ALPHA // Maturity (see include/mysql/plugin.h)*/

EXPERIMENTAL (for test plugins. ALPHA kind of implies it'll be BETA, GAMMA
and STABLE eventually)

> +}
> +maria_declare_plugin_end;
> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> index 5cecd9f50f7..988619cb5f9 100644
> --- a/sql/sql_type.cc
> +++ b/sql/sql_type.cc
> @@ -121,8 +121,23 @@ bool Type_handler_data::init()
>  
>  
>  const Type_handler *
> -Type_handler::handler_by_name(const LEX_CSTRING &name)
> +Type_handler::handler_by_name(THD *thd, const LEX_CSTRING &name)
>  {
> +  plugin_ref plugin;
> +  if ((plugin= my_plugin_lock_by_name(thd, &name, MariaDB_DATA_TYPE_PLUGIN)))
> +  {
> +    /*
> +      INSTALL PLUGIN is not fully supported for data type plugins yet.

Why? What's not supported?

> +      Fow now we have only mandatory built-in plugins
> +      and dynamic plugins for test purposes,
> +      Should be safe to unlock the plugin immediately.
> +    */
> +    const Type_handler *ph= reinterpret_cast<st_mariadb_data_type*>
> +                              (plugin_decl(plugin)->info)->type_handler;
> +    plugin_unlock(thd, plugin);
> +    return ph;
> +  }
> +
>  #ifdef HAVE_SPATIAL
>    const Type_handler *ha= type_collection_geometry.handler_by_name(name);
>    if (ha)
> diff --git a/sql/sql_type.h b/sql/sql_type.h
> index 8f2d4d0c49d..b01330f30e4 100644
> --- a/sql/sql_type.h
> +++ b/sql/sql_type.h
> @@ -3221,9 +3221,9 @@ class Information_schema_character_attributes
>  class Type_handler
>  {
>  protected:
> -  static const Name m_version_default;
> -  static const Name m_version_mysql56;
> -  static const Name m_version_mariadb53;
> +  static const MYSQL_PLUGIN_IMPORT Name m_version_default;
> +  static const MYSQL_PLUGIN_IMPORT Name m_version_mysql56;
> +  static const MYSQL_PLUGIN_IMPORT Name m_version_mariadb53;

Why do you need that? Parent's behavior should be always fine for plugins.
I don't think plugins should know about this at all.

>    String *print_item_value_csstr(THD *thd, Item *item, String *str) const;
>    String *print_item_value_temporal(THD *thd, Item *item, String *str,
>                                       const Name &type_name, String *buf) 
> const;
> @@ -5129,9 +5130,9 @@ class Type_handler_bool: public Type_handler_long
>  
>  class Type_handler_longlong: public Type_handler_general_purpose_int
>  {
> -  static const Name m_name_longlong;
> -  static const Type_limits_int m_limits_sint64;
> -  static const Type_limits_int m_limits_uint64;
> +  static const MYSQL_PLUGIN_IMPORT Name m_name_longlong;
> +  static const MYSQL_PLUGIN_IMPORT Type_limits_int m_limits_sint64;
> +  static const MYSQL_PLUGIN_IMPORT Type_limits_int m_limits_uint64;

same here

>  public:
>    virtual ~Type_handler_longlong() {}
>    const Name name() const { return m_name_longlong; }
> diff --git a/sql/table.cc b/sql/table.cc
> index ea333cb2ecd..70b3814df6c 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -2291,12 +2291,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, 
> bool write,
>  
>          if (field_data_type_info_array.count())
>          {
> +          const LEX_CSTRING &info= field_data_type_info_array.
> +                                     element(i).type_info();
>            DBUG_EXECUTE_IF("frm_data_type_info",
>              push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE,
>              ER_UNKNOWN_ERROR, "DBUG: [%u] name='%s' type_info='%.*s'",
>              i, share->fieldnames.type_names[i],
> -            (uint) field_data_type_info_array.element(i).type_info().length,
> -            field_data_type_info_array.element(i).type_info().str););
> +            (uint) info.length, info.str););
> +
> +          if (info.length)
> +          {
> +            const Type_handler *h= Type_handler::handler_by_name(thd, info);
> +            if (h)
> +              handler= h;
> +          }

I don't see where you handle the error that "unknown type"

>          }
>        }


Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org

_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to     : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Reply via email to