From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, craig(dot)ringer(at)2ndquadrant(dot)com, robertmhaas(at)gmail(dot)com, PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: BUG: pg_stat_statements query normalization issues with combined queries |
Date: | 2017-01-12 18:00:47 |
Message-ID: | alpine.DEB.2.20.1701121752430.3788@lancre |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello Tom,
> Yeah, I doubt that this technique for getting the raw locations in the
> grammar+lexer works reliably.
It worked reliably on all tests, and the assumption seemed sound to me,
given the one-look-ahead property and that statement reductions can only
triggered when ';' or end of input comes.
> Anyway, I decided to see what it would take to do it the way I had
> in mind, which was to stick a separate RawStmt node atop each statement
> parsetree. The project turned out a bit larger than I hoped :-(,
Indeed: I tried it as it looked elegant, but it did not show to be the
simple thing you announced...
> [...] Furthermore, the output of pg_plan_queries is now always a list of
> PlannedStmt nodes, even for utility statements.
I stopped exactly there when I tried: I thought that changing that would
be enough to get a reject because it would be considered much too invasive
in too many places for fixing a small bug.
> So this ends up costing one extra palloc per statement, or two extra
> in the case of a utility statement, but really that's quite negligible
> compared to everything else that happens in processing a statement.
> As against that, I think it makes the related code a lot clearer.
Having spent some time there, I agree that a refactoring and cleanup was
somehow needed...
> The sheer size of the patch is a bit more than Fabien's patch,
Yep, nearly twice as big and significantly more complex, but the result is
a definite improvement.
> but what it is touching is not per-statement-type code but code that
> works generically with lists of statements. So I think it's less likely
> to cause problems for other patches.
Patch applies (with "patch", "git apply" did not like it though),
compiles, overall make check ok, pgss make check ok as well. I do not know
whether the coverage of pg tests is enough to know whether anything is
broken...
I noticed that you also improved the pgss fix and tests. Good. I was
unsure about removing spaces at the end of the query because of possible
encoding issues.
The update end trick is nice to deal with empty statements. I wonder
whether the sub "query" in Copy, View, CreateAs, Explain, Prepare,
Execute, Declare... could be typed RawStmt* instead of Node*. That would
avoid some casts in updateRawStmtEnd, but maybe add some other elsewhere.
One point bothered me a bit when looking at the initial code, that your
refactoring has not changed: the location is about a string which is not
accessible from the node, so that the string must be kept elsewhere and
passed as a second argument here and there. ISTM that it would make some
sense to have the initial string directly available from RawStmt, Query
and PlannedStmt.
--
Fabien.
From | Date | Subject | |
---|---|---|---|
Next Message | Vladimir Rusinov | 2017-01-12 18:04:15 | Re: [PATCH] Rename pg_switch_xlog to pg_switch_wal |
Previous Message | Tom Lane | 2017-01-12 17:59:30 | Re: Improvements in psql hooks for variables |