Re: [Maria-developers] 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
> >> committer: Alexander Barkov
> >> 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
> 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
20.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 000..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 000..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 PLU
Re: [Maria-developers] 3f38da9145c: MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
Hi Sergei, On 9/17/19 10:59 AM, Alexander Barkov wrote: Hi Sergei, Thank you for your review! I've fixed some of your suggestions. For other suggestions I have questions and comments. See below. A new patch is here: https://github.com/MariaDB/server/commit/62d9ca85706bc0c78ca174703ca46e2e82002491 Sorry, wrong URL. The patch is actually here: https://github.com/MariaDB/server/commit/17eeab8b470c237ce93d7ba9b5ecc6c4317d46e5 Alternatively, just go to the branch: https://github.com/MariaDB/server/compare/bb-10.5-bar-m20016 and choose the top commit. ___ 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
Re: [Maria-developers] 3f38da9145c: MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
Hi Sergei,
Thank you for your review!
I've fixed some of your suggestions.
For other suggestions I have questions and comments. See below.
A new patch is here:
https://github.com/MariaDB/server/commit/62d9ca85706bc0c78ca174703ca46e2e82002491
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
committer: Alexander Barkov
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.
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
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?
Can these changes in sql_type.h and plugin_data_type.h be done under a
separate MDEV later?
+
/*
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 000..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.
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-debug.test
b/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test
new file mode 100644
index 000..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.
Fixed.
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 000..758f94904c1
--- /dev/null
+++ b/plugin/type_test
Re: [Maria-developers] 3f38da9145c: MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
Hi, Alexander!
On Sep 16, Alexander Barkov wrote:
> revision-id: 3f38da9145c (mariadb-10.4.4-251-g3f38da9145c)
> parent(s): e6ff3f9d1c8
> author: Alexander Barkov
> committer: Alexander Barkov
> 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 000..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 000..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 000..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_STATUSACTIVE
> +PLUGIN_TYPE DATA TYPE
> +PLUGIN_AUTHORMariaDB
> +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;
> +TableCreate 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_IN
