Thanks for looking at the patch and for suggesting an alternative
approach. See some comments inline.
On 28 October 2010 14:49, Jorgen Loland <jorgen.loland@stripped> wrote:
> Hi Olav,
> Apart from Tor's request for documentation, the solution looks good.
> However, I think this bug can be solved by checking the range after all. The
> question is whether or not we want to invest time in fixing it since this is
> a fix for a non-prioritized SE. I have an outline for a solution:
> In mi_rkey(), inside the while() that checks the index condition, the newly
> found index record is stored in info->lastkey. Further, the end_range is
> stored in the ha_myisam object. You can get it from here
> These two keys can be fed to my_handler's ha_key_cmp(), something like this:
> <inside while(), after _mi_search_next()>
> if (((ha_myisam*)info->index_cond_func_arg)->end_range)
> int cmp_res= 0;
> uchar *end= ((ha_myisam*)info->index_cond_func_arg)->end_range->key;
> cmp_res= ha_key_cmp(keyinfo->seg, key_buff, end,
> use_key_length, SEARCH_FIND, not_used);
> if (cmp_res > 0)
I have tried out this suggestion and it seems to work. One extra thing
that needed to be implemented for this to work was that the handler's
end_range.key is on "server format" and is normally used for comparing
against values already converted to "server format" and stored in the
record0 buffer. Your suggested code is working directly against the
content of the index entry which is on MyISAM format. So to make this
work I had to convert ("pack") the end_range.key to MyISAM format
before doing any comparision.
I have a working prototype based on your suggestion that seems to work
fine with all tests in the main test suite passing.
> The problem is that mi_rkey.c does not include ha_myisam.h, so we'll need to
> connect these somehow. This can, e.g., be done by having a function pointer
> (as done with index_cond_func) or the end_range key in the info object.
A third alternative for this would be to move this check into the
existing index_cond_func_myisam() function in ha_myisam.cc. This would
then require this function to get access to the info->lastkey member.
If we solve this bug by adding this extra code for check of "end
range" to handle the special case for BLOB this would lead to
duplication of "end range" check. We would have one in
index_cond_func_myisam() that does this using the content of the
record0 buffer and one that do this check using the last read myisam
key entry. I think if we go for this solution we should convert all
testing of "end range" related to ICP to just use the last read key on
MyISAM format (some more work).
I think it would be good to do all these changes. Still, I think the
best way to solve this bug is to use my original approach to fix the
ICP bug and then rather file a feature request bug against MyISAM to
change the way MyISAM does "end range" check to use your suggested
approach. Any objections?