From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CA0EC469719 for ; Thu, 27 Feb 2020 16:21:14 +0300 (MSK) Date: Thu, 27 Feb 2020 16:21:13 +0300 From: Nikita Pettik Message-ID: <20200227132113.GC12687@tarantool.org> References: <1581359737.397395198@f221.i.mail.ru> <20200225130257.GA12687@tarantool.org> <1a3127f4-df43-2e45-7045-64b7bfd395b1@tarantool.org> <1582732235.10826512@f556.i.mail.ru> <11433298-80bb-336c-06a2-ce310634960f@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <11433298-80bb-336c-06a2-ce310634960f@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box: sql prepare and execute statistics should be reflected in box.stat() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches 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.