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 <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.
> 
> 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 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