Re: [Tarantool-patches] [PATCH] box: sql prepare and execute statistics should be reflected in box.stat()

[Nikita Pettik <korablev@tarantool.org>]

On 27 Feb 01:08, Vladislav Shpilevoy wrote:
> Good catch, and shame on me for lgtming the patch!
> 
> On 26/02/2020 16:50, Maria Khaydich wrote:
> > Hey, I’ve been doing the 2.2 version for the same issue and realized there’s a bug in the commit that was already merged to master.
> >  
> >> 1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
> > see that in iproto.cc. This is a separate question, why do we collect
> > IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
> > here. 
> >  
> > The reason I was collecting stats for box.prepare here in the first place is because it looked to me like the execution of method sql_prepare_and_execute implied the user is preparing _and_ executing something at the same time,
> 
> From what I know, this method is invoked when we execute a non prepared
> statement. Such a query is not cached in the prep statements cache, and
> was not sent as PREPARE. So this would be a surprise for a user to see
> increased PREPARE after a usual execute().
> 
> > and whereas all other methods boiled down to either sql_prepare or sql_execute in the end, this one collected statistics only for IPROTO_EXECUTE, so I thought it was necessary to cover IPROTO_PREPARE as well. But your argument was aimed at something a bit different, so I missed the global picture here. 
> > The point is, there’s no need to do another rmean_collect(), as it happens later when sql_execute is being called. Otherwise the stats would have been collected twice for each executed statement as it was obvious in different tests (that I also added in the follow up patch).
> >  
> > Also, I’m wondering if we need to collect the stats for IPROTO_PREPARE in sql_reprepare as well?

sql_reprepare is a part of sql_prepare so we shouldn't account it.

> > It seems relevant due the nature of this method: «Re-compile statement and refresh global prepared statement cache with the newest value». Meaning, it’s another instance of prepare, right? 
> 
> This would be good to account somewhere, but certainly not here.
> box.stat() is not for such internal things. This is just request counters.
> Only for explicit requests. Reprepare would be good to add to box.stat.sql()
> some day.
> 
> I propose to just drop rmean_collect() from sql_prepare_and_execute(). This
> would solve the problem.

Exactly. Please send smth like 'hotfix' for the last commit, I guess it
is too late to force push to master.