Hi, Alexander! On Feb 26, Alexander Barkov wrote:

Hi, Sergei, Georg,

Please review a fixed version of the patch for MDEV-17832.

There are two files attached: - mdev-17821.v18.diff (server changes)

Here you are:

diff --git a/include/mysql.h b/include/mysql.h index ec49ca0482a..7ba7503ca4e 100644 --- a/include/mysql.h +++ b/include/mysql.h @@ -411,6 +413,19 @@ MYSQL_FIELD * STDCALL mysql_fetch_fields(MYSQL_RES *res); MYSQL_ROW_OFFSET STDCALL mysql_row_tell(MYSQL_RES *res); MYSQL_FIELD_OFFSET STDCALL mysql_field_tell(MYSQL_RES *res);

+ +typedef struct st_mariadb_metadata_string +{ + const char *str; + size_t length; +} MARIADB_FIELD_METADATA_STRING;

not LEX_CSTRING? it's inside the server. and plugins can use MYSQL_CONST_LEX_STRING, if needed. clients don't see this header, so why a new type?

+ + +MARIADB_FIELD_METADATA_STRING + mariadb_field_metadata_attr(const MYSQL_FIELD *field, + enum mariadb_field_metadata_attr_t type); + + unsigned int STDCALL mysql_field_count(MYSQL *mysql); my_ulonglong STDCALL mysql_affected_rows(MYSQL *mysql); my_ulonglong STDCALL mysql_insert_id(MYSQL *mysql); diff --git a/client/client_metadata.h b/client/client_metadata.h new file mode 100644 index 00000000000..ee0185e5049 --- /dev/null +++ b/client/client_metadata.h @@ -0,0 +1,58 @@ +#ifndef SQL_METADATA_INCLUDED +#define SQL_METADATA_INCLUDED +/* + Copyright (c) 2020, MariaDB Corporation. + + 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-1335 USA */ + +/* This file is originally from the mysql distribution. Coded by monty */

what do you mean by that? I didn't find it anywhere copy-paste from sql_string.h ?

+ +#ifdef USE_PRAGMA_INTERFACE +#pragma interface /* gcc class implementation */ +#endif

same?

+ +#include "sql_string.h" + + +class Client_field_metadata +{ + MARIADB_FIELD_METADATA_STRING m_type; + MARIADB_FIELD_METADATA_STRING m_format; +public: + Client_field_metadata(MYSQL_FIELD *field) + :m_type(mariadb_field_metadata_attr(field, + MARIADB_FIELD_METADATA_DATA_TYPE_NAME)), + m_format(mariadb_field_metadata_attr(field, + MARIADB_FIELD_METADATA_FORMAT_NAME)) + { } + void append_to(Binary_string *to) const + { + uint orig_to_length= to->length(); + if (m_type.length) + { + to->append(C_STRING_WITH_LEN("type=")); + to->append(m_type.str, m_type.length); + } + if (m_format.length) + { + if (to->length() != orig_to_length) + to->append(" ", 1); + to->append(C_STRING_WITH_LEN("format=")); + to->append(m_format.str, m_format.length); + } + } +}; + + +#endif // SQL_METADATA_INCLUDED diff --git a/libmysqld/libmysql.c b/libmysqld/libmysql.c index 2be94882303..8e3647f3c24 100644 --- a/libmysqld/libmysql.c +++ b/libmysqld/libmysql.c @@ -732,6 +732,35 @@ mysql_fetch_field(MYSQL_RES *result) }

