Re: BUG: pg_stat_statements query normalization issues with combined queries

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: coelho(at)cri(dot)ensmp(dot)fr
Cc: tgl(at)sss(dot)pgh(dot)pa(dot)us, craig(dot)ringer(at)2ndquadrant(dot)com, robertmhaas(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: BUG: pg_stat_statements query normalization issues with combined queries
Date: 2017-01-12 13:12:33
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


At Fri, 30 Dec 2016 15:10:42 +0100 (CET), Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote in <alpine(dot)DEB(dot)2(dot)20(dot)1612301453280(dot)32017(at)lancre>
> Hello,
> >> Yeah, that's what I was thinking of. There aren't very many places
> >> that
> >> would need to know about that, I believe; [...]
> >
> > For fixing the information in pg_stat_statement, the location data
> > must be transported from the parsed node to the query to the planned
> > node, because the later two nodes types are passed to different hooks.
> >
> > Now the detail is that utility statements, which seems to be nearly
> > all of them but select/update/delete/insert, do not have plans: The
> > statement itself is its own plan... so there is no place to store the
> > location & length.
> Here is an updated version:
> Changes wrt v2:
> - I have added the missing stuff under /nodes/, this is stupid code that
> should be automatically generated:-(
> - I have added comments in "gram.y" about how the length is computed.
> I have also slightly simplified the rule code there.
> - I have rename "location" in create table space to "location_dir"
> to avoid confusion.
> - I have renamed the fields "location" and "length" instead of q*.
> - I have moved the location & lenth copies in standard_planner.
> - I have fixed the function declaration typo.
> - I have simplified pgss_store code to avoid a copy, and move the
> length truncation in qtext_store.
> - I have improved again the pg_stat_statement regression tests with
> combined utility statement tests, which implied some fixes to
> extract the right substring for utility queries.
> However, not changed:
> - I cannot use the intermediate node trick suggested by Tom because
> it does not work for utility statements which do not have plans, so
> the code still adds location & length, sorry.

Wrapping the output of pg_parse_query still works for non-utility
commands. But pg_stat_statement retrieves the location
information via ProcessUtility, which has plantree. Wrapping
plantree with the same struct also works but it imacts too widely
and extremely error-prone.

One available solution is, as Fabien did, bring location and
length data along with transformation. And anther is give a
chopped query to query tree.

The attached patch does the second way (but quite ugly and
incomplete, it's PoC). This seems working as expected to a
certain degree but somewhat bogus.. A significant drawback of
this is that this makes a copy of each query in multistatement.

> - I still use the 'last_semicolon' lexer variable. The alternative is to
> change rules so as not to skip empty statements, then write a loop to
> compute the length based on successor location, and remove the empty
> statements. It can be done, I do not think it is better, it is only
> different and more verbose. I'll do it if required by a committer.

I think this is doable in the way shown in this patch. But this
seems somewhat bogus, too..


Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
0001-Fix-current-query-string-of-query-tree.patch text/x-patch 13.4 KB
0002-Change-regtest-of-pg_stat_statements.patch text/x-patch 14.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2017-01-12 13:34:10 Re: Proposal for changes to recovery.conf API
Previous Message Tom Lane 2017-01-12 13:09:02 Re: Unused member root in foreign_glob_cxt