Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Mmm.. At Sat, 28 Jan 2017 11:52:20 +0800, Craig Ringer wrote in > On 28 Jan. 2017 02:08, "Tom Lane" wrote: > > Kyotaro HORIGUCHI writes: > >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 28 Jan. 2017 02:08, "Tom Lane" wrote: Kyotaro HORIGUCHI writes: > By the way the existing comment for the hook, >> * >> * We provide a function hook variable that lets loadable plugins get >> * control when ProcessUtility is called. Such

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Kyotaro HORIGUCHI writes: > By the way the existing comment for the hook, >> * >> * We provide a function hook variable that lets loadable plugins get >> * control when ProcessUtility is called. Such a plugin would normally >> * call standard_ProcessUtility().

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Craig Ringer writes: > On 27 Jan. 2017 14:34, "Tom Lane" wrote: >> "The same queryString may be passed to multiple invocations of >> ProcessUtility when processing a query string containing multiple >> semicolon-separated statements; one should

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 27 Jan. 2017 14:34, "Tom Lane" wrote: Craig Ringer writes: > So perhaps: > "The same query string may be passed to multiple invocations of > ProcessUtility if a utility statement invokes subcommands (e.g. ALTER > TABLE), in which case

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

At Thu, 26 Jan 2017 22:34:57 -0500, Tom Lane wrote in <23778.1485488...@sss.pgh.pa.us> > Craig Ringer writes: > > So perhaps: > > > "The same query string may be passed to multiple invocations of > > ProcessUtility if a utility statement

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Craig Ringer writes: > So perhaps: > "The same query string may be passed to multiple invocations of > ProcessUtility if a utility statement invokes subcommands (e.g. ALTER > TABLE), in which case context will be set to > PROCESS_UTILITY_SUBCOMMAND, or if the user

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 26 January 2017 at 21:42, Tom Lane wrote: > Craig Ringer writes: >> One suggestion: it's currently non-obvious that ProcessUtility_hook >> gets called with the full text of all parts of a multi-statement. > > OK, we can improve that ... > >>

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Craig Ringer writes: > One suggestion: it's currently non-obvious that ProcessUtility_hook > gets called with the full text of all parts of a multi-statement. OK, we can improve that ... > The same query string may be passed to multiple invocations of >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 15 January 2017 at 05:18, Tom Lane wrote: > Fabien COELHO writes: >>> It ends up being about 30 fewer lines of code overall, despite there >>> being four places that have to make ad-hoc RawStmt nodes. On the whole >>> I feel like this is cleaner, > >>

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO writes: >> It ends up being about 30 fewer lines of code overall, despite there >> being four places that have to make ad-hoc RawStmt nodes. On the whole >> I feel like this is cleaner, > I agree: Better typing, more homogeneous code (PlannedStmt for all),

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Here's a v2 that does it like that. Applies, compiles, overall & pgss make check are both ok. Personnally I find having RawStmt only for the top parsed statement cleaner, and it significantly reduces changes in the parser, as expected, without inducing too much changes elsewhere. Same

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO writes: >> One thing that I'm not quite satisfied with is the business with >> non-top-level RawStmt nodes in some utility statements. >> ... >> So I'm now thinking that it might be better if the grammar produced >> RawStmt only at top level, and anybody who

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