+/************************************************************************** +** Return mysql field metadata +**************************************************************************/ +MARIADB_FIELD_METADATA_STRING +mariadb_field_metadata_attr(const MYSQL_FIELD *field, + enum mariadb_field_metadata_attr_t type) +{ + static const MARIADB_FIELD_METADATA_STRING null_str= {NULL, 0}; + const char *ptr= field->type_info; + const char *end= field->type_info + field->type_info_length; + for ( ; ptr < end ; ) + { + MARIADB_FIELD_METADATA_STRING res; + size_t itype; + if ((uchar) *ptr > 127) + return null_str; + itype= (size_t) (*ptr++); + res.length= (size_t) (*ptr++); + res.str= ptr; + if (res.str + res.length > end) + return null_str; + if (itype == (size_t) type) + return res; + ptr+= res.length; + } + return null_str;

this means when you need both the type and the format, like in the Client_field_metadata constructor, you call mariadb_field_metadata_attr_t twice, for N metadata fields you need to scan the string N times. why not to do it in one scan, filling in an array MARIADB_FIELD_METADATA_STRING metadata[MARIADB_FIELD_METADATA_MAX];

+} + + /************************************************************************** Move to a specific row and column **************************************************************************/ diff --git a/mysql-test/main/gis.result b/mysql-test/main/gis.result index 936924ffe87..ebdb4ddfd24 100644 --- a/mysql-test/main/gis.result +++ b/mysql-test/main/gis.result @@ -5095,5 +5095,51 @@ ERROR HY000: Operator does not exists: 'CAST(expr AS multilinestring)' SELECT CONVERT(1, MULTIPOLYGON); ERROR HY000: Operator does not exists: 'CAST(expr AS multipolygon)' # +# MDEV-17832 Protocol: extensions for Pluggable types and JSON, GEOMETRY +# +SET NAMES utf8; +CREATE TABLE t1 ( +p POINT, +ls LINESTRING, +pl POLYGON, +mp MULTIPOINT, +mls MULTILINESTRING, +mpl MULTIPOLYGON, +gc GEOMETRYCOLLECTION, +g GEOMETRY +) CHARACTER SET utf8; +SELECT * FROM t1; +Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr +def test t1 t1 p p 255 (type=point) 4294967295 0 Y 144 0 63 +def test t1 t1 ls ls 255 (type=linestring) 4294967295 0 Y 144 0 63

A bit strange to have length=4294967295 for point and linestring, don't you think?

+def test t1 t1 pl pl 255 (type=polygon) 4294967295 0 Y 144 0 63 +def test t1 t1 mp mp 255 (type=multipoint) 4294967295 0 Y 144 0 63 +def test t1 t1 mls mls 255 (type=multilinestring) 4294967295 0 Y 144 0 63 +def test t1 t1 mpl mpl 255 (type=multipolygon) 4294967295 0 Y 144 0 63 +def test t1 t1 gc gc 255 (type=geometrycollection) 4294967295 0 Y 144 0 63 +def test t1 t1 g g 255 4294967295 0 Y 144 0 63 +p ls pl mp mls mpl gc g +SELECT +COALESCE(p) AS p, +COALESCE(ls) AS ls, +COALESCE(pl) AS pl, +COALESCE(mp) AS mp, +COALESCE(mls) AS mls, +COALESCE(mpl) AS mpl, +COALESCE(gc) AS gc, +COALESCE(g) AS g +FROM t1; +Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr +def p 255 (type=point) 4294967295 0 Y 128 0 63 +def ls 255 (type=linestring) 4294967295 0 Y 128 0 63 +def pl 255 (type=polygon) 4294967295 0 Y 128 0 63 +def mp 255 (type=multipoint) 4294967295 0 Y 128 0 63 +def mls 255 (type=multilinestring) 4294967295 0 Y 128 0 63 +def mpl 255 (type=multipolygon) 4294967295 0 Y 128 0 63 +def gc 255 (type=geometrycollection) 4294967295 0 Y 128 0 63 +def g 255 4294967295 0 Y 128 0 63 +p ls pl mp mls mpl gc g +DROP TABLE t1;

please test type aggregation too, like COALESCE(p,ls), COALESCE(pl, mpl), etc

