pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFrom: Julien Rouhaud <rjuju123@gmail.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: imai.yoshikazu@fujitsu.com <imai.yoshikazu@fujitsu.com>
Cc: legrand legrand <legrand_legrand@hotmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
Date: Wed, 25 Mar 2020 14:45:53 +0100
Message-ID: <20200325134553.GA14054@nol> (raw)
In-Reply-To: <bdfee4e0-a304-2498-8da5-3cb52c0a193e@oss.nttdata.com>
References: <CAFMSG9HJQr=H8doWJOp=wqyKbVqxMLkk_Qu2KfpmkKvS-Xn7qQ@mail.gmail.com>
<CAOBaU_YekXNsQ819w=eBTM_aQ+BYPcVWLX0sDO-5xRNhqipbRA@mail.gmail.com>
<OSAPR01MB46099E7D03547A2A7292716494FA0@OSAPR01MB4609.jpnprd01.prod.outlook.com>
<1584180240397-0.post@n3.nabble.com>
<20200314172733.mg7qpyumlyythm25@nol>
<TY2PR01MB46172D9531F2D3CE4C036C1A94F90@TY2PR01MB4617.jpnprd01.prod.outlook.com>
<20200316214912.iakenhp7vyd37hmg@nol>
<6300601584711975@vla4-87a00c2d2b1b.qloud-c.yandex.net>
<20200320193004.rqgf3iim4fugq3sm@nol>
<bdfee4e0-a304-2498-8da5-3cb52c0a193e@oss.nttdata.com>
On Wed, Mar 25, 2020 at 10:09:37PM +0900, Fujii Masao wrote:
>
> On 2020/03/21 4:30, Julien Rouhaud wrote:
> > On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
> > > Hello
> > >
> > > Yet another is missed in docs: total_time
> >
> > Oh good catch! I rechecked many time the field, and totally missed that the
> > documentation is referring to the view, which has an additional column, and not
> > the function. Attached v9 fixes that.
>
> Thanks for the patch! Here are the review comments from me.
>
> - PGSS_V1_3
> + PGSS_V1_3,
> + PGSS_V1_8
>
> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
> I *guess* that's because previously this version was maintained
> independently from pg_stat_statements' version. For example,
> pg_stat_statements 1.4 seems to have used PGSS_V1_3.
Oh right. It seems that I changed that many versions ago, I'm not sure why.
I'm personally fine with any, but I think this was previously raised and
consensus was to keep distinct counters. Unless you prefer to keep it this
way, I'll send an updated version (with other possible modifications depending
on the rest of the mail) using PGSS_V1_4.
> + /*
> + * We can't process the query if no query_text is provided, as pgss_store
> + * needs it. We also ignore query without queryid, as it would be treated
> + * as a utility statement, which may not be the case.
> + */
>
> Could you tell me why the planning stats are not tracked when executing
> utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
> the planner would work.
I explained that in [1]. The problem is that the underlying statement doesn't
get the proper stmt_location and stmt_len, so you eventually end up with two
different entries. I suggested fixing transformTopLevelStmt() to handle the
various DDL that can contain optimisable statements, but everyone preferred to
postpone that for a future enhencement.
> +static BufferUsage
> +compute_buffer_counters(BufferUsage start, BufferUsage stop)
> +{
> + BufferUsage result;
>
> BufferUsageAccumDiff() has very similar logic. Isn't it better to expose
> and use that function rather than creating new similar function?
Oh, I thought this wouldn't be acceptable. That's indeed better so I'll do
that instead.
> values[i++] = Int64GetDatumFast(tmp.rows);
> values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
> values[i++] = Int64GetDatumFast(tmp.shared_blks_read);
>
> Previously (without the patch) pg_stat_statements_1_3() reported
> the buffer usage counters updated only in execution phase. But,
> in the patched version, pg_stat_statements_1_3() reports the total
> of buffer usage counters updated in both planning and execution
> phases. Is this OK? I'm not sure how seriously we should ensure
> the backward compatibility for pg_stat_statements....
That's indeed a behavior change, although the new behavior is probably better
as user want to know how much resource a query is consuming overall. We could
distinguish all buffers with a plan/exec version, but it seems quite overkill.
> +/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */
>
> ISTM it's good timing to have also pg_stat_statements--1.8.sql since
> the definition of pg_stat_statements() is changed. Thought?
I thought that since CreateExtension() was modified to be able to find it's way
automatically, we shouldn't provide base version anymore, to minimize
maintenance burden and also avoid possible bug/discrepancy. The only drawback
is that it'll do multiple CREATE or DROP/CREATE of the function usually once
per database, which doesn't seem like a big problem.
[1] https://www.postgresql.org/message-id/CAOBaU_Y-y+VOhTZgDOuDk6-9V72-ZXdWccXo_kx0P4DDBEEh9A@mail.gmail...
view thread (127+ messages) latest in thread
Message-ID: <20200325134553.GA14054@nol>
Permalink: ../20200325134553.GA14054@nol/
Also on: postgresql.org/message-id/20200325134553.GA14054@nol
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rjuju123@gmail.com, masao.fujii@oss.nttdata.com, sk@zsrv.org, imai.yoshikazu@fujitsu.com, legrand_legrand@hotmail.com
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
In-Reply-To: <20200325134553.GA14054@nol>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox