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