List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:March 22 2010 1:01pm
Subject:Re: bzr commit into mysql-6.0-codebase-bugfixing branch
(roy.lyseng:3845) Bug#51110
View as plain text  
Hello Roy,

Roy Lyseng a écrit, Le 22.03.2010 13:21:
> #At file:///home/rl136806/mysql/repo/mysql-6.0-work3/ based on
> revid:alik@stripped
> 
>  3845 Roy Lyseng	2010-03-22
>       Bug#51110: Wrong result with COUNT(*) and semijoin
>       
>       The duplicate weedout semijoin strategy is shipping certain column
>       combinations to a temporary table, in order to filter out any duplicates
>       caused by reading duplicate rows from semijoined tables.
>       
>       The temporary table starts out as a memory table, but if the size reaches
>       a maximum value, it is converted to a disk-based table.
>       When such conversion occurs, the number of rows output from the query is
>       reported as one less than it should be.
>       
>       The reason for this is that when do_sj_dups_weedout() creates the
>       disk-based table, the return value is erroneously reported as 1
>       instead of 0, meaning that the row in question is always registered
>       as a duplicate. The problem is fixed by setting proper return values.
>       
>       mysql-test/r/subselect3.result
>         Fixed result for bug#51110.
>       
>       mysql-test/r/subselect3_jcl6.result
>         Fixed result for bug#51110.
>       
>       mysql-test/t/subselect3.test
>         Extended previously failing test to be performed with and without
>         temporary table overflow.
>       
>       sql/sql_select.cc
>         do_sj_dups_weedout() returns 1 if create_internal_tmp_table_from_heap()
>         is called successfully.
> 
>     modified:
>       mysql-test/r/subselect3.result
>       mysql-test/r/subselect3_jcl6.result
>       mysql-test/t/subselect3.test
>       sql/sql_select.cc
> === modified file 'mysql-test/r/subselect3.result'
> --- a/mysql-test/r/subselect3.result	2010-01-20 10:11:29 +0000
> +++ b/mysql-test/r/subselect3.result	2010-03-22 12:21:17 +0000
> @@ -1185,11 +1185,18 @@ id	select_type	table	type	possible_keys	
>  flush status;
>  select count(*) from t0 A, t0 B, t0 C, t0 D where D.a in (select a from t1 E);
>  count(*)
> -4999
> +5000

I admire how someone can have checked out a wrong test result and it 
also passed review.

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-03-19 19:16:53 +0000
> +++ b/sql/sql_select.cc	2010-03-22 12:21:17 +0000
> @@ -16737,13 +16737,17 @@ int do_sj_dups_weedout(THD *thd, SJ_TMP_
>    error=
> sjtbl->tmp_table->file->ha_write_row(sjtbl->tmp_table->record[0]);
>    if (error)
>    {
> -    /* create_internal_tmp_table_from_heap will generate error if needed */
> -    if (sjtbl->tmp_table->file->is_fatal_error(error, HA_CHECK_DUP)
> &&
> -        create_internal_tmp_table_from_heap(thd, sjtbl->tmp_table,
> +    /* If this is a duplicate error, return immediately */
> +    if (!sjtbl->tmp_table->file->is_fatal_error(error, HA_CHECK_DUP))
> +      DBUG_RETURN(1);
> +    /*
> +      Other error than duplicate error: Attempt to create a temporary table.
> +      If creation is successful, the row is not a duplicate.
> +    */
> +    if (create_internal_tmp_table_from_heap(thd, sjtbl->tmp_table,
>                                              sjtbl->start_recinfo,
>                                              &sjtbl->recinfo, error, 1))
>        DBUG_RETURN(-1);
> -    DBUG_RETURN(1);
>    }
>    DBUG_RETURN(0);
>  }

I don't understand. Here write_row() is calling heap_write(); that 
function first looks for a free position in the table 
(next_free_record_pos()), and the size limit is checked there.
So, no matter if the record is or not a duplicate, write_row() will 
return HA_ERR_RECORD_FILE_FULL, so we will come to 
create_internal_tmp_table_from_heap() which will succeed, so we will 
(after the patch) return 0 (non-duplicate). What if the row was actually 
a duplicate? These are all guesses, maybe a testcase can be crafted.
How about a loop:

bool retried= false;
retry:
error= write_row()
if (error)
{
   // row was not inserted
   if it_is_a_"duplicate_row"_error
      DBUG_RETURN(1);
   if (retried)
     DBUG_RETURN(-1); // real error
   if create_internal_tmp_table etc
      DBUG_RETURN(-1);
   // now table is converted and ready for insertion
   retried= true;
   goto retry;
}
DBUG_RETURN(0);


-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com
Thread
bzr commit into mysql-6.0-codebase-bugfixing branch (roy.lyseng:3845)Bug#51110Roy Lyseng22 Mar
  • Re: bzr commit into mysql-6.0-codebase-bugfixing branch(roy.lyseng:3845) Bug#51110Guilhem Bichot22 Mar
    • Re: bzr commit into mysql-6.0-codebase-bugfixing branch(roy.lyseng:3845) Bug#51110Roy Lyseng22 Mar
      • Re: bzr commit into mysql-6.0-codebase-bugfixing branch(roy.lyseng:3845) Bug#51110Guilhem Bichot22 Mar
        • Re: bzr commit into mysql-6.0-codebase-bugfixing branch(roy.lyseng:3845) Bug#51110Roy Lyseng23 Mar