Hi, Alexander! On Sep 16, Alexander Barkov wrote:
revision-id: 3f38da9145c (mariadb-10.4.4-251-g3f38da9145c) parent(s): e6ff3f9d1c8 author: Alexander Barkov <bar@mariadb.com> committer: Alexander Barkov <bar@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 security@mariadb.org