- developers - lists.mariadb.org

Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
by Sergei Golubchik 12 Jan '22

12 Jan '22

Hi, Rucha! On Jan 12, Rucha Deodhar wrote: > On Tue, Jan 11, 2022 at 11:49 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > +--error ER_PARSE_ERROR > > > +INSERT INTO t1 VALUES XXX (1),(2); > > > > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > > > index 8ebefbb3d82..47ed3ded4a7 100644 > > > --- a/sql/sql_yacc.yy > > > +++ b/sql/sql_yacc.yy > > > @@ -8426,6 +8426,8 @@ query_specification_start: > > > { > > > SELECT_LEX *sel; > > > LEX *lex= Lex; > > > + if (lex->sql_command == SQLCOM_INSERT) > > > + thd->get_stmt_da()->inc_current_row_for_warning(); > > > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) > > > MYSQL_YYABORT; > > > > better not to have an if() in this rule that's used almost for > > everything. > > When we have INSERT...SELECT counter has to be 1 for errors like > ER_ILLEGAL_VALUE_FOR_TYPE which are caught during parsing. > Example: > INSERT INTO t1 SELECT a, b, 1.79769313486232e+308 FROM t2; > > So I added if() to increment the counter if we are parsing INSERT so > that ROW_NUMBER for statements like the one above gives ROW_NUMBER=1. I see. But you put a condition on every query's execution path to fix the case that, I believe, literally nobody cares about. I don't think any user ever will check the ROW_NUMBER after the syntax error :) And if someone will, we can say that in INSERT INTO t1 VALUES XXX (1),(2); the first row starts after VALUES, so ROW_NUMBER=1 is expected. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Andrei! On Jan 11, Andrei Elkin wrote: > Howdy, Sergei! > > To the question of the usage of trX cache by non-trX let me answer > broadly to mention @@binlog_direct_non_transactional_update = false > leads to aggregation of mixed, say innodb + myisam, events in trx > cache. That's different. The bug summary is "GTID event falsely marked transactional", meaning, the group of events is not transactional and it's falsely marked as transactional. If you have an innodb/myisam mix, it is correctly marked transactional. You need to have only non-transactional events in the transactional cache. And I'm trying to understand how can that happen. > I may need to process deeper the referred comments though. > > Next to the commit's idea, it actually covers your concern. I wonder > on the 'Wrong' conclusion. There is no intent to xidify anything > extra. Th commit assumes xid eVent is created, before Gtid one, > accordingly. And that fact is propagated to Gtid ctor. Your commit, if I'm not mistaken, assumes that "last event is Xid if and only if the cache contains a transaction". Which is incorrect, a transaction does not necesarily have to end with a Xid event. /Sergei > On Mon, 10 Jan 2022, 20:56 Sergei Golubchik, <serg(a)mariadb.org> wrote: > > > Hi, Andrei! > > > > On Jan 10, Andrei Elkin wrote: > > > revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75) > > > parent(s): 160d97a4aaa > > > author: Andrei Elkin > > > committer: Andrei Elkin > > > timestamp: 2021-08-08 14:20:57 +0300 > > > message: > > > > > > MDEV-24654 GTID event falsely marked transactional > > > > > > GTID event can be falsely marked transactional in few cases including > > > binary-logging on the slave side upon execution. > > > In some execution branches bin-logging of non-transactional > > > events can be done through transactional cache. E.g see comments > > > in THD::binlog_write_table_map(). > > > > I saw that and still couldn't understand why bin-logging of > > non-transactional events can be done thr-ough transactional cache. > > > > > In order to not create the false 'trans' tag the fact of Xid event > > > logging is checked to compute correct argument to Gtid_log_event > > > ctor. > > > > This is wrong. Transactions only end with a Xid event if all > > participating engines support XA. Try to create an InnoDB+FederatedX > > transaction (I just did) and it'll end with Query_log_event("COMMIT") > > > > > diff --git a/sql/log.cc b/sql/log.cc > > > index 1d9b4645421..7fef2e0d739 100644 > > > --- a/sql/log.cc > > > +++ b/sql/log.cc > > > @@ -8118,9 +8118,11 @@ > > MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry, > > > uint64 commit_id) > > > { > > > binlog_cache_mngr *mngr= entry->cache_mngr; > > > + bool has_xid= entry->end_event->get_type_code() == XID_EVENT; > > > DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt"); > > > > > > - if (write_gtid_event(entry->thd, false, entry->using_trx_cache, > > commit_id)) > > > + if (write_gtid_event(entry->thd, false, > > > + entry->using_trx_cache && has_xid, commit_id)) > > > DBUG_RETURN(ER_ERROR_ON_WRITE); > > > > > > if (entry->using_stmt_cache && !mngr->stmt_cache.empty() && > > > > > Regards, > > Sergei > > VP of MariaDB Server Engineering > > and security(a)mariadb.org > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Andrei! On Jan 11, Andrei Elkin wrote: > On Tue, Jan 11, 2022 at 1:06 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > Hi, Andrei! > > > > On Jan 11, Andrei Elkin wrote: > > > Howdy, Sergei! > > > > > > To the question of the usage of trX cache by non-trX let me answer > > > broadly to mention @@binlog_direct_non_transactional_update = false > > > leads to aggregation of mixed, say innodb + myisam, events in trx > > > cache. > > > > That's different. The bug summary is "GTID event falsely marked > > transactional", meaning, the group of events is not transactional and > > it's falsely marked as transactional. If you have an innodb/myisam > > mix, it is correctly marked transactional. > > > > You need to have only non-transactional events in the transactional > > cache. And I'm trying to understand how can that happen. > > Got your very specific point is about a pure myisam can be trx-cached. > This is OPTION_BINLOG_GTID > commit 5426facdcbfba2d78dab3c709cbf278073383b7c 's decision > >> - All tables are regarded as transactional tables in the binary log > (to ensure things are executed exactly as on the master) Okay, thanks, that answers my question Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! On Jan 11, Rucha Deodhar wrote: > revision-id: 7a746072a5e (mariadb-10.7.1-26-g7a746072a5e) > parent(s): 7dfaded9625 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-05 12:05:22 +0530 > message: > > MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero > > Analysis: m_current_row_for_warning counter is set to 0 at the beginning > of parsing. > Fix: Reset it to 1 and increment it before starting to parse insert values, > during SET and SELECT for INSERT...SET statement and INSERT...SELECT > respectively. > > diff --git a/mysql-test/main/get_diagnostics.result b/mysql-test/main/get_diagnostics.result > diff --git a/mysql-test/main/get_diagnostics.test b/mysql-test/main/get_diagnostics.test > index e8d81dca1e6..467f624c49c 100644 > --- a/mysql-test/main/get_diagnostics.test > +++ b/mysql-test/main/get_diagnostics.test > @@ -1687,3 +1687,18 @@ GET DIAGNOSTICS CONDITION 3 @n = ROW_NUMBER; > SELECT @n; > > DROP TABLE t; > + > +--echo # > +--echo # MDEV-26878: Query failing with syntax error sets ROW_NUMBER to > +--echo # non-zero > +--echo # > + > +CREATE TABLE t1 (id1 INT); > + > +--error ER_PARSE_ERROR > +INSERT INTO t1 VALUES XXX (1),(2); please add a test, like, INTO t1 VALUES (1), (ALL); what ROW_NUMBER will it show? > + > +GET DIAGNOSTICS CONDITION 1 @n= ROW_NUMBER; > +SELECT @n; > + > +DROP TABLE t1; > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 8ebefbb3d82..47ed3ded4a7 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -8426,6 +8426,8 @@ query_specification_start: > { > SELECT_LEX *sel; > LEX *lex= Lex; > + if (lex->sql_command == SQLCOM_INSERT) > + thd->get_stmt_da()->inc_current_row_for_warning(); > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) > MYSQL_YYABORT; better not to have an if() in this rule that's used almost for everything. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8e21b91fd31: MDEV-26875: Wrong user in SET DEFAULT ROLE error
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Anel! On Jan 11, Anel Husakovic wrote: > revision-id: 8e21b91fd31 (mariadb-10.5.12-71-g8e21b91fd31) > parent(s): e5a9dcfda20 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-10-25 14:43:38 +0200 > message: > > MDEV-26875: Wrong user in SET DEFAULT ROLE error > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 65f053aa09b..0a59f5fce37 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -3273,7 +3273,7 @@ static int check_user_can_set_role(THD *thd, const char *user, > { > /* Role is not granted but current user can see the role */ > my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > - MYF(0), thd->security_ctx->priv_user, > + MYF(0), user, > thd->security_ctx->priv_host, rolename); That's strange. you'll use `user` and `thd->security_ctx->priv_host` ? They generally correspond to two different accounts. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b14b6f31fa2: MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from function Analysis: ROW_NUMBER should be 0 because the error produced is not related to statement that modifies table data. It is just wrong number of arguments. However, ROW_NUMBER is 1 because m_current_row_for_warning is not set to 0 before executing function. Fix: Set m_current_row_for_warning to 0.
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! What if you have INSERT t1 VALUES (f1(1)), (f1(2)), (f1()); ? On Jan 11, Rucha Deodhar wrote: > revision-id: b14b6f31fa2 (mariadb-10.6.1-143-gb14b6f31fa2) > parent(s): d555ae38381 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-21 17:13:13 +0530 > message: > > MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from > function > Analysis: ROW_NUMBER should be 0 because the error produced is not related to > statement that modifies table data. It is just wrong number of arguments. > However, ROW_NUMBER is 1 because m_current_row_for_warning is not set to 0 > before executing function. > Fix: Set m_current_row_for_warning to 0. > > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index d1cf5c2d9bd..ebae2f356bd 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -2033,6 +2033,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, > */ > if (argcount != m_pcont->context_var_count()) > { > + thd->get_stmt_da()->reset_current_row_for_warning(0); > /* > Need to use my_error here, or it will not terminate the > invoking query properly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 394bb653b0b: MDEV-26695: Number of an invalid row is not calculated for table value constructor
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! please, see below On Jan 11, Rucha Deodhar wrote: > revision-id: 394bb653b0b (mariadb-10.6.1-140-g394bb653b0b) > parent(s): 3c857a07d92 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-20 01:30:04 +0530 > message: > > MDEV-26695: Number of an invalid row is not calculated for table value > constructor > > Analysis: counter does not increment while sending rows for table value > constructor and so row_number assumes the default value (0 in this case). > Fix: Increment the counter to avoid counter using default value. > > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > index 49f319b3856..a191ec9c328 100644 > --- a/sql/sql_tvc.cc > +++ b/sql/sql_tvc.cc > @@ -422,7 +422,9 @@ bool table_value_constr::exec(SELECT_LEX *sl) > DBUG_ENTER("table_value_constr::exec"); > List_iterator_fast<List_item> li(lists_of_values); > List_item *elem; > + THD *cur_thd= sl->parent_lex->thd; > ha_rows send_records= 0; > + int rc; > > if (select_options & SELECT_DESCRIBE) > DBUG_RETURN(false); > @@ -438,16 +440,21 @@ bool table_value_constr::exec(SELECT_LEX *sl) > > while ((elem= li++)) > { > + cur_thd->get_stmt_da()->inc_current_row_for_warning(); > if (send_records >= sl->master_unit()->lim.get_select_limit()) > - break; > + goto reset_counter_and_exit; Why goto, break worked here just fine, didn't it? Also, note that if you jump from here out of the loop, you'll do if (rc>0) where rc wasn't initialized yet. > - int rc= > - result->send_data_with_check(*elem, sl->master_unit(), send_records); > + rc= result->send_data_with_check(*elem, sl->master_unit(), send_records); > if (!rc) > send_records++; > else if (rc > 0) > - DBUG_RETURN(true); > + goto reset_counter_and_exit; break here too? > } > > +reset_counter_and_exit: > + cur_thd->get_stmt_da()->reset_current_row_for_warning(0); why do you need to reset here? > + if (rc>0) > + DBUG_RETURN(true); > + > if (result->send_eof()) > DBUG_RETURN(true); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ce69e0c8e78: MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Rucha! First, note that it's now a 10.9 task, so don't push it into 10.8, please. As for the patch - code-wise it's all good. New mode names are confusing and should be clarified, see the comment below. On Jan 10, Rucha Deodhar wrote: > revision-id: ce69e0c8e78 (mariadb-10.6.1-88-gce69e0c8e78) > parent(s): 22556499397 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-06 13:25:29 +0530 > message: > > MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable > > Analysis: There are 2 server variables- "old_mode" and "old". "old" is > no longer needed as "old_mode" has replaced it (however still used in > some places in the code). "old_mode" and "old" has same purpose- > emulate behavior from previous MariaDB versions. So they can be merged > to avoid confusion. Fix: Deprecate "old" variable and create another > mode for @@old_mode to mimic behavior of previous "old" variable. > Create specific modes for specifix task that --old sql variable was > doing earlier and use the new modes instead. > > diff --git a/mysql-test/suite/sys_vars/r/old_mode_basic.result b/mysql-test/suite/sys_vars/r/old_mode_basic.result > index a6b95f1c60c..2f3742160eb 100644 > --- a/mysql-test/suite/sys_vars/r/old_mode_basic.result > +++ b/mysql-test/suite/sys_vars/r/old_mode_basic.result > @@ -264,3 +264,42 @@ SET @@collation_database = @save_collation_database; > # > # End of 10.6 test > # > +# > +# Beginning of 10.7 test > +# 10.9 now > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index c2a219ee015..d1eaacd777c 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3755,6 +3774,8 @@ static const char *old_mode_names[]= > "NO_PROGRESS_INFO", > "ZERO_DATE_TIME_CAST", > "UTF8_IS_UTF8MB3", > + "INDEX_MASKING", > + "CHECKSUM_FORMATTING", that's confusing. I don't understsand from the name of the option what "CHECKSUM_FORMATTING" or "INDEX_MASKING" changes. "NO_PROGRESS_INFO" is clear, there won't be progress info "UTF8_IS_UTF8MB3" is clear "ZERO_DATE_TIME_CAST" is kind of understandable too, CAST(<time> AS DATETIME) will be "0000-00-00 <time>", that is zero date CHECKSUM_FORMATTING and INDEX_MASKING is unclear. For CHECKSUM_FORMATTING I'd suggest CHECKSUM_SLOW_NULLS (see the commit that introduced this behavior: 496741d5761f) "INDEX_MASKING" was introduced in the commit 79542930ea1c, see the comment there, it explains what was done. This should help you to suggest a better name for this mode. > 0 > }; > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Andrei! On Jan 10, Andrei Elkin wrote: > revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75) > parent(s): 160d97a4aaa > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2021-08-08 14:20:57 +0300 > message: > > MDEV-24654 GTID event falsely marked transactional > > GTID event can be falsely marked transactional in few cases including > binary-logging on the slave side upon execution. > In some execution branches bin-logging of non-transactional > events can be done through transactional cache. E.g see comments > in THD::binlog_write_table_map(). I saw that and still couldn't understand why bin-logging of non-transactional events can be done through transactional cache. > In order to not create the false 'trans' tag the fact of Xid event > logging is checked to compute correct argument to Gtid_log_event > ctor. This is wrong. Transactions only end with a Xid event if all participating engines support XA. Try to create an InnoDB+FederatedX transaction (I just did) and it'll end with Query_log_event("COMMIT") > diff --git a/sql/log.cc b/sql/log.cc > index 1d9b4645421..7fef2e0d739 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8118,9 +8118,11 @@ MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry, > uint64 commit_id) > { > binlog_cache_mngr *mngr= entry->cache_mngr; > + bool has_xid= entry->end_event->get_type_code() == XID_EVENT; > DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt"); > > - if (write_gtid_event(entry->thd, false, entry->using_trx_cache, commit_id)) > + if (write_gtid_event(entry->thd, false, > + entry->using_trx_cache && has_xid, commit_id)) > DBUG_RETURN(ER_ERROR_ON_WRITE); > > if (entry->using_stmt_cache && !mngr->stmt_cache.empty() && > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 734aee3cd3c: MDEV-27018 IF and COALESCE lose "json" property
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Alexander! On Dec 28, Alexander Barkov wrote: > revision-id: 734aee3cd3c (mariadb-10.5.13-33-g734aee3cd3c) > parent(s): 2776635cb98 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2021-12-27 17:54:31 +0400 > message: > > MDEV-27018 IF and COALESCE lose "json" property > > Hybrid functions (IF, COALESCE, etc) did not preserve the JSON property > from their arguments. The same problem was repeatable for single row subselects. > > The problem happened because the method Item::is_json_type() was inconsistently > implemented across the Item hierarchy. For example, Item_hybrid_func > and Item_singlerow_subselect did not override is_json_type(). Okay. The approach is fine, but see some questions below: > diff --git a/sql/field.cc b/sql/field.cc > index 2c768527ced..8fa3bbd538c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7277,6 +7277,19 @@ bool Field_longstr::send(Protocol *protocol) > } > > > +const Type_handler *Field_string::type_handler() const > +{ > + if (is_var_string()) > + return &type_handler_var_string; > + /* > + This is a temporary solution and will be fixed soon (in 10.9?). > + Type_handler_string_json will provide its own Field_string_json. why? > + */ > + if (Type_handler_json_common::has_json_valid_constraint(this)) could you please use names that don't depend on how exactly we detect json? especially if the goal is to move to FORMAT JSON syntax > + return &type_handler_string_json; > + return &type_handler_string; > +} > + > /* Copy a string and fill with space */ > > int Field_string::store(const char *from, size_t length,CHARSET_INFO *cs) > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index c1801c1ae3e..3b2753d80a6 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const > > /***************************************************************************/ > > +class Recursive_type_pair_iterator > +{ > + const Type_handler *m_a; > + const Type_handler *m_b; > + uint m_switched_to_base_count; > +public: > + Recursive_type_pair_iterator(const Type_handler *a, > + const Type_handler *b, > + uint switched_to_base_count= 0) > + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count) > + { } > + const Type_handler *a() const { return m_a; } > + const Type_handler *b() const { return m_b; } > + Recursive_type_pair_iterator base() const > + { > + Recursive_type_pair_iterator res(m_a->type_handler_base(), > + m_b->type_handler_base()); > + res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL); > + if (res.m_a == NULL) > + res.m_a= m_a; > + if (res.m_b == NULL) > + res.m_b= m_b; > + return res; > + } > + bool done() const > + { > + switch (m_switched_to_base_count) > + { > + case 2: > + /* > + Expression: > + COALESCE(TYPE1, TYPE2) > + > + where both type handlers derive from some other handlers, e.g. > + VARCHAR -> TYPE1 > + TEXT -> TYPE2 > + > + Continue aggregation as: > + SELECT COALESCE(VARCHAR, TEXT) > + > + Note, we now have only one level of data type inheritance, > + e.g. VARCHAR -> VARCHAR/JSON > + > + This code branch will change when we have longer inheritance chains: > + A0 -> A1 -> A2 -> A3 > + B0 -> B1 -> B2 > + > + Functions like COALESE(A2, B2) will need to do full overload resolution: > + - iterate through all pairs from the below matrix > + - find all existing candidates > + - resolve ambiguities in case multiple candidates, etc. and I don't see how you're going to do that. you need to try m_a+m_b.base(), m_a.base()+m_b, and m_a.base()+m_b.base(). And that's not what you're doing. I suggest to add an assert that m_switched_to_base_count < 2 and stop trying to solve complex cases until we have at least one. > + > + A0 A1 A2 A3 > + B0 . . . . > + B1 . . . . > + B2 . . . . > + */ > + return false; > + > + case 1: > + /* > + Expression: > + COALESCE(TYPE1, TEXT) > + > + If only one handler derives from some other handler: > + VARCHAR-> TYPE1 > + > + Continue aggregation as: > + SELECT COALESCE(VARCHAR, TEXT) > + */ > + return false; > + > + case 0: > + default: > + /* > + Non of the two handlers are derived from other handlers. > + Nothing left to try. > + */ > + return true; > + } > + } > +}; > diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h > index 6c4ee8cb2eb..4a394809a06 100644 > --- a/sql/sql_type_json.h > +++ b/sql/sql_type_json.h > @@ -21,18 +21,145 @@ ... > + bool Item_append_extended_type_info(Send_field_extended_metadata *to, > + const Item *item) const > + { > + return set_format_name(to); // Send "format=json" in the protocol > + } > + > + bool Item_hybrid_func_fix_attributes(THD *thd, > + const char *name, > + Type_handler_hybrid_field_type *hybrid, > + Type_all_attributes *attr, > + Item **items, uint nitems) > + const override > + { > + if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr, > + items, nitems)) > + return true; > + /* > + The above call can change the type handler on "hybrid", e.g. > + choose a proper BLOB type handler according to the calculated max_length. > + Convert general purpose string type handler to its JSON counterpart. > + This makes hybrid functions preserve JSON data types, e.g.: > + COALESCE(json_expr1, json_expr2) -> JSON > + */ > + hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler())); I don't get it. It's supposed to "fix attributes", like signed/unsigned, max length, etc. The correct type handler should've been set already, otherwise this type handler method wouldn't even be called. > + return false; > + } > }; > > + > +class Type_handler_string_json: > + public Type_handler_general_purpose_string_to_json<Type_handler_string, > + type_handler_string> > +{ }; > diff --git a/sql/sql_type_json.cc b/sql/sql_type_json.cc > index a804366ec03..a84f720bcc9 100644 > --- a/sql/sql_type_json.cc > +++ b/sql/sql_type_json.cc > @@ -20,17 +20,111 @@ > #include "sql_class.h" > > > -Type_handler_json_longtext type_handler_json_longtext; > +Named_type_handler<Type_handler_string_json> > + type_handler_string_json("char/json"); do you mean these names are not shown anywhere? > +Named_type_handler<Type_handler_varchar_json> > + type_handler_varchar_json("varchar/json"); > + > +Named_type_handler<Type_handler_tiny_blob_json> > + type_handler_tiny_blob_json("tinyblob/json"); > + > +Named_type_handler<Type_handler_blob_json> > + type_handler_blob_json("blob/json"); > + > +Named_type_handler<Type_handler_medium_blob_json> > + type_handler_medium_blob_json("mediumblob/json"); > + > +Named_type_handler<Type_handler_long_blob_json> > + type_handler_long_blob_json("longblob/json"); ... > +/* > + This method ressembles what Field_blob::type_handler() resembles (also below) > + does for general purpose BLOB type handlers. > +*/ > +const Type_handler * > +Type_handler_json_common::json_blob_type_handler_by_length_bytes(uint len) > +{ > + switch (len) { > + case 1: return &type_handler_tiny_blob_json; > + case 2: return &type_handler_blob_json; > + case 3: return &type_handler_medium_blob_json; > + } > + return &type_handler_long_blob_json; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3