Horton

Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)

Peter Geoghegan <peter@2ndquadrant.com>

View thread
From: Peter Geoghegan <peter@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Daniel Farina <daniel@heroku.com>, Robert Haas <robertmhaas@gmail.com>, PG Hackers <pgsql-hackers@postgresql.org>
Date: 2012-03-25T14:34:29Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Restructure SELECT INTO's parsetree representation into CreateTableAsStmt.

  2. Extend the parser location infrastructure to include a location field in

  3. Teach eval_const_expressions() to simplify an ArrayCoerceExpr to a constant

Attachments

I've attached a patch with the required modifications. I also attach
revised tests, since naturally I have continued with test-driven
development.

On 22 March 2012 18:49, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> On 22 March 2012 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm inclined to think that the most useful behavior is to teach the
>> rewriter to copy queryId from the original query into all the Queries
>> generated by rewrite.  Then, all rules fired by a source query would
>> be lumped into that query for tracking purposes.  This might not be
>> the ideal behavior either, but I don't see a better solution.
>
> +1. This behaviour seems fairly sane. The lumping together of DO ALSO
> and DO INSTEAD operations was a simple oversight.

Implemented. We simply do this now:

*************** RewriteQuery(Query *parsetree, List *rew
*** 2141,2146 ****
--- 2142,2154 ----
  errmsg("WITH cannot be used in a query that is rewritten by rules
into multiple queries")));
  }

+ /* Mark rewritten queries with their originating queryId */
+ foreach(lc1, rewritten)
+ {
+ Query   *q = (Query *) lfirst(lc1);
+ q->queryId = orig_query_id;
+ }
+
  return rewritten;
 }

>> Either way, I think we'd be a lot better advised to define a single
>> hook "post_parse_analysis_hook" and make the core code responsible
>> for calling it at the appropriate places, rather than supposing that
>> the contrib module knows exactly which core functions ought to be
>> the places to do it.

I have done this too. The hook is called in the following places, and
some tests won't pass if any one of them is commented out:

parse_analyze
parse_analyze_varparams
pg_analyze_and_rewrite_params

I have notably *not* added anything to the following transformstmt
call site functions for various obvious reasons:

inline_function
parse_sub_analyze
transformInsertStmt
transformDeclareCursorStmt
transformExplainStmt
transformRuleStmt

I assert against pg_stat_statements fingerprinting a query twice, and
have reasonable test coverage for nested queries (both due to rules
and function execution) now. I also tweaked pg_stat_statements itself
in one or two places.

I restored the location field to the ParamCoerceHook signature, but
the removal of code to modify the param location remains (again, not
because I need it, but because I happen to think that it ought to be
consistent with Const).

Thoughts?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services