Re: Sample values for pg_stat_statements

On 03/10/2018 03:02 PM, Tomas Vondra wrote:
> Hi,
>
> I've looked at this patch today. I like the idea / intent in general, as
> it helps with some investigation tasks. That being said, I have a couple
> of questions/comments based on read through the patch:

Thanks! Attached is a patch addressing your concerns.

> 1) I see you've renamed the .sql script from 1.4 to 1.6. I thought we've
> abandoned that approach some time ago, and are now only doing the
> upgrade scripts. That is, keep 1.4 script and then 1.4--1.5 and 1.5-1.6.
> That's how other extensions are doing it now, at least - see btree_gin
> for example. But maybe pg_stat_statements has to do it this way for some
> reason, not sure?

I wrote this over a year ago. I have now changed it to conform to
modern style.

> 2) The patch should have updated doc/src/sgml/pgstatstatements.sgml

It did, but somehow missed inclusion in the patch. That's fixed, too.

> 3) Do we really need both collect_consts and collect_params? I can't
> really imagine wanting to set only one of those options. In any case,
> the names seem unnecessarily abbreviated - just use collect_constants
> and collect_parameters.

I can't see wanting parameters without constants, but I can see wanting
constants without parameters, so I think the two are justified.
Abbreviations removed.

> 4) The width_threshold GUC name seems rather weird. I mean, I wouldn't
> use "threshold" in this context, and it's really unclear size of what is
> it referring to. We do have a precedent, though, as pg_stat_activity has
> track_activity_query_size, so I suggest using either parameters_size or
> max_parameters_size (prefixed by "pg_stat_statements." of course).

Fixed.

> 5) I don't quite see why keeping the first set of parameter values we
> happen to see would be particularly useful. For example, I'm way more
> interested in values for the longest execution - why not to keep those?

For one, it's much harder to keep those because by the time you know how
long it took, you've lost the values and would have to re-jumble. At
least, that's the impression I got while writing the patch. If I am
mistaken, I will gladly look at it more.

I think having a random example is still quite valuable, and the longest
version is quite likely in the logs, too.

> 6) I suggest to use the same naming style as the existing functions, so
> for example CollectParams should be pgss_CollectParams (and it's missing
> a comment too).

Fixed.

> 7) There are a couple of places where the code style violates project
> rules, e.g. by placing {} around a single command in if-statement.

I'm pretty sure there are places where we do that to distinguish the if
code from the if condition but I couldn't readily find one. I do find
examples of using braces when there is also a comment in there, so I
just moved the comment.

> 8) I see Andres mentioned possible privacy issues (not quite sure what
> is 'data minimalism', mentioned by Andres). I'm not sure it's a problem,
> considering it can be disabled and it's subject to the usual role check
> (susperuser/role_read_all_stats). Unfortunately we can't use the same
> approach as pg_stat_activity (only showing data for user's own queries).
> Well, we could keep per-user samples, but that might considerably
> inflate the file size.

I think I handle that well enough with permission checking, but I'm open
to more debate on it.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support