Re: [HACKERS] proposal: better support for debugging of overloaded functions
On 27.01.2012 15:48, Pavel Stehule wrote: 2012/1/26 Abhijit Menon-Sen: 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: better support for debugging of overloaded functions
At 2011-11-24 17:44:16 +0100, pavel.steh...@gmail.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 diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index 6ba1e5e..eee6f6c 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -342,6 +342,7 @@ do_compile(FunctionCallInfo fcinfo, compile_tmp_cxt = MemoryContextSwitchTo(func_cxt); function->fn_name = pstrdup(NameStr(procStruct->proname)); + function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid); function->fn_oid = fcinfo->flinfo->fn_oid; function->fn_xmin = HeapTupleHeaderGetXmin(procTup->t_data); function->fn_tid = procTup->t_self; diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 5ce8d6e..57e337e 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -799,7 +799,7 @@ plpgsql_exec_error_callback(void *arg) * local variable initialization" */ errcontext("PL/pgSQL function \"%s\" line %d %s", - estate->func->fn_name, + estate->func->fn_signature, estate->err_stmt->lineno, _(estate->err_text)); } @@ -810,7 +810,7 @@ plpgsql_exec_error_callback(void *arg) * arguments into local variables" */ errcontext("PL/pgSQL function \"%s\" %s", - estate->func->fn_name, + estate->func->fn_signature, _(estate->err_text)); } } @@ -818,13 +818,13 @@ plpgsql_exec_error_callback(void *arg) { /* translator: last %s is a plpgsql statement type name */ errcontext("PL/pgSQL function \"%s\" line %d at %s", - estate->func->fn_name, + estate->func->fn_signature, estate->err_stmt->lineno, plpgsql_stmt_typename(estate->err_stmt)); } else errcontext("PL/pgSQL function \"%s\"", - estate->func->fn_name); + estate->func->fn_signature); } diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 0aef8dc..739b9e4 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -679,6 +679,7 @@ typedef struct PLpgSQL_func_hashkey typedef struct PLpgSQL_function {/* Complete compiled function */ char *fn_name; + char *fn_signature; Oid fn_oid; TransactionId fn_xmin; ItemPointerData fn_tid; diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out index 4f47374..4da1c53 100644 --- a/src/test/regress/expected/domain.out +++ b/src/test/regress/expected/domain.out @@ -493,7 +493,7 @@ begin end$$ language plpgsql; select doubledecrement(3); -- fail because of implicit null assignment ERROR: domain pos_int does not allow null values -CONTEXT: PL/pgSQL function "doubledecrement" line 3 during statement block local variable initialization +CONTEXT: PL/pgSQL function "doubledecrement(pos_int)" line 3 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 0; begin @@ -501,7 +501,7 @@ begin end$$ language plpgsql; select doubledecrement(3); -- fail at initialization assignment ERROR: value for domain pos_int violates check constraint "pos_int_check" -CONTEXT: PL/pgSQL function "doubledecrement" line 3 during statement block local variable initialization +CONTEXT: PL/pgSQL function "doubledecrement(pos_int)" line 3 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 1; begin @@ -514,10 +514,10 @@ select doubledecrement(0); -- fail before call ERROR: value for domain pos_int violates check constraint "pos_int_check" select doubledecrement(1); -- fail at assignment to v ERROR: value for domain pos_int violates check constraint "pos_int_check" -CONTEXT: PL/pgSQL function "doubledecrement" line 4 at assignment +CONTEXT: PL/pgSQL function "doubledecrement(pos_int)" line 4 at assignment select doubledecrement(2); -- fail at return ERROR: value for domain pos_int violates check constraint "pos_int_check" -CONTEXT: PL/pgSQL function "doubledecrement" while casting return value to function's return type +CONTEXT: PL/pgSQL function "doubledecrement(pos_int)" while casting return value to function's return type select doubledecrement(3); -- good doubledecrement - @@ -566,7 +566,7 @@ end$$ language plpgsql; select array_elem_check(121.00); ERROR: numeric field overflow DETAIL: A field with precision 4,
Re: [HACKERS] proposal: better support for debugging of overloaded functions
On Sun, Nov 20, 2011 at 6:16 AM, Pavel Stehule 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: better support for debugging of overloaded functions
2011/11/18 Robert Haas : > On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule > 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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: better support for debugging of overloaded functions
2011/11/18 Robert Haas : > On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule > 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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: better support for debugging of overloaded functions
On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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('O0032', 'O')::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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers