Re: proposal: better support for debugging of overloaded functions

Hello

I am missing a some unique identifier in exception info. I would to
use it in combination with \sf statement

I have a log

WARNING: NP_CPS: a cannot to build a RSLT object
DETAIL: dsql_text: SELECT * FROM
public._npacceptflatfile(order_nr:=to_number('O00000032',
'O99999999')::int,sequence_nr:=1,ref_sequence_nr:=2,recipient_op:=201,losing_op:=303)
message: cannot to identify real type for record type variable
CONTEXT: PL/pgSQL function "assign_rslts" line 50 at assignment
SQL statement "SELECT workflow.assign_rslts('2011-12-18',
'09:30:30',
to_operator := 201,
from_operator := 303)"
PL/pgSQL function "inline_code_block" line 855 at PERFORM

and I would to look on "assign_rslts" function, but
ohs=# \sf workflow.assign_rslts
ERROR: more than one function named "workflow.assign_rslts"

and I have to find a parameters and manually build a parameters list.
My proposal is enhancing l CONTEXT line about function's oid and
possibility to use this oid in \sf and \df function

some like

CONTEXT: PL/pgSQL function "assign_rslts" line 50 at assignment (oid: 65903)

...

\sf+ 65903

This simple feature can reduce a time that is necessary to identify a
bug in overloaded functions.

Other possibility is just enhancing context line to be more friendly
to \sf statement

CONTEXT: PL/pgSQL function workflow.assign_rslts(date,time without
time zone,operatorid_type,operatorid_type)"" line 50 at assignment

But this is not too readable and it implementation is harder, because
in exception time is not access to system tables - so this string
should be cached somewhere.

Notes, ideas??

Regards

Pavel Stehule

On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> CONTEXT:  PL/pgSQL function "assign_rslts" line 50 at assignment (oid: 65903)
>
> \sf+ 65903

I'm pretty unenthused by the idea of making OIDs more user-visible
than they already are. If the message is ambiguous, we should include
argument types and (if not the object that would be visible under the
current search_path) a schema qualification. Spitting out a five (or
six or seven or eight) digit number doesn't seem like a usability
improvement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/11/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> CONTEXT:  PL/pgSQL function "assign_rslts" line 50 at assignment (oid: 65903)
>>
>> \sf+ 65903
>
> I'm pretty unenthused by the idea of making OIDs more user-visible
> than they already are.  If the message is ambiguous, we should include
> argument types and (if not the object that would be visible under the
> current search_path) a schema qualification.  Spitting out a five (or
> six or seven or eight) digit number doesn't seem like a usability
> improvement.
>

yes - it's not nice - but it is simple and robust and doesn't depend
on actual search_path setting.

Nicer solution is a function signature - it can be assembled when
function is compiled. I see only one disadvantage - signature can be
too wide and can depend on search_path (and search_path can be
different when function is executed and when someone run sql console).
Signature should be prepared before execution, because there are no
access to system tables after exception.

I like any solution, because debugging of overloaded function is terrible now.

Regards

Pavel

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

2011/11/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> CONTEXT:  PL/pgSQL function "assign_rslts" line 50 at assignment (oid: 65903)
>>
>> \sf+ 65903
>
> I'm pretty unenthused by the idea of making OIDs more user-visible
> than they already are.  If the message is ambiguous, we should include
> argument types and (if not the object that would be visible under the
> current search_path) a schema qualification.  Spitting out a five (or
> six or seven or eight) digit number doesn't seem like a usability
> improvement.

Is possible to add GUC variable plpgsql.log_function_signature (maybe
just log_function_signature (for all PL))? I am not sure about GUC
name.

When this variable is true, then CONTEXT line will contain a qualified
function's signature instead function name

I don't would to check if function name is ambiguous or not after
exception is raised. There is a problem with access to system tables
and then exception handling can be slower. Using a qualified name is
necessary, because psql meta statements are not "smart" - they are
based on search_path and fact, so name is not ambiguous doesn't help
there.

Regards

Pavel Stehule

p.s. Other issue is missing CONTEXT line for RAISE EXCEPTION

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Sun, Nov 20, 2011 at 6:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Is possible to add GUC variable plpgsql.log_function_signature (maybe
> just log_function_signature (for all PL))? I am not sure about GUC
> name.
>
> When this variable is true, then CONTEXT line will contain a qualified
> function's signature instead function name

Sure, but why? If it's possible to do that, I think we should just do
it always. It might be a net reduction in readability for people who
don't use overloading but do have functions with very long names and
lots and lots of arguments, but even if you think that's good design,
I think the general principle that an error message should uniquely
identify the object responsible for the error ought to take
precedence.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello

2011/11/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Nov 20, 2011 at 6:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Is possible to add GUC variable plpgsql.log_function_signature (maybe
>> just log_function_signature (for all PL))? I am not sure about GUC
>> name.
>>
>> When this variable is true, then CONTEXT line will contain a qualified
>> function's signature instead function name
>
> Sure, but why?  If it's possible to do that, I think we should just do
> it always.  It might be a net reduction in readability for people who
> don't use overloading but do have functions with very long names and
> lots and lots of arguments, but even if you think that's good design,
> I think the general principle that an error message should uniquely
> identify the object responsible for the error ought to take
> precedence.

I inclined so this is good solution

there is a VIP patch

patch is relative long, but almost all are changes in regress tests.
Changes in plpgsql are 5 lines

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

At 2011-11-24 17:44:16 +0100, pavel(dot)stehule(at)gmail(dot)com wrote:
>
> patch is relative long, but almost all are changes in regress tests.
> Changes in plpgsql are 5 lines

The change looks good in principle. The patch applies to HEAD with a bit
of fuzz and builds fine… but it fails tests, because it's incomplete.

Pavel, your patch doesn't contain any changes to pl_exec.c. Did you just
forget to submit them? Anyway, some errcontext() calls need to be taught
to print ->fn_signature rather than ->fn_name. I made those changes, and
found some more failing tests.

Updated patch attached. Ready for committer.

-- ams

Hello

2012/1/26 Abhijit Menon-Sen <ams(at)toroid(dot)org>:
> At 2011-11-24 17:44:16 +0100, pavel(dot)stehule(at)gmail(dot)com wrote:
>>
>> patch is relative long, but almost all are changes in regress tests.
>> Changes in plpgsql are 5 lines
>
> The change looks good in principle. The patch applies to HEAD with a bit
> of fuzz and builds fine… but it fails tests, because it's incomplete.
>
> Pavel, your patch doesn't contain any changes to pl_exec.c. Did you just
> forget to submit them? Anyway, some errcontext() calls need to be taught
> to print ->fn_signature rather than ->fn_name. I made those changes, and
> found some more failing tests.

It was my mistake - using fn_signature for runtime errors is good idea

>
> Updated patch attached. Ready for committer.

I found a small issue - there was uninitialized fn_signature for
online blocks so I append line

function->fn_signature = pstrdup(func_name); to
plpgsql_compile_inline(char *proc_source) function

modified patch is in attachment

Pavel

>
> -- ams

On 27.01.2012 15:48, Pavel Stehule wrote:
> 2012/1/26 Abhijit Menon-Sen<ams(at)toroid(dot)org>:
>> Updated patch attached. Ready for committer.
>
> I found a small issue - there was uninitialized fn_signature for
> online blocks so I append line
>
> function->fn_signature = pstrdup(func_name); to
> plpgsql_compile_inline(char *proc_source) function
>
> modified patch is in attachment

Thanks, committed!

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com