Hi
Sorry for the delay on following up on this.
2013/9/18 Marko Tiikkaja <ma...@joh.to>:
> Hi,
>
> Attached is a patch with the following changes:
>
> On 16/09/2013 10:57, I wrote:
>>
>> On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote:
>>>
>>> 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.
>
> Fixed.
Confirmed :)
>>> 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.
>
> Decided to remove the DETAIL line in these cases.
Yes, on reflection I think that makes most sense.
>>> 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.
>
> Yup, they're escaped. Did just that. Also copied the "parameters: " prefix
> on the DETAIL line from there.
The output looks like this now:
postgres=# select get_userid(E'foo''');
ERROR: query returned more than one row
DETAIL: parameters: $1 = 'foo'''
CONTEXT: PL/pgSQL function get_userid(text) line 6 at SQL statement
which looks fine to me.
>>> 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.
>
> I went with format_expr_params and format_preparedparamsdata.
Thanks. "format_preparedparamsdata" is a bit hard to scan, but that's
just nitpicking on my part ;)
>>> * 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.
>
> Still agreeing with both of us, added a comment each.
Thanks.
> I also added the new keywords to the unreserved_keywords production, as per
> the instructions near the beginning of pl_gram.y.
Good catch.
The patch looks good to me now; does the status need to be changed to
"Ready for Committer"?
Regards
Ian Barwick
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
