Re: [HACKERS] proposal: better support for debugging of overloaded functions
On 27.01.2012 15:48, Pavel Stehule wrote: 2012/1/26 Abhijit Menon-Sena...@toroid.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 -- 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, scale 2 must round to an absolute value less than 10^2.
-CONTEXT: PL/pgSQL
Re: [HACKERS] proposal: better support for debugging of overloaded functions
On Sun, Nov 20, 2011 at 6:16 AM, Pavel Stehule pavel.steh...@gmail.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 -- 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 robertmh...@gmail.com: On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule pavel.steh...@gmail.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 -- 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
Re: [HACKERS] proposal: better support for debugging of overloaded functions
On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule pavel.steh...@gmail.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 -- 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 robertmh...@gmail.com: On Fri, Nov 18, 2011 at 6:24 AM, Pavel Stehule pavel.steh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
