Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.92) (envelope-from ) id 1jHmku-0003wA-OI for pgsql-hackers@arkaria.postgresql.org; Fri, 27 Mar 2020 11:02:28 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1jHmkt-0003vx-FQ for pgsql-hackers@arkaria.postgresql.org; Fri, 27 Mar 2020 11:02:27 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1jHmkt-0003sh-6J for pgsql-hackers@lists.postgresql.org; Fri, 27 Mar 2020 11:02:27 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jHmkn-0002PK-U2 for pgsql-hackers@postgresql.org; Fri, 27 Mar 2020 11:02:25 +0000 Received: from hnk.local (sp49-97-92-76.msc.spmode.ne.jp [49.97.92.76]) by oss.nttdata.com (Postfix) with ESMTPSA id D02156064A; Fri, 27 Mar 2020 20:02:15 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.102.2 at oss.nttdata.com Subject: Re: Planning counters in pg_stat_statements (using pgss_store) To: Julien Rouhaud Cc: Sergei Kornilov , "imai.yoshikazu@fujitsu.com" , legrand legrand , "pgsql-hackers@postgresql.org" References: <1584180240397-0.post@n3.nabble.com> <20200314172733.mg7qpyumlyythm25@nol> <20200316214912.iakenhp7vyd37hmg@nol> <6300601584711975@vla4-87a00c2d2b1b.qloud-c.yandex.net> <20200320193004.rqgf3iim4fugq3sm@nol> <20200325134553.GA14054@nol> <20200326132242.GA80836@nol> <20200327100001.qarktrnxldmoxzgd@nol> From: Fujii Masao Message-ID: Date: Fri, 27 Mar 2020 20:02:15 +0900 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200327100001.qarktrnxldmoxzgd@nol> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk On 2020/03/27 19:00, Julien Rouhaud wrote: > On Thu, Mar 26, 2020 at 02:22:47PM +0100, Julien Rouhaud wrote: >> On Thu, Mar 26, 2020 at 08:08:35PM +0900, Fujii Masao wrote: >>> >>> Here are other comments. >>> >>> - if (jstate) >>> + if (kind == PGSS_JUMBLE) >>> >>> Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead. >>> >>> If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead >>> and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought? >> >> Yes, we could be using jstate here. I originally used that to avoid passing >> PGSS_EXEC (or the other one) as a way to say "ignore this information as >> there's the jstate which says it's yet another meaning". If that's not an >> issue, I can change that as PGSS_NUM_KIND will clearly improve the explicit "2" >> all over the place. > > Done, passing PGSS_PLAN when jumble is intended, with a comment saying that the > pgss_kind is ignored in that case. > >>> + total_time >>> + double precision >>> + >>> + >>> + Total time spend planning and executing the statement, in milliseconds >>> + >>> + >>> >>> pg_stat_statements view has this column but the function not. >>> We should make both have the column or not at all, for consistency? >>> I'm not sure if it's good thing to expose the sum of total_plan_time >>> and total_exec_time as total_time. If some users want that, they can >>> easily calculate it from total_plan_time and total_exec_time by using >>> their own logic. >> >> I think we originally added it as a way to avoid too much compatibility break, >> and also because it seems like a field most users will be interested in anyway. >> Now that I'm thinking about it again, I indeed think it was a mistake to have >> that in view part only. Not mainly for consistency, but for users who would be >> interested in the total_time field while not wanting to pay the overhead of >> retrieving the query text if they don't need it. So I'll change that! > > Done > >>> + nested_level++; >>> + PG_TRY(); >>> >>> In old thread [1], Tom Lane commented the usage of nested_level >>> in the planner hook. There seems no reply to that so far. What's >>> your opinion about that comment? >>> >>> [1] https://www.postgresql.org/message-id/28980.1515803777@sss.pgh.pa.us >> >> Oh thanks, I didn't noticed this part of the discussion. I agree with Tom's >> concern, and I think that having a specific nesting level variable for the >> planner is the best workaround, so I'll implement that. > > Done. > > I also exported BufferUsageAccumDiff as mentioned previously, as it seems > clearner and will avoid future useless code churn, and run pgindent. > > v10 attached. Thanks for updating the patches! Regarding 0001 patch, I have one nitpicking comment; - result = standard_planner(parse, cursorOptions, boundParams); + result = standard_planner(parse, query_text, cursorOptions, boundParams); -standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) +standard_planner(Query *parse, const char *querytext, int cursorOptions, + ParamListInfo boundParams) -pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams) +pg_plan_query(Query *querytree, const char *query_text, int cursorOptions, + ParamListInfo boundParams) The patch uses "query_text" and "querytext" as the name of newly-added argument. They should be unified? IMO "query_string" looks better name because it's used in other functions like pg_analyze_and_rewrite(), pg_parse_query() for the sake of consistency. Thought? Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters