Hi, Alexander! On Sep 17, Alexander Barkov wrote:
On 9/16/19 10:33 PM, Sergei Golubchik wrote:
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.
For now I defined interface version as:
#define MariaDB_DATA_TYPE_INTERFACE_VERSION (MYSQL_VERSION_ID << 8)
So, if I understand correctly, every distinct MySQL server version will require data type plugins compiled exactly for this version.
Right
Note, sql_type.h includes the following headers:
#include "mysqld.h" #include "sql_array.h" #include "sql_const.h" #include "sql_time.h" #include "sql_type_real.h" #include "compat56.h" C_MODE_START #include <ma_dyncol.h> C_MODE_END
and forward-defines around 60 classes and structures, e.g:
class Field; class Column_definition; class Column_definition_attributes; class Key_part_spec; class Item; ...
Does it also mean that all these 60 classes/structures should be a part of ABI?
Not necessarily, may be you can keep them as forward declarations?
Can these changes in sql_type.h and plugin_data_type.h be done under a separate MDEV later?
If we decide what to do with the version. Now it's 100500.0, then it'll be 100501.0, etc. If you stabilize the API in a separate MDEV and 10.5.0 is released meanwhile, the stable API version will have to be something like 200000.0, larger than any MYSQL_VERSION_ID << 8. If there will be no 10.5 releases between MDEV-20016 and this new separate MDEV, then the API versioning can start from 1.0.
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
I'll add INET6 as a full-featured plugin when MDEV-20016 is in. It will test all aspects of a data type.
Push them together, please.
The purpose of the current task is rather simple: - to introduce new plugin type - to make sure that data type plugins can write/read themselves to FRM file.
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
?
For now the grammar understands identifiers, but only those that are not keyword (neither reserved, nor non-reserved). Perhaps most of non-reserved keywords could be allowed as well without grammar conflicts. We can do it later.
sure
I'm not sure if we should add backticks. I'd prefer to limit the data type name not to have spaces or other special characters, so quoting is not required.
the problem, as usual, happens when a new MariaDB release adds new reserved words and the dump (or binlog) can no longer be applied. I'd say that we can not quote type names in two cases: * we don't care that after an upgrade old dumps or binlogs may become unusable * the grammar accepts any identifier (also reserved keywords) for the type name I don't like backticking everything and don't like breaking old dumps, so I'd like the second approach :) But I am not sure the parser will be able to parse unambiguously every statement if we allow reserved keywords as type names. Still worth checking out, but not much hope :(
+) 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
Done.
and show how mysql --column-type-info handles it.
This will be added when the protocol changes are in.
I think you can add it now to show what the client is receiving after this your patch. And when the protocol changes are in, the output will change.
+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"));
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() ?
Field_test_int8 derives from Field_longlong, so it has to override the method anyway.
Using type_handler()->name() would involve and extra virtual call which can be avoided in this particular case.
it's never used in tight loops, so I don't think one virtual call would make much of a difference. I'd rather suggest to keep the interface simpler and not require one more method. Or, as an option - make the base class sql_type to use type_handler()->name() and every individual plugin's Field_xxx could overwrite it, if it so wishes. But it won't be a requirement.
+ // 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.
Plugin names use upper case:
locale_info/locale_info.cc: "LOCALES", type_geom/plugin.cc: "SPATIAL_REF_SYS", type_geom/plugin.cc: "GEOMETRY_COLUMNS", type_test/plugin.cc: "TEST_INT8", wsrep_info/plugin.cc: "WSREP_MEMBERSHIP", wsrep_info/plugin.cc: "WSREP_STATUS",
All data types use lower case.
What do you propose?
First, not all: binlog mysql_native_password mysql_old_password wsrep CSV MEMORY Aria MyISAM MRG_MyISAM CLIENT_STATISTICS INDEX_STATISTICS TABLE_STATISTICS USER_STATISTICS SQL_SEQUENCE InnoDB INNODB_TRX INNODB_LOCKS INNODB_LOCK_WAITS INNODB_CMP INNODB_CMP_RESET INNODB_CMPMEM INNODB_CMPMEM_RESET INNODB_CMP_PER_INDEX INNODB_CMP_PER_INDEX_RESET INNODB_BUFFER_PAGE INNODB_BUFFER_PAGE_LRU INNODB_BUFFER_POOL_STATS INNODB_METRICS INNODB_FT_DEFAULT_STOPWORD INNODB_FT_DELETED INNODB_FT_BEING_DELETED INNODB_FT_CONFIG INNODB_FT_INDEX_CACHE INNODB_FT_INDEX_TABLE INNODB_SYS_TABLES INNODB_SYS_TABLESTATS INNODB_SYS_INDEXES INNODB_SYS_COLUMNS INNODB_SYS_FIELDS INNODB_SYS_FOREIGN INNODB_SYS_FOREIGN_COLS INNODB_SYS_TABLESPACES INNODB_SYS_DATAFILES INNODB_SYS_VIRTUAL INNODB_MUTEXES INNODB_SYS_SEMAPHORE_WAITS INNODB_TABLESPACES_ENCRYPTION INNODB_TABLESPACES_SCRUBBING PERFORMANCE_SCHEMA SEQUENCE unix_socket FEEDBACK user_variables partition daemon_example cracklib_password_check pam ARCHIVE test_versioning file_key_management SPHINX EXAMPLE UNUSABLE LOCALES WSREP_MEMBERSHIP WSREP_STATUS QUERY_CACHE_INFO simple_password_check qa_auth_interface SQL_ERROR_LOG pam FEDERATED METADATA_LOCK_INFO simple_parser qa_auth_server CONNECT FEDERATED ed25519 AUDIT_NULL handlersocket ROCKSDB ROCKSDB_CFSTATS ROCKSDB_DBSTATS ROCKSDB_PERF_CONTEXT ROCKSDB_PERF_CONTEXT_GLOBAL ROCKSDB_CF_OPTIONS ROCKSDB_COMPACTION_STATS ROCKSDB_GLOBAL_INFO ROCKSDB_DDL ROCKSDB_SST_PROPS ROCKSDB_INDEX_FILE_MAP ROCKSDB_LOCKS ROCKSDB_TRX ROCKSDB_DEADLOCK QUERY_RESPONSE_TIME QUERY_RESPONSE_TIME_AUDIT Mroonga Mroonga_stats DISKS test_plugin_server cleartext_plugin_server debug_key_management auth_0x0100 BLACKHOLE SPIDER SPIDER_ALLOC_MEM two_questions three_attempts TEST_SQL_DISCOVERY TokuDB TokuDB_trx TokuDB_lock_waits TokuDB_locks TokuDB_file_map TokuDB_fractal_tree_info TokuDB_fractal_tree_block_map TokuDB_background_job_status example_key_management SERVER_AUDIT aws_key_management gssapi handlersocket named_pipe It's up to the plugin author what letter case to use, there isn't any consistency here. So, you can use lower case if you'd like. Or you can copy and lower case the name when the plugin is loaded, if you want to ensure it is lower case. But don't force the plugin writers to repeat stuff that you know already.
+ 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.
The parent class Type_handler_longlong does not need to write its name into FRM, as it's identified by the type code, like all other built-in data types.
Then create a new Type_handler_longlong_plugin class that inherits from Type_handler_longlong but has all methods as appropriate for a plugin. And your Type_handler_test_int8 should inherit from it. But don't make all plugins to repeat the same method.
+ 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(...)
Historically we had two kinds of functions (have a look into 10.0):
- To create from FRM data:
Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length, uchar *null_pos, uchar null_bit, uint pack_flag, enum_field_types field_type, CHARSET_INFO *field_charset, Field::geometry_type geom_type, Field::utype unireg_check, TYPELIB *interval, const char *field_name)
- To create from Item: create_tmp_field_from_item() Field_new_decimal::create_from_item(item)
They use different input data formats for attributes.
These two interfaces have migrated to:
- make_table_field_from_def(), to create from FRM. - make_table_field(), to create from Item.
I plan to add a new method in Item so it can return attributes in the format suitable for make_table_field_from_def().
I planned to do it at some point later, in a separate patch.
Either do it now, before this patch, or use some other way to have only one make_field() in the type handler. What 10.0 used to do 7 years ago is not a reason to complicate the API by requiring plugin write to have two almost identical methods.
+ +static Type_handler_test_int8 type_handler_test_int8; + + +const Type_handler *Field_test_int8::type_handler() const +{ + return &type_handler_test_int8; +} ... 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?
UNINSTALL PLUGIN does not check that the data type handler is currently being used by parallel threads. So for now some limitation is assumed, as in this comment:
I think this should be fixed. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org