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