One thing that I'm not quite satisfied with is the business with non-top-level RawStmt nodes in some utility statements. a wart from gram.y's perspective, and it's mostly a wart from analyze.c's perspective as well --- the parse analyze routines mostly just throw away the non-top-level

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Craig Ringer writes: > Sounds like it'd be better as a separate change so as not to block this one. Yeah, if we do that at all it should be a separate patch IMO. The concerns are largely different. > I really like what you have done Tom, though I'm about to travel

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 13 Jan. 2017 16:35, "Kyotaro HORIGUCHI" wrote: Hello, At Thu, 12 Jan 2017 20:08:54 +0100 (CET), Fabien COELHO wrote in > > About having a pointer to the initial string from RawStmt, Query & >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Kyotaro HORIGUCHI writes: > I found an inconsistency (in style, not function:) between > copyfuncs and equalfuncs. The patch handles the three members > utilityStmt, stmt_location and stmt_len of Query at once and > copyfuncs seems to be edited to follow the rule,

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello, It is big, but I think quite reasonable. The refactoring makes many things clearer and I like it. No objection for the patch. The affect of changing some API's are not a problem. At Thu, 12 Jan 2017 19:00:47 +0100 (CET), Fabien COELHO wrote in

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello, At Thu, 12 Jan 2017 20:08:54 +0100 (CET), Fabien COELHO wrote in > > About having a pointer to the initial string from RawStmt, Query & > PlannedStmt: > > > I remembered one reason why we haven't done this: it's unclear

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

About having a pointer to the initial string from RawStmt, Query & PlannedStmt: I remembered one reason why we haven't done this: it's unclear how we'd handle copying if we do it. If, say, Query contains a "char *" pointer then you'd expect copyObject() to pstrdup that string, [..., So]

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

[...] it seems a lot cleaner to me. I agree. [...] It's safe in backend-legal encodings. Good:-) [...] The trouble for statements like EXPLAIN is that we replace the sub-statement during parse analysis, [...] Ok. So Node* is fine to keep it generic. -- Fabien. -- Sent via

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

I wrote: > Fabien COELHO writes: >> 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

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO writes: >> [...] 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

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

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

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Kyotaro HORIGUCHI writes: > At Fri, 30 Dec 2016 15:10:42 +0100 (CET), Fabien COELHO > wrote in >> - I cannot use the intermediate node trick suggested by Tom because >> it does not work for

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello, At Fri, 30 Dec 2016 15:10:42 +0100 (CET), Fabien COELHO wrote in > > Hello, > > >> Yeah, that's what I was thinking of. There aren't very many places > >> that > >> would need to know about that, I believe; [...] > > >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Craig, [...] It's touching every single utility statement type, which is not only pretty invasive in itself, but will break any pending patches that add more utility statement types. Yep, but it is limited to headers and the break is trivial... I'm not sure how else that part can be

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 28 December 2016 at 20:11, Fabien COELHO wrote: > > Hello Tom, > >> [...] It's touching every single utility statement type, which is not only >> pretty invasive in itself, but will break any pending patches that add more >> utility statement types. > > > Yep, but it is

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Tom, [...] It's touching every single utility statement type, which is not only pretty invasive in itself, but will break any pending patches that add more utility statement types. Yep, but it is limited to headers and the break is trivial... And you've not followed through on the

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 28 December 2016 at 08:33, Tom Lane wrote: >> I would not say that the current patch is giant & invasive, if you >> abstract the two added fields to high-level statements. > > It's touching every single utility statement type, which is not only > pretty invasive in itself,

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO writes: >>> How? The issue is that stmtmulti silently skip some ';' when empty >>> statements are found, [...] >> Maybe you should undo that. > I was trying to be minimally invasive in the parser, i.e. not to change > any rules. That's fairly silly when the

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Tom, How? The issue is that stmtmulti silently skip some ';' when empty statements are found, [...] Maybe you should undo that. I was trying to be minimally invasive in the parser, i.e. not to change any rules. I've generally found that trying to put optimizations into the grammar

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO writes: > How? The issue is that stmtmulti silently skip some ';' when empty > statements are found, so I need to keep track of that to know where to > stop, using the beginning location of the next statement, which is > probably your idea, does not work.

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

