From: Roy Lyseng
Date: June 20 2011 8:28am
Subject: Re: review of WL#5860: Make COST_VECT a properly encapsulated C++
 class
List-Archive: http://lists.mysql.com/commits/139490
Message-Id: <4DFF04B6.70102@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 18.06.11 12.09, Guilhem Bichot wrote:
> Hello,
>
> 1)
> + bool is_zero() const
> + {
> + if (io_cost || cpu_cost || import_cost || mem_cost)
> + return false;
> + return true;
>
> return !(io_cost || cpu_cost || import_cost || mem_cost);
> is simpler.
>
> 2) In definition of class, short comments like:
> + /**
> + Add to IO cost
> + */
> could be shorter on one line:
> /// Add to IO cost
>
> 3) While you're changing DsMrr_impl::choose_mrr_impl(), could you move the
> declaration of "COST_VECT dsmrr_cost" down to right before the call to
> get_disk_sweep_mrr_cost()?
>
> 4) Biggest question:
> @@ -14105,7 +14107,8 @@ void Optimize_table_order::advance_sj_st
>
> DBUG_ENTER("Optimize_table_order::advance_sj_state");
>
> - pos->prefix_cost.convert_from_cost(*current_read_time);
> + DBUG_ASSERT(pos->prefix_cost.is_zero());
> + pos->prefix_cost.add_io(*current_read_time);
> I'm not 100% sure that the assertion will never fire, as a POSITION serves
> during evaluation of multiple candidate plans, without, IIUC, any resetting in
> between; could a next pass find a non-zero cost left by a previous pass? I
> suggest asking Roy to double check this.

It looks weird to me. However, I guess the main problem here is that 
advance_sj_state() looks at cost data partially in pos->prefix_cost and 
partially in *current_read_time.

Thanks,
Roy