Re: plpgsql.print_strict_params

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

In response to

Responses

Browse pgsql-hackers by date

  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)