On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote:
I'm taking a look at this patch as part of the current commitfest [*],
Thanks!
However the sample function provided in the documentation throws a
runtime error due to a missing FROM-clause entry.
Ugh. I'll look into fixing that.
* Does it follow SQL spec, or the community-agreed behavior?
SQL spec: n/a. I do note that it makes use of the "#" syntax
before the body of a PL/pgSQL function, which is currently only
used for "#variable_conflict" [*]. I can imagine this syntax might
be used for other purposes down the road, so it might be worth
keeping an eye on it before it becomes a hodge-podge of ad-hoc
options.
Agreed. I have two patches like this on the commitfest and one more
cooking, so if more than one of these make it into PG, we should
probably discuss how the general mechanism should work and look like in
the future.
* Have all the bases been covered?
This lineis in "exec_get_query_params()" and "exec_get_dynquery_params()":
return "(no parameters)";
Presumably the message will escape translation and this line should be better
written as:
return _("(no parameters)");
Nice catch. Will look into this. Another option would be to simply
omit the DETAIL line if there were no parameters. At this very moment
I'm thinking that might be a bit nicer.
Also, if the offending query parameter contains a single quote, the output
is not escaped:
postgres=# select get_userid(E'foo''');
ERROR: query returned more than one row
DETAIL: p1 = 'foo''
CONTEXT: PL/pgSQL function get_userid(text) line 9 at SQL statement
Not sure if that's a real problem though.
Hmm.. I should probably look at what happens when the parameters to a
prepared statement are currently logged and imitate that.
* Does it follow the project coding guidelines?
Yes.
The functions added in pl_exec.c - "exec_get_query_params()" and
"exec_get_dynquery_params()" do strike me as potentially misnamed,
as they don't actually execute anything but are more utility
functions for generating formatted output.
Maybe "format_query_params()" etc. would be better?
Agreed, they could use some renaming.
* Are the comments sufficient and accurate?
"exec_get_query_params()" and "exec_get_dynquery_params()"
could do with some brief comments explaining their purpose.
Agreed.
(It's the first time I've "formally" reviewed a patch for a commitfest
so please let me know if I'm missing something.)
I personally think you did an excellent job. Huge thanks so far!
Regards,
Marko Tiikkaja
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