+# # End of 10.5 tests # diff --git a/mysql-test/main/gis.test b/mysql-test/main/gis.test index 48f2803b27d..22bd1553bc2 100644 --- a/mysql-test/main/gis.test +++ b/mysql-test/main/gis.test @@ -3180,6 +3180,38 @@ SELECT CONVERT(1, MULTILINESTRING); SELECT CONVERT(1, MULTIPOLYGON);

+--echo # +--echo # MDEV-17832 Protocol: extensions for Pluggable types and JSON, GEOMETRY +--echo # + +SET NAMES utf8; +CREATE TABLE t1 ( + p POINT, + ls LINESTRING, + pl POLYGON, + mp MULTIPOINT, + mls MULTILINESTRING, + mpl MULTIPOLYGON, + gc GEOMETRYCOLLECTION, + g GEOMETRY +) CHARACTER SET utf8; +--disable_ps_protocol

why?

+--enable_metadata +SELECT * FROM t1; +SELECT + COALESCE(p) AS p, + COALESCE(ls) AS ls, + COALESCE(pl) AS pl, + COALESCE(mp) AS mp, + COALESCE(mls) AS mls, + COALESCE(mpl) AS mpl, + COALESCE(gc) AS gc, + COALESCE(g) AS g +FROM t1; +--disable_metadata +--enable_ps_protocol +DROP TABLE t1; + --echo # --echo # End of 10.5 tests --echo # diff --git a/mysql-test/main/mysql-metadata.test b/mysql-test/main/mysql-metadata.test new file mode 100644 index 00000000000..bab44496f78 --- /dev/null +++ b/mysql-test/main/mysql-metadata.test @@ -0,0 +1,22 @@ +-- source include/have_working_dns.inc +-- source include/not_embedded.inc + +--echo # +--echo # MDEV-17832 Protocol: extensions for Pluggable types and JSON, GEOMETRY +--echo # + +SET NAMES utf8; +CREATE TABLE t1 ( + js0 JSON, + js1 TEXT CHECK (JSON_VALID(js1)), + js2 TEXT CHECK (LENGTH(js2) > 0 AND JSON_VALID(js2)), + js3 TEXT CHECK (LENGTH(js2) > 0 OR JSON_VALID(js2)) +) CHARACTER SET utf8; + +--replace_regex /0 rows in set [(].*[)]/0 rows in set (TIME)/

Why [(] ? because backslash escapes don't work in replace_regex?

+--exec $MYSQL -vvv --column-type-info --database=test -e "SELECT * FROM t1;" + +--replace_regex /0 rows in set [(].*[)]/0 rows in set (TIME)/ +--exec $MYSQL -vvv --column-type-info --database=test -e "SELECT JSON_COMPACT(js0) FROM t1;" + +DROP TABLE t1; diff --git a/plugin/type_inet/sql_type_inet.cc b/plugin/type_inet/sql_type_inet.cc index 6803bdba434..79f85cd1f19 100644 --- a/plugin/type_inet/sql_type_inet.cc +++ b/plugin/type_inet/sql_type_inet.cc @@ -705,6 +705,11 @@ class Field_inet6: public Field str.set_ascii(name.ptr(), name.length()); }

+ bool extended_type_info_append_to(Extended_type_info *to) const override + { + return type_handler_inet6.extended_type_info_append_to(to); + } +

I don't like that it needs to be done _per plugin_ there should be a default implementation that just appends the type name. A plugin will only need to overwrite it if it wants to do something more complex. Actually, I'd even not let the plugin to overwrite it. Just append the type name for _every_ pluggable type. Unconditionally. We can revise it if there will be a convincing use case, no need to overengineer it now.

