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

[Vladislav Shpilevoy]

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? 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.

sql_prepare_and_execute() will be accounted as EXECUTE due to calling sql_execute().
sql_execute() will be accounted as EXECUTE.
sql_prepare() as PREPARE.

Nothing more.