[Maria-developers] MDEV-11485 Split Item_func_between::val_int() into virtual methods in Type_handler
Hello Sanja,
Please review a patch for MDEV-11485.
Thanks!
commit ee91200893adaec59ad7543711a230f6417815f0
Author: Alexander Barkov
Date: Mon Dec 5 20:15:36 2016 +0400
MDEV-11485 Split Item_func_between::val_int() into virtual methods in Type_handler
- Removes "Item_result Item_func_opt_neg::m_compare_type" and introduces
"Type_handler_hybrid_field_type Item_func_opt_neg::m_comparator" instead.
- Removes Item_func_between::compare_as_dates, because
the new member m_comparator now contains the precise information
about the data type that is used for comparison, which is important
for TIME vs DATETIME.
- Adds a new method:
Type_handler_hybrid_field_type::aggregate_for_comparison(const Type_handler*),
as a better replacement for item_cmp_type(), which additionally can handle
TIME vs DATE/DATETIME/TIMESTAMP correctly. Additionally, it correctly
handles TIMESTAMP which fixes the problem reported in MDEV-11482.
The old compare_as_dates/find_date_time_item() based code didn't handle
comparison between TIME and TIMESTAMP correctly and erroneously used TIME
comparison instead of DATETIME comparison.
- Adds a new method:
Type_handler_hybrid_field_type::aggregate_for_comparison(Item **, uint nitems),
as a better replacement for agg_cmp_type(), which can handle TIME.
- Splits Item_func_between::val_int() into pieces val_int_cmp_xxx(),
one new method per XXX_RESULT.
- Adds a new virtual method Type_handler::Item_func_between_val_int()
whose implementations use Item_func_between::val_int_cmp_xxx().
- Makes type_handler_longlong and type_handler_newdecimal public,
as they are now needed in item_cmpfunc.cc.
Note:
This patch does not change Item_func_in to use the new aggregation methods,
so it still uses collect_cmp_type()/item_cmp_type() based aggregation.
Item_func_in will be changed in a separate patch and item_cmp_type() will be
removed.
diff --git a/mysql-test/r/type_timestamp.result b/mysql-test/r/type_timestamp.result
index d4afed8..577ae5a 100644
--- a/mysql-test/r/type_timestamp.result
+++ b/mysql-test/r/type_timestamp.result
@@ -980,5 +980,17 @@ Warnings:
Warning 1441 Datetime function: datetime field overflow
DROP TABLE t1;
#
+# MDEV-11482 Incorrect result for (time_expr BETWEEN timestamp_exp1 AND timestamp_expr2)
+#
+SET @@sql_mode=DEFAULT;
+SET @@timestamp=UNIX_TIMESTAMP('2001-01-01 10:20:30');
+CREATE TABLE t1 (a TIMESTAMP,b TIMESTAMP);
+INSERT INTO t1 VALUES ('2001-01-01 00:00:00','2001-01-01 23:59:59');
+SELECT * FROM t1 WHERE TIME'10:20:30' BETWEEN a and b;
+a b
+2001-01-01 00:00:00 2001-01-01 23:59:59
+DROP TABLE t1;
+SET @@timestamp=DEFAULT;
+#
# End of 10.3 tests
#
diff --git a/mysql-test/t/type_timestamp.test b/mysql-test/t/type_timestamp.test
index 2dad92b..7f32f7a 100644
--- a/mysql-test/t/type_timestamp.test
+++ b/mysql-test/t/type_timestamp.test
@@ -577,5 +577,16 @@ EXPLAIN SELECT * FROM t1 WHERE a >= COALESCE(DATE_ADD(TIMESTAMP'-01-01 00:00
DROP TABLE t1;
--echo #
+--echo # MDEV-11482 Incorrect result for (time_expr BETWEEN timestamp_exp1 AND timestamp_expr2)
+--echo #
+SET @@sql_mode=DEFAULT;
+SET @@timestamp=UNIX_TIMESTAMP('2001-01-01 10:20:30');
+CREATE TABLE t1 (a TIMESTAMP,b TIMESTAMP);
+INSERT INTO t1 VALUES ('2001-01-01 00:00:00','2001-01-01 23:59:59');
+SELECT * FROM t1 WHERE TIME'10:20:30' BETWEEN a and b;
+DROP TABLE t1;
+SET @@timestamp=DEFAULT;
+
+--echo #
--echo # End of 10.3 tests
--echo #
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 98b179b..65754d2 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -118,14 +118,15 @@ static int cmp_row_type(Item* item1, Item* item2)
0 otherwise
*/
-static int agg_cmp_type(Item_result *type, Item **items, uint nitems)
+bool Type_handler_hybrid_field_type::aggregate_for_comparison(Item **items,
+ uint nitems)
{
uint unsigned_count= items[0]->unsigned_flag;
- type[0]= items[0]->cmp_type();
+ set_handler(items[0]->type_handler());
for (uint i= 1 ; i < nitems ; i++)
{
unsigned_count+= items[i]->unsigned_flag;
-type[0]= item_cmp_type(type[0], items[i]);
+aggregate_for_comparison(items[i]->type_handler());
/*
When aggregating types of two row expressions we have to check
that they have the same cardinality and that each component
@@ -133,15 +134,16 @@ static int agg_cmp_type(Item_result *type, Item **items, uint nitems)
the signature of the corresponding component of the second row
expression.
*/
-if (type[0] == ROW_RESULT && cmp_row_type(items[0], items[i]))
- return 1; // error found: invalid usage of rows
+if (cmp_type() == ROW_RESULT && cmp_row_type(items[0], items[i]))
+ return true; // error found: invalid usage of rows
}
/**
If all arguments are of INT type but
Re: [Maria-developers] Please review MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
Hi, Alexander! On Dec 05, Alexander Barkov wrote: > Hello Sergei, > > Please review a patch for MDEV-10717. ok to push Regards, Sergei Chief Architect MariaDB 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
Re: [Maria-developers] Fix for TokuDB and parallel replication
Hi Kristian! Thanks for the fix, I should be able to give it a try sometimes this week. Jocelyn Le 28/11/2016 à 10:10, Kristian Nielsen a écrit : Parallel replication so far did not work well with TokuDB, as some people who tried it found out. I have now pushed to 10.1 some patches to solve the problems. There are two main fixes: 1. Fix some races where a waiting transaction would miss its wakeup and get a lock timeout on a waiting row lock, even though the lock was released by the holding transaction. This fix is due to great work by Rich Prohaska. This problem is actually not specific to replication, normal transactions on a master will experience it too. But it hurts replication a lot, since replication must commit transactions in order, so one stalled transaction stalls all following transactions as well. 2. Implement the conflict detection and handling necessary for optimistic parallel replication to work. This basically implements thd_rpl_deadlock_check() and hton->kill_query methods. This should solve the problems where optimistic parallel replication with TokuDB breaks with lock wait timeouts. If someone wants to test it, I have made a tree available with just these fixes on top of MariaDB 10.1.19: https://github.com/knielsen/server/tree/tokudb_optimistic_parallel_replication The fix should appear in 10.1.20 eventually. The first part of the patch has also been submitted by Rich to upstream. When this is (hopefully) merged upstream, and upstream merged into MariaDB, the MariaDB version of the fix should be replaced with the Percona one. I tried making the MariaDB version of the fix identical to Rich's pull request and keeping it in a separate commit, so this should hopefully be simple to do when the time comes. - Kristian. ___ 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 ___ 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
[Maria-developers] Please review MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
Hello Sergei,
Please review a patch for MDEV-10717.
Thanks!
commit 5eefa9acae7456e60cc5f8992906d437bc0afdc8
Author: Alexander Barkov
Date: Mon Dec 5 16:50:12 2016 +0400
MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
The problem was that null_value was not set to "false" on a well-formed row.
If an ill-formed row was followed by a well-forned row, null_value remained
"true" in the call of Item::send() for the well-formed row.
diff --git a/mysql-test/r/ctype_utf8.result b/mysql-test/r/ctype_utf8.result
index 121168c..294c2cb 100644
--- a/mysql-test/r/ctype_utf8.result
+++ b/mysql-test/r/ctype_utf8.result
@@ -5869,5 +5869,31 @@ SELECT length(data) AS len FROM (SELECT REPEAT('ä', 65537) AS data) AS sub;
len
131074
#
+# MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
+#
+CREATE TABLE t1 (i INT, KEY(i));
+INSERT INTO t1 VALUES (20081205),(20050327);
+SELECT HEX(i), HEX(CHAR(i USING utf8)) FROM t1;
+HEX(i) HEX(CHAR(i USING utf8))
+131F197 0131
+1326A35 01326A35
+Warnings:
+Warning 1300 Invalid utf8 character string: 'F197'
+SET sql_mode='STRICT_ALL_TABLES';
+SELECT HEX(i), HEX(CHAR(i USING utf8)) FROM t1;
+HEX(i) HEX(CHAR(i USING utf8))
+131F197 NULL
+1326A35 01326A35
+Warnings:
+Warning 1300 Invalid utf8 character string: 'F197'
+SELECT CHAR(i USING utf8) FROM t1;
+CHAR(i USING utf8)
+###
+###
+Warnings:
+### 1300 Invalid utf8 character string: 'F197'
+SET sql_mode=DEFAULT;
+DROP TABLE t1;
+#
# End of 5.5 tests
#
diff --git a/mysql-test/t/ctype_utf8.test b/mysql-test/t/ctype_utf8.test
index d6fdc6c..75581ed 100644
--- a/mysql-test/t/ctype_utf8.test
+++ b/mysql-test/t/ctype_utf8.test
@@ -1682,5 +1682,19 @@ SELECT length(data) AS len FROM (SELECT REPEAT('ä', 65536) AS data) AS sub;
SELECT length(data) AS len FROM (SELECT REPEAT('ä', 65537) AS data) AS sub;
--echo #
+--echo # MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
+--echo #
+CREATE TABLE t1 (i INT, KEY(i));
+INSERT INTO t1 VALUES (20081205),(20050327);
+SELECT HEX(i), HEX(CHAR(i USING utf8)) FROM t1;
+SET sql_mode='STRICT_ALL_TABLES';
+SELECT HEX(i), HEX(CHAR(i USING utf8)) FROM t1;
+# Avoid garbage in the output
+--replace_column 1 ###
+SELECT CHAR(i USING utf8) FROM t1;
+SET sql_mode=DEFAULT;
+DROP TABLE t1;
+
+--echo #
--echo # End of 5.5 tests
--echo #
diff --git a/sql/item.cc b/sql/item.cc
index 3448b23..ede1df6 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -5743,6 +5743,7 @@ String *Item::check_well_formed_result(String *str, bool send_error)
uint wlen= cs->cset->well_formed_len(cs,
str->ptr(), str->ptr() + str->length(),
str->length(), &well_formed_error);
+ null_value= false;
if (wlen < str->length())
{
THD *thd= current_thd;
___
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] Working on spider patches, MDEV-7698
Hi!
On Wed, Nov 30, 2016 at 3:26 AM, kentoku wrote:
> Hi!
>
>> If this is ok, I will add this and close MDEV-7700
>
> It looks connect string is visible, but never used. This is different
> from what I think.
> I just added patch for connect string to MDEV-7698. Please review it.
> This changes do not cause error for federated_partition test.
Let's discuss this today when you are at my place.
Some quick comments and questions:
- Did you see my patch to remember table->s->connect_string, even if
partition tables are using it. Why isn't this fixing the problem?
About the patch:
> diff -Narup ./server/sql/ha_partition.cc
> ./server-with-connect-string-patch/sql/ha_partition.cc
> --- ./server/sql/ha_partition.cc2016-11-29 19:56:47.037326899 +0900
> +++ ./server-with-connect-string-patch/sql/ha_partition.cc2016-11-30
> 09:57:24.0 +0900
> @@ -1515,7 +1515,10 @@ int ha_partition::prepare_new_partition(
>if ((error= set_up_table_before_create(tbl, part_name, create_info,
> p_elem)))
> goto error_create;
>
> + if (!(file->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> + {
>tbl->s->connect_string = p_elem->connect_string;
> + }
I am not sure that adding a HTON is the right way.
Why isn't enough to in all cases just ensure that the handler for the
underlaying tables are not changing the connect string given by the end user?
This is what my patch is doing.
One benefit of my patch is that we will remember the global the connect string
in all cases, which this patch doesn't guarantee.
>if ((error= file->ha_create(part_name, tbl, create_info)))
>{
> /*
> @@ -2554,10 +2560,13 @@ int ha_partition::set_up_table_before_cr
>}
>info->index_file_name= part_elem->index_file_name;
>info->data_file_name= part_elem->data_file_name;
> + if (!(m_file[0]->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> + {
>info->connect_string= part_elem->connect_string;
>if (info->connect_string.length)
> info->used_fields|= HA_CREATE_USED_CONNECTION;
>tbl->s->connect_string= part_elem->connect_string;
> + }
The above is probably wrong. If the partition engine had before store
connect strings for the engine, it should be able to read them in the future
too, even if the flags has changed value.
> @@ -3488,11 +3497,17 @@ int ha_partition::open(const char *name,
> {
>create_partition_name(name_buff, name, name_buffer_ptr,
> NORMAL_PART_NAME,
> FALSE);
> + if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> + {
>table->s->connect_string = m_connect_string[(uint)(file-m_file)];
> + }
I assume this is the real reason for your patch.
You want to ensure that if the partition doesn't support connect
strings and there was no connect strings given to the partition, we
should send in the main table's partition string.
ok, there is a point. I will fix this.
>if ((error= (*file)->ha_open(table, name_buff, mode,
> test_if_locked | HA_OPEN_NO_PSI_CALL)))
> goto err_handler;
> + if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> + {
>bzero(&table->s->connect_string, sizeof(LEX_STRING));
> + }
I think the above is always wrong. Better to always restore the connect_string
to it's original value.
> --- ./server/sql/sql_table.cc2016-11-29 19:56:47.118326899 +0900
> +++ ./server-with-connect-string-patch/sql/sql_table.cc2016-11-30
> 09:47:28.0 +0900
> @@ -7978,6 +7978,11 @@ mysql_prepare_alter_table(THD *thd, TABL
> create_info->comment.str= table->s->comment.str;
> create_info->comment.length= table->s->comment.length;
>}
> + if (!create_info->connect_string.str)
> + {
> +create_info->connect_string.str= table->s->connect_string.str;
> +create_info->connect_string.length= table->s->connect_string.length;
> + }
We should not need this as we have already in the same function:
if (!(used_fields & HA_CREATE_USED_CONNECTION))
create_info->connect_string= table->s->connect_string;
> diff -Narup ./server/sql/table.cc
> ./server-with-connect-string-patch/sql/table.cc
> --- ./server/sql/table.cc2016-11-29 19:56:47.129326899 +0900
> +++ ./server-with-connect-string-patch/sql/table.cc2016-11-30
> 09:50:45.0 +0900
> @@ -3758,6 +3758,7 @@ void update_create_info_from_table(HA_CR
>create_info->default_table_charset= share->table_charset;
>create_info->table_charset= 0;
>create_info->comment= share->comment;
> + create_info->connect_string= share->connect_string;
>create_info->transactional= share->transactional;
>create_info->page_checksum= share->page_checksum;
>create_info->option_list= share->option_list;
Why is the above needed?
Do you have a test case that breaks if the above is not done?
Regards,
Monty
___
Mailing list: https://launchp
Re: [Maria-developers] Working on spider patches, MDEV-7701
Hi! >> However, I would like to have a clear comment the purpose of the function >> set_top_table_and_fields(). > > This function is used to get correlating of a parent (table/column) > and children (table/column). When conditions are pushed down to child > table (like child of myisammarge), child table needs to know about > which table/column is my parent for understanding conditions. Thanks. I have now added this as a comment to where the function is defined. >> + virtual int set_top_table_and_fields(TABLE *top_table, > ... cut ... >> Why is this int and not void ? >> Do you have in mind that this may be possible to fail in the future? >> If it's never going to change, then better to do this void. >> >> I will make it void for now and change the relevant code. We can make it >> int again if needed in the future. > > VP allocates some memories in this function. So this function has > possibility of causing out of memory error. That's why this function > returns int. ok. I will change the code for this. >> I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which >> allwoed me to make the above tests and the following identical ones >> much >> simpler. > > ok. And now everyone can create myisammarge type storage engine! good. We do need some tests for this... Regards, Monty ___ 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