bool validate_value_in_record(THD *thd, const uchar *record) const override { return false; diff --git a/sql/field.cc b/sql/field.cc index 1ce49b0bdfa..8bab37a567c 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2039,6 +2039,19 @@ void Field::make_send_field(Send_field *field) field->set_handler(type_handler()); field->flags=table->maybe_null ? (flags & ~NOT_NULL_FLAG) : flags; field->decimals= 0; + extended_type_info_append_to(&field->extended_type_info); + if (check_constraint) + { + /* + Append the format that is implicitly implied by the CHECK CONSTRAINT. + For example: + CREATE TABLE t1 (js longtext DEFAULT NULL CHECK (json_valid(a))); + SELECT j FROM t1; + will add "cf=json" to the extended type info metadata for t1.js.

format=json ?

+ */ + check_constraint->expr-> + format_by_check_constraint_append_to(&field->extended_type_info); + }

in the base Field class? Not in, say, Field_longstr?

}

diff --git a/sql/item.h b/sql/item.h index 6a9d401b101..d0ff49275b4 100644 --- a/sql/item.h +++ b/sql/item.h @@ -1814,6 +1814,14 @@ class Item: public Value_source, /* This is to handle printing of default values */ virtual bool need_parentheses_in_default() { return false; } virtual void save_in_result_field(bool no_conversions) {} + /* + Data type format implied by the CHECK CONSTRAINT, + to be sent to the client in the result set metadata. + */ + virtual bool format_by_check_constraint_append_to(Extended_type_info *) const

This is rather strange order of words, it doesn't read well. May be better append_format_by_check_constraint() ?

+ { + return false; + } /* set value of aggregate function in case of no rows for grouping were found */ diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 6df2b5dbd3a..949e389698a 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -6481,6 +6481,20 @@ Item *Item_cond_and::neg_transformer(THD *thd) /* NOT(a AND b AND ...) -> */ }

+bool +Item_cond_and::format_by_check_constraint_append_to(Extended_type_info *to) const +{ + List_iterator_fast<Item> li(const_cast<List<Item>&>(list));

Why the cast?

+ Item *item; + while ((item= li++)) + { + if (item->format_by_check_constraint_append_to(to)) + return true; + } + return false; +} + + Item *Item_cond_or::neg_transformer(THD *thd) /* NOT(a OR b OR ...) -> */ /* NOT a AND NOT b AND ... */ { diff --git a/sql/sql_type.h b/sql/sql_type.h index ce87c8e9d93..899f189adeb 100644 --- a/sql/sql_type.h +++ b/sql/sql_type.h @@ -137,6 +138,40 @@ enum column_definition_type_t };

+class Extended_type_info: public Binary_string +{ +public: + void init() { length(0); } + bool append_chunk(mariadb_field_metadata_attr_t type, + const LEX_CSTRING &value) + { + /* + If we eventually support many metadata chunk types and long metadata + values, we'll need to encode type and length using net_store_length() + and do corresponding changes to the unpacking code in libmariadb. + For now let's just assert that type and length fit into one byte. + */ + DBUG_ASSERT(net_length_size((ulonglong) type) == 1); + DBUG_ASSERT(net_length_size((ulonglong) value.length) == 1);

why do you need these casts?

+ size_t nbytes= 1/*type*/ + 1/*length*/ + value.length; + if (reserve(nbytes)) + return true; + qs_append((char) (uchar) type); + qs_append((char) (uchar) value.length); + qs_append(&value); + return false; + } + bool append_format(const LEX_CSTRING &str) + { + return append_chunk(MARIADB_FIELD_METADATA_FORMAT_NAME, str); + } + bool append_type(const LEX_CSTRING &str) + { + return append_chunk(MARIADB_FIELD_METADATA_DATA_TYPE_NAME, str); + } +}; + + class Data_type_statistics { public: @@ -3426,6 +3461,11 @@ class Type_handler return field_type(); } virtual protocol_send_type_t protocol_send_type() const= 0; + virtual bool Item_extended_type_info_append_to(Extended_type_info *to,

Item_append_extended_type_info ?

+ const Item *item) const + { + return false; + } virtual Item_result result_type() const= 0; virtual Item_result cmp_type() const= 0; virtual enum_dynamic_column_type

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org