From: | Marko Tiikkaja <marko(at)joh(dot)to> |
---|---|
To: | Ian Lawrence Barwick <barwick(at)gmail(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: plpgsql.print_strict_params |
Date: | 2013-09-17 20:15:09 |
Message-ID: | 5238B84D.30703@joh.to |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
>> 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.
>> 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 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.
>> * 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.
I also added the new keywords to the unreserved_keywords production, as
per the instructions near the beginning of pl_gram.y.
Regards,
Marko Tiikkaja
Attachment | Content-Type | Size |
---|---|---|
print_strict_params_v3.patch | text/plain | 20.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2013-09-17 20:24:34 | Re: [RFC] Extend namespace of valid guc names |
Previous Message | Robert Haas | 2013-09-17 20:09:38 | Re: patch: add MAP_HUGETLB to mmap() where supported (WIP) |