Re: Sample values for pg_stat_statements

Hi Vik,

this is my review of your patch. I hope I've ticked all the necessary
boxes.

Submission review:
Patch has context, applies cleanly, make and make check run
successfully, patch contains tests for the added functionality.
The patch doesn't seem to contain any documentation regarding the new
columns.

I think the patch could be shorter due to some not really necessary
whitespace changes, e.g. lines 414, ff. in the pgss.1.patch file.
Modifying the first test for '!entry' to '!entry || need_params' in
line 628 and adding another test for '!entry' later in the file leads
to many unneccessarily changed lines, because they are simply indented
one step further (Prominently noticeable with comments, eg. line 672-
678 and 733-739.
I'd like to see the check for 'need_params' have it's own block,
leaving the existing block largely intact.
This could happen after the original 'if(!entry)'' block, at which
point you can be sure that an entry already exists, so there is no need
for the duplicated code that stores params and consts in the query text
file and their references in the entry.

Usability review:
The patch fulfills it's goal. The new columns consist of arrays of text
as well as an array of regtypes. Unfortunately, this makes the
pg_stat_statements view even more cluttered and confusing. (The view
was very cluttered before your patch, the best solution is probably to
not 'SELECT * FROM pg_stat_statements;'...)

Regarding the security implications that I can think of, this patch
behaves in similar fashion to the rest of pg_stat_statements, showing
the consts, params, and param_types only to users with proper access
rights and if the showtext flag is set.

Feature test:
The feature works as advertised and does not seem to lead to any assert
failures or memory management errors.
Manual testing indicates that data is properly persisted through
database shutdowns and restarts.

If the intended purpose is to have some basic idea of the kinds of
values that are used with certain statements, I'd like to suggest that
you take a look at my own patch, which implements the tracking of good
and bad plans in pg_stat_statements, in the current commitfest. My
approach not only shows the values that where used when the statement
was executed for the first time (regarding the lifetime of the
pg_stat_statements tracked data), but it shows values of possibly more
current executions of the statements and offers the possibility to
identify values leading to very good or very poor performance.

Maybe we can combine our efforts; Here is one idea that came to my
mind:
- Store the parameters of a statement if the execution led to a new
slower or faster plan.
- Provide a function that allows users to take the jumbled query
expression and have the database explain it, based on the parameters
that were recorded previously.

Kind regards
Julian Markwort