maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #11963
Re: 3f38da9145c: MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
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@xxxxxxxxxxx>
> >> committer: Alexander Barkov <bar@xxxxxxxxxxx>
> >> 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@xxxxxxxxxxx
References