An additional comment on parser(planner?) part. This patch make planner() copy the location and length from parse to result, but copying such stuffs is a job of standard_planner. I put the copy in planner because standard_planner may not be called by planner, and in all cases I think

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Kyotaro-san, In nonterminal stmtmulti, setQueryLocation is added and used to calcualte and store the length of every stmt's. This needs an extra storage in bse_yy_extra_type The extra storage is one int. and introduces a bit complicated stuff. But I think raw_parser can do that

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello, At Tue, 27 Dec 2016 10:28:53 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI wrote in <20161227.102853.204155513.horiguchi.kyot...@lab.ntt.co.jp> > Putting the two above together, the following is my suggestion > for the parser part. > > - Make all parse

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello, At Mon, 26 Dec 2016 16:00:57 +0100 (CET), Fabien COELHO wrote in > > Hello Craig, > > > Please put me (ringerc) down as a reviewer. > > Done. > > Please do not loose time reviewing stupid version 1... skip to version >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Craig, Please put me (ringerc) down as a reviewer. Done. Please do not loose time reviewing stupid version 1... skip to version 2 directly:-) Also, note that the query-location part may be considered separately from the pg_stat_statements part which uses it. -- Fabien. -- Sent

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 23 Dec. 2016 17:53, "Fabien COELHO" wrote: Yes. I'll try to put together a patch and submit it to the next CF. > Here it is. I'll add this to the next CF. Great. Please put me (ringerc) down as a reviewer. I'll get to this as soon as holidays settle down. It'd be very

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On 21 Dec. 2016 11:44, "Robert Haas" wrote: On Tue, Dec 20, 2016 at 6:18 AM, Fabien COELHO wrote: > Would this approach be acceptable, or is modifying Nodes a no-go area? > > If it is acceptable, I can probably put together a patch and submit it. > >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Yes. I'll try to put together a patch and submit it to the next CF. Here it is. I'll add this to the next CF. Oops... better without a stupid overflow. Shame on me! -- Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Yes. I'll try to put together a patch and submit it to the next CF. Here it is. I'll add this to the next CF. This patch fixes the handling of compound/combined queries by pg_stat_statements (i.e. several queries sent together, eg with psql: "SELECT 1 \; SELECT 2;"). This bug was found

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Kyotaro-san, [...] Agreed that copying statement string would be too much. But the new *Stmt node should somehow have also the end location of the statement since the user of a parse tree cannot look into another one. Yes. I was thinking of adding a "length" field next to "location",

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

At Wed, 21 Dec 2016 09:28:58 +0100 (CET), Fabien COELHO wrote in > > Hello Robert & Kyotaro, > > >> I'm a little doubtful about your proposed fix. However, I haven't > >> studied the code, so I don't know what other approach

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello Robert & Kyotaro, I'm a little doubtful about your proposed fix. However, I haven't studied the code, so I don't know what other approach might be better. That is indeed the question! The key point is that the parser parses several statements from a string, but currently there is no

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

At Tue, 20 Dec 2016 22:42:48 -0500, Robert Haas wrote in > On Tue, Dec 20, 2016 at 6:18 AM, Fabien COELHO wrote: > > Would this approach be acceptable, or is modifying Nodes a

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

On Tue, Dec 20, 2016 at 6:18 AM, Fabien COELHO wrote: > Would this approach be acceptable, or is modifying Nodes a no-go area? > > If it is acceptable, I can probably put together a patch and submit it. > > If not, I suggest to update the documentation to tell that >

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

Hello again pgdevs, More investigations conclude that the information is lost by the parser. From a ;-separated string raw_parser returns a List which are directly the nodes of the statements, with empty statements silently skipped. The Node entry of high level statements (*Stmt) does NOT

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

I was expecting the 2 combined queries either to be separated in individual queries "SELECT data FROM Stuff WHERE id = ?" or in one large queries with three ?, but not the above result... Oops, I forgot the attachement, see repeat script on 9.6.1 After some quick investigation, I concluded

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

I was expecting the 2 combined queries either to be separated in individual queries "SELECT data FROM Stuff WHERE id = ?" or in one large queries with three ?, but not the above result... Oops, I forgot the attachement, see repeat script on 9.6.1 -- Fabien. combined_statement_issue.sql