maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #03960
Igor please review: BUG#707925: Wrong result with join_cache_level=6 optimizer_use_mrr = force (incremental, BKA join) in file:///home/psergey/dev2/5.3/]
On Wed, Mar 02, 2011 at 08:04:39AM -0800, Igor Babaev wrote:
> > +SELECT alias2.f1 AS field1 , alias1.f2 AS field2
> > +FROM t1 AS alias1
> > +LEFT OUTER JOIN t1 AS alias2 ON alias1.f2 = alias2.f5
> > +WHERE alias2.f3 > 'k'
> > +AND alias2.f3 <= 'z'
> > +OR alias2.f4 IN ('o', 'y')
> > +AND alias2.f4 >= 'g' AND alias2.f4 < 'z';
> > +field1 field2
> > +Warnings:
> > +Warning 1292 Truncated incorrect DOUBLE value: 'k'
>
> Why do we need comparison of a string with an integer that generates the
> above warning? Can we do without it? Why not to use char(4) instead
> if integer?
Got rid of string vs integer comparison, as well as simplified the testcase a
bit (further simplification is difficult). Please find the result below:
----- Forwarded message from Sergey Petrunya <psergey@xxxxxxxxxxxx> -----
From: Sergey Petrunya <psergey@xxxxxxxxxxxx>
To: commits@xxxxxxxxxxx
X-Mailer: mail (GNU Mailutils 1.2)
Date: Wed, 2 Mar 2011 17:06:15 +0300 (MSK)
Subject: [Commits] Rev 2929: BUG#707925: Wrong result with
join_cache_level=6 optimizer_use_mrr = force (incremental,
BKA join) in file:///home/psergey/dev2/5.3/
At file:///home/psergey/dev2/5.3/
------------------------------------------------------------
revno: 2929
revision-id: psergey@xxxxxxxxxxxx-20110302140613-4qt8xvdvxs4t4m98
parent: psergey@xxxxxxxxxxxx-20110301072222-1rmapkelx9l1kw8u
committer: Sergey Petrunya <psergey@xxxxxxxxxxxx>
branch nick: 5.3
timestamp: Wed 2011-03-02 17:06:13 +0300
message:
BUG#707925: Wrong result with join_cache_level=6 optimizer_use_mrr = force (incremental, BKA join)
- Add debugging aid which maps MRR's range_ids (which are pointers so are different on every execution
and hard to track) to range numbers (which allow much easier debugging). The mapper is not compiled
unless the developer puts a #define into code to request it.
- Fix the bug: the problem was that Mrr_ordered_index_reader's interrupt_read() and resume_read() would
save and restore 1) index tuple 2) the rowid (as bytes returned by handler->position()). Clustered
primary key columns were not saved/restored.
They are not explicitly present in the index tuple (i.e. table->key_info[secondary_key].key_parts
doesn't list them), but they are actually there, in particular
table->field[clustered_primary_key_member].part_of_key(secondary_key) == 1. Index condition pushdown
code [correctly] uses the latter as inidication that pushed index condition can refer to clustered PK
members.
The fix was to make interrupt_read()/resume_read() to save/restore clustered primary key members as well,
so that we get correct values for them when evaluating pushed index condition.
[Second attempt, make the code compile without the debugging aid]
=== modified file 'mysql-test/r/innodb_mrr.result'
--- a/mysql-test/r/innodb_mrr.result 2011-01-17 21:26:04 +0000
+++ b/mysql-test/r/innodb_mrr.result 2011-03-02 14:06:13 +0000
@@ -736,3 +736,31 @@
c1 c2 c3
08:29:45 NULL 2009-02-01
drop table `t1`;
+#
+# BUG#707925: Wrong result with join_cache_level=6 optimizer_use_mrr = force (incremental, BKA join)
+#
+set @_save_join_cache_level= @@join_cache_level;
+set join_cache_level = 6;
+CREATE TABLE t1 (
+f1 int(11), f2 int(11), f3 varchar(1), f4 varchar(1),
+PRIMARY KEY (f1),
+KEY (f3),
+KEY (f2)
+) ENGINE=InnoDB;
+INSERT INTO t1 VALUES ('11','8','f','f'),('12','5','v','v'),('13','8','s','s'),
+('14','8','a','a'),('15','6','p','p'),('16','7','z','z'),('17','2','a','a'),
+('18','5','h','h'),('19','7','h','h'),('20','2','v','v'),('21','9','v','v'),
+('22','142','b','b'),('23','3','y','y'),('24','0','v','v'),('25','3','m','m'),
+('26','5','z','z'),('27','9','n','n'),('28','1','d','d'),('29','107','a','a');
+select count(*) from (
+SELECT alias1.f2
+FROM
+t1 AS alias1 JOIN (
+t1 AS alias2 FORCE KEY (f3) JOIN
+t1 AS alias3 FORCE KEY (f2) ON alias3.f2 = alias2.f2 AND alias3.f4 = alias2.f3
+) ON alias3.f1 <= alias2.f1
+) X;
+count(*)
+361
+set join_cache_level=@_save_join_cache_level;
+drop table t1;
=== modified file 'mysql-test/t/innodb_mrr.test'
--- a/mysql-test/t/innodb_mrr.test 2011-01-17 21:26:04 +0000
+++ b/mysql-test/t/innodb_mrr.test 2011-03-02 14:06:13 +0000
@@ -426,3 +426,31 @@
SELECT * FROM t1 WHERE c2 <=> NULL ORDER BY c2 LIMIT 2;
drop table `t1`;
+--echo #
+--echo # BUG#707925: Wrong result with join_cache_level=6 optimizer_use_mrr = force (incremental, BKA join)
+--echo #
+set @_save_join_cache_level= @@join_cache_level;
+set join_cache_level = 6;
+CREATE TABLE t1 (
+ f1 int(11), f2 int(11), f3 varchar(1), f4 varchar(1),
+ PRIMARY KEY (f1),
+ KEY (f3),
+ KEY (f2)
+) ENGINE=InnoDB;
+INSERT INTO t1 VALUES ('11','8','f','f'),('12','5','v','v'),('13','8','s','s'),
+('14','8','a','a'),('15','6','p','p'),('16','7','z','z'),('17','2','a','a'),
+('18','5','h','h'),('19','7','h','h'),('20','2','v','v'),('21','9','v','v'),
+('22','142','b','b'),('23','3','y','y'),('24','0','v','v'),('25','3','m','m'),
+('26','5','z','z'),('27','9','n','n'),('28','1','d','d'),('29','107','a','a');
+
+select count(*) from (
+ SELECT alias1.f2
+ FROM
+ t1 AS alias1 JOIN (
+ t1 AS alias2 FORCE KEY (f3) JOIN
+ t1 AS alias3 FORCE KEY (f2) ON alias3.f2 = alias2.f2 AND alias3.f4 = alias2.f3
+ ) ON alias3.f1 <= alias2.f1
+) X;
+
+set join_cache_level=@_save_join_cache_level;
+drop table t1;
=== modified file 'sql/multi_range_read.cc'
--- a/sql/multi_range_read.cc 2011-02-18 22:31:01 +0000
+++ b/sql/multi_range_read.cc 2011-03-02 14:06:13 +0000
@@ -387,15 +387,25 @@
bool Mrr_ordered_index_reader::set_interruption_temp_buffer(uint rowid_length,
uint key_len,
+ uint saved_pk_len,
uchar **space_start,
uchar *space_end)
{
- if (space_end - *space_start <= (ptrdiff_t)(rowid_length + key_len))
+ if (space_end - *space_start <= (ptrdiff_t)(rowid_length + key_len + saved_pk_len))
return TRUE;
support_scan_interruptions= TRUE;
saved_rowid= *space_start;
*space_start += rowid_length;
+
+ if (saved_pk_len)
+ {
+ saved_primary_key= *space_start;
+ *space_start += saved_pk_len;
+ }
+ else
+ saved_primary_key= NULL;
+
saved_key_tuple= *space_start;
*space_start += key_len;
@@ -406,17 +416,25 @@
void Mrr_ordered_index_reader::set_no_interruption_temp_buffer()
{
support_scan_interruptions= FALSE;
- saved_key_tuple= saved_rowid= NULL; /* safety */
+ saved_key_tuple= saved_rowid= saved_primary_key= NULL; /* safety */
have_saved_rowid= FALSE;
}
void Mrr_ordered_index_reader::interrupt_read()
{
DBUG_ASSERT(support_scan_interruptions);
+ TABLE *table= file->get_table();
/* Save the current key value */
- key_copy(saved_key_tuple, file->get_table()->record[0],
- &file->get_table()->key_info[file->active_index],
+ key_copy(saved_key_tuple, table->record[0],
+ &table->key_info[file->active_index],
keypar.key_tuple_length);
+
+ if (saved_primary_key)
+ {
+ key_copy(saved_primary_key, table->record[0],
+ &table->key_info[table->s->primary_key],
+ table->key_info[table->s->primary_key].key_length);
+ }
/* Save the last rowid */
memcpy(saved_rowid, file->ref, file->ref_length);
@@ -433,9 +451,16 @@
void Mrr_ordered_index_reader::resume_read()
{
- key_restore(file->get_table()->record[0], saved_key_tuple,
- &file->get_table()->key_info[file->active_index],
+ TABLE *table= file->get_table();
+ key_restore(table->record[0], saved_key_tuple,
+ &table->key_info[file->active_index],
keypar.key_tuple_length);
+ if (saved_primary_key)
+ {
+ key_restore(table->record[0], saved_primary_key,
+ &table->key_info[table->s->primary_key],
+ table->key_info[table->s->primary_key].key_length);
+ }
}
@@ -738,8 +763,8 @@
@retval other Error
*/
-int DsMrr_impl::dsmrr_init(handler *h_arg, RANGE_SEQ_IF *seq_funcs,
- void *seq_init_param, uint n_ranges, uint mode,
+int DsMrr_impl::dsmrr_init(handler *h_arg, RANGE_SEQ_IF *seq_funcs_arg,
+ void *seq_init_param_arg, uint n_ranges, uint mode,
HANDLER_BUFFER *buf)
{
THD *thd= current_thd;
@@ -748,6 +773,8 @@
uint key_buff_elem_size;
handler *h_idx;
Mrr_ordered_rndpos_reader *disk_strategy= NULL;
+ RANGE_SEQ_IF seq_funcs= *seq_funcs_arg;
+ void *seq_init_param= seq_init_param_arg;
bool do_sort_keys= FALSE;
DBUG_ENTER("DsMrr_impl::dsmrr_init");
LINT_INIT(key_buff_elem_size); /* set/used when do_sort_keys==TRUE */
@@ -810,8 +837,8 @@
{
/* Pre-calculate some parameters of key sorting */
keypar.use_key_pointers= test(mode & HA_MRR_MATERIALIZED_KEYS);
- seq_funcs->get_key_info(seq_init_param, &keypar.key_tuple_length,
- &keypar.key_tuple_map);
+ seq_funcs.get_key_info(seq_init_param, &keypar.key_tuple_length,
+ &keypar.key_tuple_map);
keypar.key_size_in_keybuf= keypar.use_key_pointers?
sizeof(char*) : keypar.key_tuple_length;
key_buff_elem_size= keypar.key_size_in_keybuf + (int)is_mrr_assoc * sizeof(void*);
@@ -819,9 +846,18 @@
/* Ordered index reader needs some space to store an index tuple */
if (strategy != index_strategy)
{
+ /* Normally we don't need to save/restore PK columns*/
+ uint saved_pk_length=0;
+ if (h_idx->primary_key_is_clustered())
+ {
+ uint pk= h_idx->get_table()->s->primary_key;
+ saved_pk_length= h_idx->get_table()->key_info[pk].key_length;
+ }
+
if (reader_factory.ordered_index_reader.
set_interruption_temp_buffer(primary_file->ref_length,
keypar.key_tuple_length,
+ saved_pk_length,
&full_buf, full_buf_end))
goto use_default_impl;
}
@@ -845,7 +881,9 @@
if (do_sort_keys && !key_buffer->have_space_for(key_buff_elem_size))
goto use_default_impl;
- if ((res= index_strategy->init(primary_file, seq_funcs, seq_init_param, n_ranges,
+ MRR_DEBUG(setup_range_id_mapping(&seq_funcs, &seq_init_param, n_ranges));
+
+ if ((res= index_strategy->init(primary_file, &seq_funcs, seq_init_param, n_ranges,
mode, &keypar, key_buffer, &buf_manager)))
goto error;
}
@@ -873,7 +911,9 @@
if ((res= setup_two_handlers()))
goto error;
- if ((res= index_strategy->init(secondary_file, seq_funcs, seq_init_param,
+ MRR_DEBUG(setup_range_id_mapping(&seq_funcs, &seq_init_param, n_ranges));
+
+ if ((res= index_strategy->init(secondary_file, &seq_funcs, seq_init_param,
n_ranges, mode, &keypar, key_buffer,
&buf_manager)) ||
(res= disk_strategy->init(primary_file, index_strategy, mode,
@@ -883,6 +923,7 @@
}
}
+
res= strategy->refill_buffer(TRUE);
if (res)
{
@@ -910,7 +951,7 @@
DBUG_ASSERT(primary_file->inited == handler::INDEX);
/* Call correct init function and assign to top level object */
Mrr_simple_index_reader *s= &reader_factory.simple_index_reader;
- res= s->init(primary_file, seq_funcs, seq_init_param, n_ranges, mode, NULL,
+ res= s->init(primary_file, &seq_funcs, seq_init_param, n_ranges, mode, NULL,
NULL, NULL);
strategy= s;
DBUG_RETURN(res);
@@ -1033,6 +1074,7 @@
DBUG_ENTER("DsMrr_impl::dsmrr_close");
close_second_handler();
strategy= NULL;
+ MRR_DEBUG(clear_range_id_mapping());
DBUG_VOID_RETURN;
}
@@ -1690,7 +1732,110 @@
DBUG_VOID_RETURN;
}
-
+#ifdef DBUG_MRR
+range_seq_t DsMrr_impl::hook_init(void *init_params, uint n_ranges, uint flags)
+{
+ DsMrr_impl *mrr= (DsMrr_impl*)init_params;
+ mrr->backend_seq= mrr->backend_init(mrr->backend_init_params, n_ranges, flags);
+ return (range_seq_t)mrr;
+}
+
+bool DsMrr_impl::hook_next(range_seq_t seq, KEY_MULTI_RANGE *range)
+{
+ DsMrr_impl *mrr= (DsMrr_impl*)seq;
+ int res;
+ if (!(res= mrr->backend_next(mrr->backend_seq, range)))
+ {
+ DBUG_ASSERT(mrr->cur_range_no < mrr->n_range_ids);
+ mrr->range_ids[mrr->cur_range_no++]= range->ptr;
+ }
+ return res;
+}
+
+void DsMrr_impl::hook_get_key_info(void *init_params, uint *length, key_part_map *map)
+{
+ DsMrr_impl *mrr= (DsMrr_impl*)init_params;
+ mrr->backend_get_key_info(mrr->backend_init_params, length, map);
+}
+
+bool DsMrr_impl::hook_skip_index_tuple(range_seq_t seq, char *range_info)
+{
+ DsMrr_impl *mrr= (DsMrr_impl*)seq;
+ int rangeno= mrr->range_id_to_number(range_info);
+ bool res= mrr->backend_skip_index_tuple(mrr->backend_seq, range_info);
+ fprintf(stderr, "checking index tuple %d -> %d\n",
+ rangeno,
+ (int)res);
+ return res;
+}
+
+bool DsMrr_impl::hook_skip_record(range_seq_t seq, char *range_info, uchar *rowid)
+{
+ DsMrr_impl *mrr= (DsMrr_impl*)seq;
+ bool res= mrr->backend_skip_record(mrr->backend_seq, range_info, rowid);
+ return res;
+}
+
+void DsMrr_impl::setup_range_id_mapping(RANGE_SEQ_IF *rseq, void **init_params, uint n_ranges)
+{
+ /*
+ Hook ourselves into RANGE_SEQ_IF so that we can watch ranges as they are
+ enumerated
+ */
+ backend_init= rseq->init;
+ rseq->init= hook_init;
+
+ backend_next= rseq->next;
+ rseq->next= hook_next;
+
+ backend_init_params= *init_params;
+ *init_params= (void*)this;
+
+ if (rseq->get_key_info)
+ {
+ backend_get_key_info= rseq->get_key_info;
+ rseq->get_key_info= hook_get_key_info;
+ }
+
+ if (rseq->skip_index_tuple)
+ {
+ backend_skip_index_tuple= rseq->skip_index_tuple;
+ rseq->skip_index_tuple= hook_skip_index_tuple;
+ }
+
+ if (rseq->skip_record)
+ {
+ backend_skip_record= rseq->skip_record;
+ rseq->skip_record= hook_skip_record;
+ }
+
+ /* Alloc space for the mapping */
+ n_range_ids= n_ranges;
+ if (n_ranges)
+ range_ids= (char**)malloc(sizeof(char*) * n_ranges);
+ else
+ range_ids= NULL;
+ cur_range_no= 0;
+}
+
+void DsMrr_impl::clear_range_id_mapping()
+{
+ if (range_ids)
+ free(range_ids);
+ range_ids= NULL;
+ n_range_ids= 0;
+}
+
+int DsMrr_impl::range_id_to_number(char *range_id)
+{
+ for (int i=0; i < n_range_ids; i++)
+ {
+ if (range_ids[i] == range_id)
+ return i;
+ }
+ return -1;
+}
+#endif
/* **************************************************************************
* DS-MRR implementation ends
***************************************************************************/
=== modified file 'sql/multi_range_read.h'
--- a/sql/multi_range_read.h 2010-12-20 11:40:12 +0000
+++ b/sql/multi_range_read.h 2011-03-02 14:06:13 +0000
@@ -271,7 +271,8 @@
mrr_funcs.skip_index_tuple(mrr_iter, range_info));
}
- bool set_interruption_temp_buffer(uint rowid_length, uint key_len,
+ bool set_interruption_temp_buffer(uint rowid_length, uint key_len,
+ uint saved_pk_len,
uchar **space_start, uchar *space_end);
void set_no_interruption_temp_buffer();
@@ -314,6 +315,7 @@
bool have_saved_rowid;
uchar *saved_key_tuple;
+ uchar *saved_primary_key;
static int compare_keys(void* arg, uchar* key1, uchar* key2);
static int compare_keys_reverse(void* arg, uchar* key1, uchar* key2);
@@ -520,7 +522,12 @@
typedef void (handler::*range_check_toggle_func_t)(bool on);
DsMrr_impl()
- : secondary_file(NULL) {};
+ : secondary_file(NULL)
+ {
+#ifdef DBUG_MRR
+ range_id_mapping_ctor();
+#endif
+ }
void init(handler *h_arg, TABLE *table_arg)
{
@@ -615,6 +622,52 @@
int setup_two_handlers();
void close_second_handler();
+
+#ifdef DBUG_MRR
+#define MRR_DEBUG(x) x
+ /*
+ Debugging aid: support for converting range_ids (which are pointers) to
+ range numbers. Range's number specifies when the range was returned by
+ RANGE_SEQ_IF::next(). First returned range gets number 0, second gets
+ number 1, and so on.
+ */
+ char **range_ids; /* range_ids[N] == range_id of range with number N */
+ int n_range_ids; /* Total number of range ids */
+
+ /* "current" range number (makes sense when walking the ranges) */
+ int cur_range_no;
+
+
+ /* RANGE_SEQ_IF override: "backend" functions */
+ range_seq_t (*backend_init)(void *init_params, uint n_ranges, uint flags);
+ bool (*backend_next)(range_seq_t seq, KEY_MULTI_RANGE *range);
+ void (*backend_get_key_info)(void *init_params, uint *length, key_part_map *map);
+ bool (*backend_skip_index_tuple)(range_seq_t seq, char *range_info);
+ bool (*backend_skip_record)(range_seq_t seq, char *range_info, uchar *rowid);
+ void *backend_init_params;
+ range_seq_t backend_seq;
+
+ /* RANGE_SEQ_IF override: our hook-functions */
+ static range_seq_t hook_init(void *init_params, uint n_ranges, uint flags);
+ static bool hook_next(range_seq_t seq, KEY_MULTI_RANGE *range);
+ static void hook_get_key_info(void *init_params, uint *length, key_part_map *map);
+ static bool hook_skip_index_tuple(range_seq_t seq, char *range_info);
+ static bool hook_skip_record(range_seq_t seq, char *range_info, uchar *rowid);
+
+ /* Mapper [de]initialization functions*/
+ void range_id_mapping_ctor()
+ {
+ range_ids= NULL;
+ n_range_ids= 0;
+ }
+ void setup_range_id_mapping(RANGE_SEQ_IF *rseq, void **init_params, uint n_ranges);
+ void clear_range_id_mapping();
+
+ /* The reason of all of the above: function to map range ids to range numbers */
+ int range_id_to_number(char *range_id);
+#else
+#define MRR_DEBUG(x)
+#endif
};
/**
_______________________________________________
commits mailing list
commits@xxxxxxxxxxx
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
----- End forwarded message -----
--
BR
Sergey
--
Sergey Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog
