Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joe Conway]

On 09/05/2015 09:14 AM, Joe Conway wrote:
> On 09/05/2015 09:05 AM, Tom Lane wrote:
>> I wrote:
>>> If there are not major objections, I'll work on cleaning up and
>>> committing the patch.
>>
>> Pushed.  I'm not too sure about the expected outputs for python other
>> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
>> feedback.
> 
> We don't have the buildfarm actually checking sepgsql yet, but I'll
> check it out manually today or tomorrow.


One-liner required for sepgsql -- attached committed and pushed.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/sepgsql/expected/label.out b/contrib/sepgsql/expected/label.out
index c84aef7..7af5189 100644
--- a/contrib/sepgsql/expected/label.out
+++ b/contrib/sepgsql/expected/label.out
@@ -156,6 +156,7 @@ LOG:  SELinux: allowed { execute } scontext=unconfined_u:unconfined_r:sepgsql_re
 LOG:  SELinux: allowed { entrypoint } scontext=unconfined_u:unconfined_r:sepgsql_regtest_user_t:s0 tcontext=system_u:object_r:sepgsql_trusted_proc_exec_t:s0 tclass=db_procedure name="function f3()"
 LOG:  SELinux: allowed { transition } scontext=unconfined_u:unconfined_r:sepgsql_regtest_user_t:s0 tcontext=unconfined_u:unconfined_r:sepgsql_trusted_proc_t:s0 tclass=process
 ERROR:  an exception from f3()
+CONTEXT:  PL/pgSQL function f3() line 2 at RAISE
 SELECT f4();			-- failed on domain transition
 LOG:  SELinux: allowed { execute } scontext=unconfined_u:unconfined_r:sepgsql_regtest_user_t:s0 tcontext=system_u:object_r:sepgsql_nosuch_trusted_proc_exec_t:s0 tclass=db_procedure name="public.f4()"
 LOG:  SELinux: allowed { entrypoint } scontext=unconfined_u:unconfined_r:sepgsql_regtest_user_t:s0 tcontext=system_u:object_r:sepgsql_nosuch_trusted_proc_exec_t:s0 tclass=db_procedure name="function f4()"


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Tom Lane]

Joe Conway  writes:
>> On 09/05/2015 09:05 AM, Tom Lane wrote:
>>> Pushed.  I'm not too sure about the expected outputs for python other
>>> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
>>> feedback.

> One-liner required for sepgsql -- attached committed and pushed.

Thanks for checking!

regards, tom lane


-- 
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] PL/pgSQL, RAISE and error context

[Tom Lane]

I wrote:
> If there are not major objections, I'll work on cleaning up and
> committing the patch.

Pushed.  I'm not too sure about the expected outputs for python other
than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
feedback.

BTW, I noticed that the PageOutput line counts for psql's usage(),
slashUsage(), and helpVariables() were all three wrong, which I'm afraid
has been their usual state in the past too.  Since commit 07c8651dd91d5aea
there's been a pretty easy way to check them, which I added comments
about; but I don't hold much hope that that will fix anything.  I wonder
whether there's some way to not need to maintain those counts manually.

regards, tom lane


-- 
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] PL/pgSQL, RAISE and error context

[Joe Conway]

On 09/05/2015 09:05 AM, Tom Lane wrote:
> I wrote:
>> If there are not major objections, I'll work on cleaning up and
>> committing the patch.
> 
> Pushed.  I'm not too sure about the expected outputs for python other
> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
> feedback.

We don't have the buildfarm actually checking sepgsql yet, but I'll
check it out manually today or tomorrow.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

> Pushed.  I'm not too sure about the expected outputs for python other
> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
> feedback.
>
>
Thank you very much

Pavel

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Tom Lane]

Pavel Stehule  writes:
> 2015-08-12 11:07 GMT+02:00 Marko Tiikkaja :
>> I'm somewhat worried that this is hiding important context from some
>> NOTICE or WARNING messages intended for novice users, but probably not
>> worried enough to go through all of them.  +3/8 from me, I guess.

> I fixed mentioned issues.

Okay, so to summarize where we seem to have ended up:

1. Remove the wart in plpgsql that causes it to suppress the innermost
CONTEXT line for RAISE.  (I think pretty much everyone agrees that this
*is* a wart.  The question is how to get rid of it without a decrease
in usability for simple cases.)

2. Change psql so that by default, it hides the entire CONTEXT output
for messages that are of less than ERROR severity.  Add a new magic
\set variable that allows choosing this behavior, or display CONTEXT
always, or display CONTEXT never.

3. Since psql actually uses libpq for formatting server messages,
add an API to libpq that implements these CONTEXT hide/show options.

The actual code changes are pretty small, but there's rather a large
change in regression test outputs; which is unsurprising, because this
heuristic for what's of interest is entirely different from the old one.

Is everyone satisfied with this solution?  It's okay with me, though
I'm concerned that there will be complaints about loss of backwards
compatibility.  (It's hard to see how the contents of CONTEXT error
fields would be a big application compatibility issue, but you never
know.)

If there are not major objections, I'll work on cleaning up and
committing the patch.  There is still work needed (eg, the API addition
of point 3 is undocumented), but the main question is just whether we
are happy with making things work this way.

regards, tom lane


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

2015-08-12 11:07 GMT+02:00 Marko Tiikkaja ma...@joh.to:

 On 8/12/15 9:36 AM, Pavel Stehule wrote:

 So, there is common agreement on this version.


 There are several instances of double semicolons.  Also,
 PsqlSettings.show_context doesn't look like a boolean to me.  For
 SHOW_CONTEXT, it would be good if the documentation mentioned the default
 value.

 I'm somewhat worried that this is hiding important context from some
 NOTICE or WARNING messages intended for novice users, but probably not
 worried enough to go through all of them.  +3/8 from me, I guess.


I fixed mentioned issues.

Regards

Pavel



 .m



libpq-context-filter-20150813-01.patch.gz
Description: GNU Zip compressed data

-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-08-10 18:43 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-08-10 9:11 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi:

 On 07/26/2015 08:34 AM, Pavel Stehule wrote:

 Hi

 here is complete patch, that introduce context filtering on client side.
 The core of this patch is trivial and small - almost all of size are
 trivial changes in regress tests - removing useless context.

 Documentation, check-world


 Looks good to me at first glance. I'll mark this as Ready for Committer.


 Is it acceptable for all?

 I have not a problem with this way.


So, there is common agreement on this version.

Best regards

Pavel



 Regards

 Pavel



 - Heikki

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/12/15 9:36 AM, Pavel Stehule wrote:

So, there is common agreement on this version.


There are several instances of double semicolons.  Also, 
PsqlSettings.show_context doesn't look like a boolean to me.  For 
SHOW_CONTEXT, it would be good if the documentation mentioned the 
default value.


I'm somewhat worried that this is hiding important context from some 
NOTICE or WARNING messages intended for novice users, but probably not 
worried enough to go through all of them.  +3/8 from me, I guess.



.m


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-08-12 11:07 GMT+02:00 Marko Tiikkaja ma...@joh.to:

 On 8/12/15 9:36 AM, Pavel Stehule wrote:

 So, there is common agreement on this version.


 There are several instances of double semicolons.  Also,
 PsqlSettings.show_context doesn't look like a boolean to me.  For
 SHOW_CONTEXT, it would be good if the documentation mentioned the default
 value.

 I'm somewhat worried that this is hiding important context from some
 NOTICE or WARNING messages intended for novice users, but probably not
 worried enough to go through all of them.  +3/8 from me, I guess.


Thank you for info

I'll fix it



 .m

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-08-10 9:11 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi:

 On 07/26/2015 08:34 AM, Pavel Stehule wrote:

 Hi

 here is complete patch, that introduce context filtering on client side.
 The core of this patch is trivial and small - almost all of size are
 trivial changes in regress tests - removing useless context.

 Documentation, check-world


 Looks good to me at first glance. I'll mark this as Ready for Committer.


Is it acceptable for all?

I have not a problem with this way.

Regards

Pavel



 - Heikki

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Heikki Linnakangas]

On 07/26/2015 08:34 AM, Pavel Stehule wrote:

Hi

here is complete patch, that introduce context filtering on client side.
The core of this patch is trivial and small - almost all of size are
trivial changes in regress tests - removing useless context.

Documentation, check-world


Looks good to me at first glance. I'll mark this as Ready for Committer.

- Heikki



--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

here is complete patch, that introduce context filtering on client side.
The core of this patch is trivial and small - almost all of size are
trivial changes in regress tests - removing useless context.

Documentation, check-world

Regards

Pavel

2015-07-26 0:42 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I am sending a next variant of filtering context patch.

 postgres=# do $$ begin raise notice 'kuku'; end $$;
 NOTICE:  kuku
 DO
 Time: 2.441 ms
 postgres=# do $$ begin raise exception 'kuku'; end $$;
 ERROR:  kuku
 CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
 Time: 0.648 ms
 postgres=# \set SHOW_CONTEXT always
 postgres=# do $$ begin raise notice 'kuku'; end $$;
 NOTICE:  kuku
 CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
 DO
 Time: 0.702 ms

 It is a variant, when I try to filter CONTEXT in libpq. There is little
 bit less granularity on libpq side than server side, but still it is enough
 - always, error, none.

 This patch is without documentation, but basic regress tests works.

 Regards

 Pavel



 2015-07-25 10:01 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-21 16:58 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/21/2015 10:38 AM, Pavel Stehule wrote:
 
  where we are with this patch? Can I do some for it?
 
 
  I still feel this approach is misguided, and we should be tweaking psql
  and/or libpq instead. I don't feel strongly though, and if some other
  committer wants to pick this up in its current form, I won't object.
 So this
  patch has reached an impasse, and if no-one else wants to pick this
 up, I'm
  going to mark this as Returned with Feedback and move on.

 That's unfortunate.  Maybe I'm missing something:

 What does a client side implementation offer that a server side
 implementation does not offer?


 I have not any problem to change the filtering to client side. Primary
 question is fix of PLpgSQL RAISE statement issue - The context field
 filtering is a necessary follow-up and trivial in both cases.

 In this case, it is acceptable for all?

 Regards

 Pavel



 merlin






libpq-context-filter-20150726-01.patch.gz
Description: GNU Zip compressed data

-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-21 16:58 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/21/2015 10:38 AM, Pavel Stehule wrote:
 
  where we are with this patch? Can I do some for it?
 
 
  I still feel this approach is misguided, and we should be tweaking psql
  and/or libpq instead. I don't feel strongly though, and if some other
  committer wants to pick this up in its current form, I won't object. So
 this
  patch has reached an impasse, and if no-one else wants to pick this up,
 I'm
  going to mark this as Returned with Feedback and move on.

 That's unfortunate.  Maybe I'm missing something:

 What does a client side implementation offer that a server side
 implementation does not offer?


I have not any problem to change the filtering to client side. Primary
question is fix of PLpgSQL RAISE statement issue - The context field
filtering is a necessary follow-up and trivial in both cases.

In this case, it is acceptable for all?

Regards

Pavel



 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

I am sending a next variant of filtering context patch.

postgres=# do $$ begin raise notice 'kuku'; end $$;
NOTICE:  kuku
DO
Time: 2.441 ms
postgres=# do $$ begin raise exception 'kuku'; end $$;
ERROR:  kuku
CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
Time: 0.648 ms
postgres=# \set SHOW_CONTEXT always
postgres=# do $$ begin raise notice 'kuku'; end $$;
NOTICE:  kuku
CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
DO
Time: 0.702 ms

It is a variant, when I try to filter CONTEXT in libpq. There is little bit
less granularity on libpq side than server side, but still it is enough -
always, error, none.

This patch is without documentation, but basic regress tests works.

Regards

Pavel



2015-07-25 10:01 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-21 16:58 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/21/2015 10:38 AM, Pavel Stehule wrote:
 
  where we are with this patch? Can I do some for it?
 
 
  I still feel this approach is misguided, and we should be tweaking psql
  and/or libpq instead. I don't feel strongly though, and if some other
  committer wants to pick this up in its current form, I won't object. So
 this
  patch has reached an impasse, and if no-one else wants to pick this up,
 I'm
  going to mark this as Returned with Feedback and move on.

 That's unfortunate.  Maybe I'm missing something:

 What does a client side implementation offer that a server side
 implementation does not offer?


 I have not any problem to change the filtering to client side. Primary
 question is fix of PLpgSQL RAISE statement issue - The context field
 filtering is a necessary follow-up and trivial in both cases.

 In this case, it is acceptable for all?

 Regards

 Pavel



 merlin



diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 6181a61..7168809
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** SyncVariables(void)
*** 2029,2034 
--- 2029,2035 
  
  	/* send stuff to it, too */
  	PQsetErrorVerbosity(pset.db, pset.verbosity);
+ 	PQsetErrorContextVisibility(pset.db, pset.show_context);
  }
  
  /*
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
new file mode 100644
index d3e3114..0bc97de
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*** helpVariables(unsigned short int pager)
*** 307,313 
  {
  	FILE	   *output;
  
! 	output = PageOutput(85, pager ? (pset.popt.topt) : NULL);
  
  	fprintf(output, _(List of specially treated variables.\n));
  
--- 307,313 
  {
  	FILE	   *output;
  
! 	output = PageOutput(86, pager ? (pset.popt.topt) : NULL);
  
  	fprintf(output, _(List of specially treated variables.\n));
  
*** helpVariables(unsigned short int pager)
*** 339,344 
--- 339,345 
  	fprintf(output, _(  PROMPT2specify the prompt used when a statement continues from a previous line\n));
  	fprintf(output, _(  PROMPT3specify the prompt used during COPY ... FROM STDIN\n));
  	fprintf(output, _(  QUIET  run quietly (same as -q option)\n));
+ 	fprintf(output, _(  SHOW_CONTEXT   when a error context will be displayed [always, error, none]\n));
  	fprintf(output, _(  SINGLELINE end of line terminates SQL command mode (same as -S option)\n));
  	fprintf(output, _(  SINGLESTEP single-step mode (same as -s option)\n));
  	fprintf(output, _(  USER   the currently connected database user\n));
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
new file mode 100644
index d34dc28..5b49059
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*** typedef struct _psqlSettings
*** 129,134 
--- 129,135 
  	const char *prompt2;
  	const char *prompt3;
  	PGVerbosity verbosity;		/* current error verbosity level */
+ 	bool		show_context;
  } PsqlSettings;
  
  extern PsqlSettings pset;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 28ba75a..534c914
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*** main(int argc, char *argv[])
*** 157,162 
--- 157,163 
  	SetVariable(pset.vars, PROMPT1, DEFAULT_PROMPT1);
  	SetVariable(pset.vars, PROMPT2, DEFAULT_PROMPT2);
  	SetVariable(pset.vars, PROMPT3, DEFAULT_PROMPT3);
+ 	SetVariable(pset.vars, SHOW_CONTEXT, error);;
  
  	parse_psql_options(argc, argv, options);
  
*** verbosity_hook(const char *newval)
*** 868,873 
--- 869,895 
  		PQsetErrorVerbosity(pset.db, pset.verbosity);
  }
  
+ static void
+ show_context_hook(const char *newval)
+ {
+ 	if (newval == NULL)
+ 		pset.show_context = PQSHOW_CONTEXT_ERROR;
+ 	else if (pg_strcasecmp(newval, always) == 0)
+ 		pset.show_context = PQSHOW_CONTEXT_ALL;
+ 	else if (pg_strcasecmp(newval, error) == 0)
+ 		pset.show_context = PQSHOW_CONTEXT_ERROR;
+ 	else if (pg_strcasecmp(newval, none) == 0)
+ 		

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas hlinn...@iki.fi wrote:
 On 07/21/2015 10:38 AM, Pavel Stehule wrote:

 where we are with this patch? Can I do some for it?


 I still feel this approach is misguided, and we should be tweaking psql
 and/or libpq instead. I don't feel strongly though, and if some other
 committer wants to pick this up in its current form, I won't object. So this
 patch has reached an impasse, and if no-one else wants to pick this up, I'm
 going to mark this as Returned with Feedback and move on.

That's unfortunate.  Maybe I'm missing something:

What does a client side implementation offer that a server side
implementation does not offer?

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Heikki Linnakangas]

On 07/21/2015 10:38 AM, Pavel Stehule wrote:

where we are with this patch? Can I do some for it?


I still feel this approach is misguided, and we should be tweaking psql 
and/or libpq instead. I don't feel strongly though, and if some other 
committer wants to pick this up in its current form, I won't object. So 
this patch has reached an impasse, and if no-one else wants to pick this 
up, I'm going to mark this as Returned with Feedback and move on.

- Heikki



--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-21 9:53 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi:

 On 07/21/2015 10:38 AM, Pavel Stehule wrote:

 where we are with this patch? Can I do some for it?


 I still feel this approach is misguided, and we should be tweaking psql
 and/or libpq instead. I don't feel strongly though, and if some other
 committer wants to pick this up in its current form, I won't object. So
 this patch has reached an impasse, and if no-one else wants to pick this
 up, I'm going to mark this as Returned with Feedback and move on.


Can we define, when we have a agreement and where not? The missing context
for RAISE EXCEPTION statement is a important issue and I would to solve it.

last patch has two parts:

1. remove plpgsql fix, that remove context for plpgsql RAISE statement - it
is working good enough for less NOTICE level, and work badly for EXCEPTION
and higher level.

2. enforce filtering of CONTEXT field on both sides (client/log)

For me, @1 is important and good solution (because there is strange
inconsistency between PLpgSQL and any other PL), @2 allows more ways - but
probably log_min_context (WARNING) is good idea too.

The advantage of context filtering on server side (client_min_message) is
one - it can be controlled by plpgsql - so I can do dynamic decision if
some NOTICE will have context or not.

The complexity will be +/- same for psql/libpq or for server side filtering.

Regards

Pavel


 - Heikki

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-09 23:16 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-09 22:57 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
 
 
  2015-07-09 20:08 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
 
  On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule 
 pavel.steh...@gmail.com
  wrote:
  
   2015-07-09 15:17 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
  
   On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
   pavel.steh...@gmail.com

   wrote:
Hi
   
second version of this patch
   
make check-world passed
  
   quickly scanning the patch, the implementation is trivial (minus
   regression test adjustments), and is, IMSNSHO, the right solution.
  
  
   yes, it is right way - the behave of RAISE statement will be much
 more
   cleaner
  
  
   Several of the source level comments need some minor wordsmithing
 and
   the GUCs are missing documentation.  If we've got consensus on the
   approach, I'll pitch in on that.
  
   thank you
 
  revised patch attached. added GUC docs and cleaned up pg_settings
  language.  Also tested patch and it works beautifully.
 
  Note, Pavel's patch does adjust default behavior to what we think is
  the right settings.
 
 
  Thank you for documentation.
 
  There is small error - default for client_min_context is error - not
 notice.
  With this level a diff from regress tests is minimal. Default for
  log_min_context should be warning.

 whoop!  thanks.   Also, I was playing a bit with the idea of making
 client_min_context superuser only setting.  The idea being this
 could be used to prevent leakage of stored procedure code in cases
 where the admins don't want it to be exposed.  I figured it was a bad
 idea though; it would frustrate debugging in reasonable cases.


 This is not designed for security usage. Probably there can be some rule
 in future - the possibility to see or don't see  a error context - OFF, ON.
 For this reason, the setting a some min level is not good way.


Hi

where we are with this patch? Can I do some for it?

Regards

Pavel



 Regards

 Pavel





 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-09 15:17 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Hi
 
  second version of this patch
 
  make check-world passed

 quickly scanning the patch, the implementation is trivial (minus
 regression test adjustments), and is, IMSNSHO, the right solution.


yes, it is right way - the behave of RAISE statement will be much more
cleaner


 Several of the source level comments need some minor wordsmithing and
 the GUCs are missing documentation.  If we've got consensus on the
 approach, I'll pitch in on that.


thank you

Pavel



 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule pavel.steh...@gmail.com wrote:

 2015-07-09 15:17 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Hi
 
  second version of this patch
 
  make check-world passed

 quickly scanning the patch, the implementation is trivial (minus
 regression test adjustments), and is, IMSNSHO, the right solution.


 yes, it is right way - the behave of RAISE statement will be much more
 cleaner


 Several of the source level comments need some minor wordsmithing and
 the GUCs are missing documentation.  If we've got consensus on the
 approach, I'll pitch in on that.

 thank you

revised patch attached. added GUC docs and cleaned up pg_settings
language.  Also tested patch and it works beautifully.

Note, Pavel's patch does adjust default behavior to what we think is
the right settings.

merlin
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
new file mode 100644
index a49b562..a268fc7
*** a/contrib/dblink/expected/dblink.out
--- b/contrib/dblink/expected/dblink.out
***
*** 1,3 
--- 1,4 
+ set client_min_context TO notice;
  CREATE EXTENSION dblink;
  CREATE TABLE foo(f1 int, f2 text, f3 text[], primary key (f1,f2));
  INSERT INTO foo VALUES (0,'a','{a0,b0,c0}');
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
new file mode 100644
index ea78cc2..cf7e57e
*** a/contrib/dblink/sql/dblink.sql
--- b/contrib/dblink/sql/dblink.sql
***
*** 1,3 
--- 1,5 
+ set client_min_context TO notice;
+ 
  CREATE EXTENSION dblink;
  
  CREATE TABLE foo(f1 int, f2 text, f3 text[], primary key (f1,f2));
diff --git a/contrib/hstore_plperl/expected/hstore_plperlu.out b/contrib/hstore_plperl/expected/hstore_plperlu.out
new file mode 100644
index 8c689ad..c97fd3f
*** a/contrib/hstore_plperl/expected/hstore_plperlu.out
--- b/contrib/hstore_plperl/expected/hstore_plperlu.out
*** INFO:  $VAR1 = {
*** 29,35 
'cc' = undef
  };
  
- CONTEXT:  PL/Perl function test1
   test1 
  ---
   2
--- 29,34 
*** $$;
*** 46,52 
  SELECT test1none('aa=bb, cc=NULL'::hstore);
  INFO:  $VAR1 = 'aa=bb, cc=NULL';
  
- CONTEXT:  PL/Perl function test1none
   test1none 
  ---
   0
--- 45,50 
*** INFO:  $VAR1 = {
*** 67,73 
'cc' = undef
  };
  
- CONTEXT:  PL/Perl function test1list
   test1list 
  ---
   2
--- 65,70 
*** $VAR2 = {
*** 92,98 
'dd' = 'ee'
  };
  
- CONTEXT:  PL/Perl function test1arr
   test1arr 
  --
  2
--- 89,94 
*** INFO:  $VAR1 = {
*** 120,129 
'cc' = undef
  };
  
- CONTEXT:  PL/Perl function test3
  INFO:  $VAR1 = 'a=1, b=boo, c=NULL';
  
- CONTEXT:  PL/Perl function test3
   test3 
  ---
   
--- 116,123 
*** INFO:  $VAR1 = {
*** 161,167 
 }
  };
  
- CONTEXT:  PL/Perl function test4
  SELECT * FROM test1;
   a |b
  ---+-
--- 155,160 
diff --git a/contrib/hstore_plpython/expected/hstore_plpython.out b/contrib/hstore_plpython/expected/hstore_plpython.out
new file mode 100644
index b7a6a92..23091d3
*** a/contrib/hstore_plpython/expected/hstore_plpython.out
--- b/contrib/hstore_plpython/expected/hstore_plpython.out
*** return len(val)
*** 13,19 
  $$;
  SELECT test1('aa=bb, cc=NULL'::hstore);
  INFO:  [('aa', 'bb'), ('cc', None)]
- CONTEXT:  PL/Python function test1
   test1 
  ---
   2
--- 13,18 
*** return len(val)
*** 32,38 
  $$;
  SELECT test1n('aa=bb, cc=NULL'::hstore);
  INFO:  [('aa', 'bb'), ('cc', None)]
- CONTEXT:  PL/Python function test1n
   test1n 
  
2
--- 31,36 
diff --git a/contrib/ltree_plpython/expected/ltree_plpython.out b/contrib/ltree_plpython/expected/ltree_plpython.out
new file mode 100644
index 934529e..c6e8a7c
*** a/contrib/ltree_plpython/expected/ltree_plpython.out
--- b/contrib/ltree_plpython/expected/ltree_plpython.out
*** return len(val)
*** 9,15 
  $$;
  SELECT test1('aa.bb.cc'::ltree);
  INFO:  ['aa', 'bb', 'cc']
- CONTEXT:  PL/Python function test1
   test1 
  ---
   3
--- 9,14 
*** return len(val)
*** 24,30 
  $$;
  SELECT test1n('aa.bb.cc'::ltree);
  INFO:  ['aa', 'bb', 'cc']
- CONTEXT:  PL/Python function test1n
   test1n 
  
3
--- 23,28 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index b91d6c7..38ae0ad
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** local0.*/var/log/postgresql
*** 4144,4149 
--- 4144,4171 
  
   variablelist
  
+  varlistentry id=guc-client-min-context xreflabel=client_min_context
+   

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
 Hi

 second version of this patch

 make check-world passed

quickly scanning the patch, the implementation is trivial (minus
regression test adjustments), and is, IMSNSHO, the right solution.

Several of the source level comments need some minor wordsmithing and
the GUCs are missing documentation.  If we've got consensus on the
approach, I'll pitch in on that.

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-09 22:57 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
 
 
  2015-07-09 20:08 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
 
  On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule pavel.steh...@gmail.com
 
  wrote:
  
   2015-07-09 15:17 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
  
   On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
   pavel.steh...@gmail.com
   wrote:
Hi
   
second version of this patch
   
make check-world passed
  
   quickly scanning the patch, the implementation is trivial (minus
   regression test adjustments), and is, IMSNSHO, the right solution.
  
  
   yes, it is right way - the behave of RAISE statement will be much more
   cleaner
  
  
   Several of the source level comments need some minor wordsmithing and
   the GUCs are missing documentation.  If we've got consensus on the
   approach, I'll pitch in on that.
  
   thank you
 
  revised patch attached. added GUC docs and cleaned up pg_settings
  language.  Also tested patch and it works beautifully.
 
  Note, Pavel's patch does adjust default behavior to what we think is
  the right settings.
 
 
  Thank you for documentation.
 
  There is small error - default for client_min_context is error - not
 notice.
  With this level a diff from regress tests is minimal. Default for
  log_min_context should be warning.

 whoop!  thanks.   Also, I was playing a bit with the idea of making
 client_min_context superuser only setting.  The idea being this
 could be used to prevent leakage of stored procedure code in cases
 where the admins don't want it to be exposed.  I figured it was a bad
 idea though; it would frustrate debugging in reasonable cases.


This is not designed for security usage. Probably there can be some rule in
future - the possibility to see or don't see  a error context - OFF, ON.
For this reason, the setting a some min level is not good way.

Regards

Pavel





 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule pavel.steh...@gmail.com wrote:


 2015-07-09 20:08 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
 
  2015-07-09 15:17 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
 
  On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
  pavel.steh...@gmail.com
  wrote:
   Hi
  
   second version of this patch
  
   make check-world passed
 
  quickly scanning the patch, the implementation is trivial (minus
  regression test adjustments), and is, IMSNSHO, the right solution.
 
 
  yes, it is right way - the behave of RAISE statement will be much more
  cleaner
 
 
  Several of the source level comments need some minor wordsmithing and
  the GUCs are missing documentation.  If we've got consensus on the
  approach, I'll pitch in on that.
 
  thank you

 revised patch attached. added GUC docs and cleaned up pg_settings
 language.  Also tested patch and it works beautifully.

 Note, Pavel's patch does adjust default behavior to what we think is
 the right settings.


 Thank you for documentation.

 There is small error - default for client_min_context is error - not notice.
 With this level a diff from regress tests is minimal. Default for
 log_min_context should be warning.

whoop!  thanks.   Also, I was playing a bit with the idea of making
client_min_context superuser only setting.  The idea being this
could be used to prevent leakage of stored procedure code in cases
where the admins don't want it to be exposed.  I figured it was a bad
idea though; it would frustrate debugging in reasonable cases.

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule pavel.steh...@gmail.com wrote:


 2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  what is RAISE EXCEPTION - first or second case?

 First: RAISE (unless caught) is no different than any other kind of error.

 On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/07/2015 04:56 PM, Merlin Moncure wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  Java's exception handling is so different from PostgreSQL's errors that
  I
  don't think there's much to be learned from that. But I'll bite:
 
  First of all, Java's exceptions always contain a stack trace. It's up to
  you
  when you catch an exception to decide whether to print it or not. try {
  ...
  } catch (Exception e) { e.printStackTrace() } is fairly common,
  actually.
  There is nothing like a NOTICE in Java, i.e. an exception that's thrown
  but
  doesn't affect the control flow. The best I can think of is
  System.out.println(), which of course has no stack trace attached to it.

 exactly.

  Perhaps you're arguing that NOTICE is more like printing to stderr, and
  should never contain any context information. I don't think that would
  be an
  improvement. It's very handy to have the context information available
  if
  don't know where a NOTICE is coming from, even if in most cases you're
  not
  interested in it.

 That's exactly what I'm arguing.  NOTICE (and WARNING) are for
 printing out information to client side logging; it's really the only
 tool we have for that purpose and it fits that role perfectly.  Of
 course, you may want to have NOTICE print context, especially when
 debugging, but some control over that would be nice and in most cases
 it's really not necessary.  I really don't understand the objection to
 offering control over that behavior although I certainly understand
 wanting to keep the default behavior as it currently is.

  This is really quite different from a programming language's exception
  handling. First, there's a server, which produces the errors, and a
  separate
  client, which displays them. You cannot catch an exception in the
  client.
 
  BTW, let me throw in one use case to consider. We've been talking about
  psql, and what to print, but imagine a more sophisticated client like
  pgAdmin. It's not limited to either printing the context or not. It
  could
  e.g. hide the context information of all messages when they occur, but
  if
  you double-click on it, it's expanded to show all the context, location
  and
  all. You can't do that if the server doesn't send the context
  information in
  the first place.
 
  I would be happy to show you the psql redirected output logs from my
  nightly server processes that spew into the megabytes because of
  logging various high level steps (did this, did that).
 
  Oh, I believe you. I understand what the problem is, we're only talking
  about how best to address it.

 Yeah.  For posterity, a psql based solution would work fine for me,
 but a server side solution has a lot of advantages (less protocol
 chatter, more configurability, keeping libpq/psql light).


 After some work on reduced version of plpgsql.min_context patch I am
 inclining to think so ideal solution needs more steps - because this issue
 has more than one dimension.

 There are two independent issues:

 1. old plpgsql workaround that reduced the unwanted call stack info for
 RAISE NOTICE. Negative side effect of this workaround is missing context
 info about the RAISE command that raises the exception. We know a function,
 but we don't know a line of related RAISE statement. The important is fact,
 so NOTICE doesn't bubble to up. So this workaround was relative successful
 without to implement some filtering on client or log side.

 2. second issue is general suppressing context info for interactive client
 or for log.

 These issues should be solved separately, because solution for @2 doesn't
 fix @1, and @1 is too local for @2.

 So what we can do?

 1. remove current plpgsql workaround - and implement client_min_context and
 log_min_context
 2. implement plpgsql.min_context, and 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Wed, Jul 8, 2015 at 4:39 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
 2015-07-08 8:35 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  what is RAISE EXCEPTION - first or second case?

 First: RAISE (unless caught) is no different than any other kind of
 error.

 On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/07/2015 04:56 PM, Merlin Moncure wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  Java's exception handling is so different from PostgreSQL's errors that
  I
  don't think there's much to be learned from that. But I'll bite:
 
  First of all, Java's exceptions always contain a stack trace. It's up
  to you
  when you catch an exception to decide whether to print it or not. try
  { ...
  } catch (Exception e) { e.printStackTrace() } is fairly common,
  actually.
  There is nothing like a NOTICE in Java, i.e. an exception that's thrown
  but
  doesn't affect the control flow. The best I can think of is
  System.out.println(), which of course has no stack trace attached to
  it.

 exactly.

  Perhaps you're arguing that NOTICE is more like printing to stderr, and
  should never contain any context information. I don't think that would
  be an
  improvement. It's very handy to have the context information available
  if
  don't know where a NOTICE is coming from, even if in most cases you're
  not
  interested in it.

 That's exactly what I'm arguing.  NOTICE (and WARNING) are for
 printing out information to client side logging; it's really the only
 tool we have for that purpose and it fits that role perfectly.  Of
 course, you may want to have NOTICE print context, especially when
 debugging, but some control over that would be nice and in most cases
 it's really not necessary.  I really don't understand the objection to
 offering control over that behavior although I certainly understand
 wanting to keep the default behavior as it currently is.

  This is really quite different from a programming language's exception
  handling. First, there's a server, which produces the errors, and a
  separate
  client, which displays them. You cannot catch an exception in the
  client.
 
  BTW, let me throw in one use case to consider. We've been talking about
  psql, and what to print, but imagine a more sophisticated client like
  pgAdmin. It's not limited to either printing the context or not. It
  could
  e.g. hide the context information of all messages when they occur, but
  if
  you double-click on it, it's expanded to show all the context, location
  and
  all. You can't do that if the server doesn't send the context
  information in
  the first place.
 
  I would be happy to show you the psql redirected output logs from my
  nightly server processes that spew into the megabytes because of
  logging various high level steps (did this, did that).
 
  Oh, I believe you. I understand what the problem is, we're only talking
  about how best to address it.

 Yeah.  For posterity, a psql based solution would work fine for me,
 but a server side solution has a lot of advantages (less protocol
 chatter, more configurability, keeping libpq/psql light).


 After some work on reduced version of plpgsql.min_context patch I am
 inclining to think so ideal solution needs more steps - because this issue
 has more than one dimension.

 There are two independent issues:

 1. old plpgsql workaround that reduced the unwanted call stack info for
 RAISE NOTICE. Negative side effect of this workaround is missing context
 info about the RAISE command that raises the exception. We know a function,
 but we don't know a line of related RAISE statement. The important is fact,
 so NOTICE doesn't bubble to up. So this workaround was relative successful
 without to implement some filtering on client or log side.


 I found a other issue of this workaround - it doesn't work well for nested
 SQL statement call, when inner statement invoke RAISE NOTICE. In this case a
 context is showed too.

 postgres=# insert into xx values(60);
 NOTICE:  
 NOTICE:  trigger_func(before_ins_stmt) called: action = INSERT, when 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-08 8:35 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  what is RAISE EXCEPTION - first or second case?

 First: RAISE (unless caught) is no different than any other kind of error.

 On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/07/2015 04:56 PM, Merlin Moncure wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  Java's exception handling is so different from PostgreSQL's errors that
 I
  don't think there's much to be learned from that. But I'll bite:
 
  First of all, Java's exceptions always contain a stack trace. It's up
 to you
  when you catch an exception to decide whether to print it or not. try
 { ...
  } catch (Exception e) { e.printStackTrace() } is fairly common,
 actually.
  There is nothing like a NOTICE in Java, i.e. an exception that's thrown
 but
  doesn't affect the control flow. The best I can think of is
  System.out.println(), which of course has no stack trace attached to it.

 exactly.

  Perhaps you're arguing that NOTICE is more like printing to stderr, and
  should never contain any context information. I don't think that would
 be an
  improvement. It's very handy to have the context information available
 if
  don't know where a NOTICE is coming from, even if in most cases you're
 not
  interested in it.

 That's exactly what I'm arguing.  NOTICE (and WARNING) are for
 printing out information to client side logging; it's really the only
 tool we have for that purpose and it fits that role perfectly.  Of
 course, you may want to have NOTICE print context, especially when
 debugging, but some control over that would be nice and in most cases
 it's really not necessary.  I really don't understand the objection to
 offering control over that behavior although I certainly understand
 wanting to keep the default behavior as it currently is.

  This is really quite different from a programming language's exception
  handling. First, there's a server, which produces the errors, and a
 separate
  client, which displays them. You cannot catch an exception in the
 client.
 
  BTW, let me throw in one use case to consider. We've been talking about
  psql, and what to print, but imagine a more sophisticated client like
  pgAdmin. It's not limited to either printing the context or not. It
 could
  e.g. hide the context information of all messages when they occur, but
 if
  you double-click on it, it's expanded to show all the context, location
 and
  all. You can't do that if the server doesn't send the context
 information in
  the first place.
 
  I would be happy to show you the psql redirected output logs from my
  nightly server processes that spew into the megabytes because of
  logging various high level steps (did this, did that).
 
  Oh, I believe you. I understand what the problem is, we're only talking
  about how best to address it.

 Yeah.  For posterity, a psql based solution would work fine for me,
 but a server side solution has a lot of advantages (less protocol
 chatter, more configurability, keeping libpq/psql light).


 After some work on reduced version of plpgsql.min_context patch I am
 inclining to think so ideal solution needs more steps - because this issue
 has more than one dimension.

 There are two independent issues:

 1. old plpgsql workaround that reduced the unwanted call stack info for
 RAISE NOTICE. Negative side effect of this workaround is missing context
 info about the RAISE command that raises the exception. We know a function,
 but we don't know a line of related RAISE statement. The important is fact,
 so NOTICE doesn't bubble to up. So this workaround was relative successful
 without to implement some filtering on client or log side.


I found a other issue of this workaround - it doesn't work well for nested
SQL statement call, when inner statement invoke RAISE NOTICE. In this case
a context is showed too.

postgres=# insert into xx values(60);
NOTICE:  
NOTICE:  trigger_func(before_ins_stmt) called: action = INSERT, when =
BEFORE, level = STATEMENT
CONTEXT:  SQL statement INSERT INTO boo VALUES(30)
PL/pgSQL function hh() 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-08 23:46 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Wed, Jul 8, 2015 at 4:39 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  2015-07-08 8:35 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:
 
 
 
  2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
 
  On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 
  wrote:
   It doesn't have to if the behavior is guarded with a GUC.  I just
   don't understand what all the fuss is about.  The default behavior
 of
   logging that is well established by other languages (for example
 java)
   that manage error stack for you should be to:
  
   *) Give stack trace when an uncaught exception is thrown
   *) Do not give stack trace in all other logging cases unless asked
 for
  
   what is RAISE EXCEPTION - first or second case?
 
  First: RAISE (unless caught) is no different than any other kind of
  error.
 
  On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
  wrote:
   On 07/07/2015 04:56 PM, Merlin Moncure wrote:
   It doesn't have to if the behavior is guarded with a GUC.  I just
   don't understand what all the fuss is about.  The default behavior
 of
   logging that is well established by other languages (for example
 java)
   that manage error stack for you should be to:
  
   *) Give stack trace when an uncaught exception is thrown
   *) Do not give stack trace in all other logging cases unless asked
 for
  
   Java's exception handling is so different from PostgreSQL's errors
 that
   I
   don't think there's much to be learned from that. But I'll bite:
  
   First of all, Java's exceptions always contain a stack trace. It's up
   to you
   when you catch an exception to decide whether to print it or not.
 try
   { ...
   } catch (Exception e) { e.printStackTrace() } is fairly common,
   actually.
   There is nothing like a NOTICE in Java, i.e. an exception that's
 thrown
   but
   doesn't affect the control flow. The best I can think of is
   System.out.println(), which of course has no stack trace attached to
   it.
 
  exactly.
 
   Perhaps you're arguing that NOTICE is more like printing to stderr,
 and
   should never contain any context information. I don't think that
 would
   be an
   improvement. It's very handy to have the context information
 available
   if
   don't know where a NOTICE is coming from, even if in most cases
 you're
   not
   interested in it.
 
  That's exactly what I'm arguing.  NOTICE (and WARNING) are for
  printing out information to client side logging; it's really the only
  tool we have for that purpose and it fits that role perfectly.  Of
  course, you may want to have NOTICE print context, especially when
  debugging, but some control over that would be nice and in most cases
  it's really not necessary.  I really don't understand the objection to
  offering control over that behavior although I certainly understand
  wanting to keep the default behavior as it currently is.
 
   This is really quite different from a programming language's
 exception
   handling. First, there's a server, which produces the errors, and a
   separate
   client, which displays them. You cannot catch an exception in the
   client.
  
   BTW, let me throw in one use case to consider. We've been talking
 about
   psql, and what to print, but imagine a more sophisticated client like
   pgAdmin. It's not limited to either printing the context or not. It
   could
   e.g. hide the context information of all messages when they occur,
 but
   if
   you double-click on it, it's expanded to show all the context,
 location
   and
   all. You can't do that if the server doesn't send the context
   information in
   the first place.
  
   I would be happy to show you the psql redirected output logs from my
   nightly server processes that spew into the megabytes because of
   logging various high level steps (did this, did that).
  
   Oh, I believe you. I understand what the problem is, we're only
 talking
   about how best to address it.
 
  Yeah.  For posterity, a psql based solution would work fine for me,
  but a server side solution has a lot of advantages (less protocol
  chatter, more configurability, keeping libpq/psql light).
 
 
  After some work on reduced version of plpgsql.min_context patch I am
  inclining to think so ideal solution needs more steps - because this
 issue
  has more than one dimension.
 
  There are two independent issues:
 
  1. old plpgsql workaround that reduced the unwanted call stack info for
  RAISE NOTICE. Negative side effect of this workaround is missing context
  info about the RAISE command that raises the exception. We know a
 function,
  but we don't know a line of related RAISE statement. The important is
 fact,
  so NOTICE doesn't bubble to up. So this workaround was relative
 successful
  without to implement some filtering on client or log side.
 
 
  I found a other issue of this workaround - it doesn't work well for
 nested
  SQL statement 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  what is RAISE EXCEPTION - first or second case?

 First: RAISE (unless caught) is no different than any other kind of error.

 On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/07/2015 04:56 PM, Merlin Moncure wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  Java's exception handling is so different from PostgreSQL's errors that I
  don't think there's much to be learned from that. But I'll bite:
 
  First of all, Java's exceptions always contain a stack trace. It's up to
 you
  when you catch an exception to decide whether to print it or not. try {
 ...
  } catch (Exception e) { e.printStackTrace() } is fairly common,
 actually.
  There is nothing like a NOTICE in Java, i.e. an exception that's thrown
 but
  doesn't affect the control flow. The best I can think of is
  System.out.println(), which of course has no stack trace attached to it.

 exactly.

  Perhaps you're arguing that NOTICE is more like printing to stderr, and
  should never contain any context information. I don't think that would
 be an
  improvement. It's very handy to have the context information available if
  don't know where a NOTICE is coming from, even if in most cases you're
 not
  interested in it.

 That's exactly what I'm arguing.  NOTICE (and WARNING) are for
 printing out information to client side logging; it's really the only
 tool we have for that purpose and it fits that role perfectly.  Of
 course, you may want to have NOTICE print context, especially when
 debugging, but some control over that would be nice and in most cases
 it's really not necessary.  I really don't understand the objection to
 offering control over that behavior although I certainly understand
 wanting to keep the default behavior as it currently is.

  This is really quite different from a programming language's exception
  handling. First, there's a server, which produces the errors, and a
 separate
  client, which displays them. You cannot catch an exception in the client.
 
  BTW, let me throw in one use case to consider. We've been talking about
  psql, and what to print, but imagine a more sophisticated client like
  pgAdmin. It's not limited to either printing the context or not. It could
  e.g. hide the context information of all messages when they occur, but if
  you double-click on it, it's expanded to show all the context, location
 and
  all. You can't do that if the server doesn't send the context
 information in
  the first place.
 
  I would be happy to show you the psql redirected output logs from my
  nightly server processes that spew into the megabytes because of
  logging various high level steps (did this, did that).
 
  Oh, I believe you. I understand what the problem is, we're only talking
  about how best to address it.

 Yeah.  For posterity, a psql based solution would work fine for me,
 but a server side solution has a lot of advantages (less protocol
 chatter, more configurability, keeping libpq/psql light).


After some work on reduced version of plpgsql.min_context patch I am
inclining to think so ideal solution needs more steps - because this issue
has more than one dimension.

There are two independent issues:

1. old plpgsql workaround that reduced the unwanted call stack info for
RAISE NOTICE. Negative side effect of this workaround is missing context
info about the RAISE command that raises the exception. We know a function,
but we don't know a line of related RAISE statement. The important is fact,
so NOTICE doesn't bubble to up. So this workaround was relative successful
without to implement some filtering on client or log side.

2. second issue is general suppressing context info for interactive client
or for log.

These issues should be solved separately, because solution for @2 doesn't
fix @1, and @1 is too local for @2.

So what we can do?

1. remove current plpgsql workaround - and implement client_min_context and
log_min_context
2. implement plpgsql.min_context, and client_min_context and log_min_context

@1 is consistent, but isn't possible to configure same behave as was before

@2 is 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

here is initial version of reduced patch. It is small code, but relative
big (although I expected bigger) change in tests.

if these changes are too big, then we have to introduce a plpgsql GUC
plpgsql.client_min_context and plpgsql.log_min_client. These GUC overwrite
global setting for plpgsql functions. I'll be more happy without these
variables. It decrease a impact of changes, but there is not clean what
behave is expected when PL are used together - and when fails PLpgSQL
function called from PLPerl. The context filtering should be really solved
on TOP level.


Regards

Pavel

2015-07-08 14:09 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
 
 
  2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:
 
  On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
  wrote:
   It doesn't have to if the behavior is guarded with a GUC.  I just
   don't understand what all the fuss is about.  The default behavior of
   logging that is well established by other languages (for example
 java)
   that manage error stack for you should be to:
  
   *) Give stack trace when an uncaught exception is thrown
   *) Do not give stack trace in all other logging cases unless asked
 for
  
   what is RAISE EXCEPTION - first or second case?
 
  First: RAISE (unless caught) is no different than any other kind of
 error.
 
  On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
  wrote:
   On 07/07/2015 04:56 PM, Merlin Moncure wrote:
   It doesn't have to if the behavior is guarded with a GUC.  I just
   don't understand what all the fuss is about.  The default behavior of
   logging that is well established by other languages (for example
 java)
   that manage error stack for you should be to:
  
   *) Give stack trace when an uncaught exception is thrown
   *) Do not give stack trace in all other logging cases unless asked
 for
  
   Java's exception handling is so different from PostgreSQL's errors
 that
   I
   don't think there's much to be learned from that. But I'll bite:
  
   First of all, Java's exceptions always contain a stack trace. It's up
 to
   you
   when you catch an exception to decide whether to print it or not.
 try {
   ...
   } catch (Exception e) { e.printStackTrace() } is fairly common,
   actually.
   There is nothing like a NOTICE in Java, i.e. an exception that's
 thrown
   but
   doesn't affect the control flow. The best I can think of is
   System.out.println(), which of course has no stack trace attached to
 it.
 
  exactly.
 
   Perhaps you're arguing that NOTICE is more like printing to stderr,
 and
   should never contain any context information. I don't think that would
   be an
   improvement. It's very handy to have the context information available
   if
   don't know where a NOTICE is coming from, even if in most cases you're
   not
   interested in it.
 
  That's exactly what I'm arguing.  NOTICE (and WARNING) are for
  printing out information to client side logging; it's really the only
  tool we have for that purpose and it fits that role perfectly.  Of
  course, you may want to have NOTICE print context, especially when
  debugging, but some control over that would be nice and in most cases
  it's really not necessary.  I really don't understand the objection to
  offering control over that behavior although I certainly understand
  wanting to keep the default behavior as it currently is.
 
   This is really quite different from a programming language's exception
   handling. First, there's a server, which produces the errors, and a
   separate
   client, which displays them. You cannot catch an exception in the
   client.
  
   BTW, let me throw in one use case to consider. We've been talking
 about
   psql, and what to print, but imagine a more sophisticated client like
   pgAdmin. It's not limited to either printing the context or not. It
   could
   e.g. hide the context information of all messages when they occur, but
   if
   you double-click on it, it's expanded to show all the context,
 location
   and
   all. You can't do that if the server doesn't send the context
   information in
   the first place.
  
   I would be happy to show you the psql redirected output logs from my
   nightly server processes that spew into the megabytes because of
   logging various high level steps (did this, did that).
  
   Oh, I believe you. I understand what the problem is, we're only
 talking
   about how best to address it.
 
  Yeah.  For posterity, a psql based solution would work fine for me,
  but a server side solution has a lot of advantages (less protocol
  chatter, more configurability, keeping libpq/psql light).
 
 
  After some work on reduced version of plpgsql.min_context patch I am
  inclining to think so ideal solution needs more steps - because this
 issue
  has more than one dimension.
 
  There are two independent issues:
 
  1. old plpgsql workaround that 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 It doesn't have to if the behavior is guarded with a GUC.  I just
 don't understand what all the fuss is about.  The default behavior of
 logging that is well established by other languages (for example java)
 that manage error stack for you should be to:

 *) Give stack trace when an uncaught exception is thrown
 *) Do not give stack trace in all other logging cases unless asked for

 what is RAISE EXCEPTION - first or second case?

First: RAISE (unless caught) is no different than any other kind of error.

On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi wrote:
 On 07/07/2015 04:56 PM, Merlin Moncure wrote:
 It doesn't have to if the behavior is guarded with a GUC.  I just
 don't understand what all the fuss is about.  The default behavior of
 logging that is well established by other languages (for example java)
 that manage error stack for you should be to:

 *) Give stack trace when an uncaught exception is thrown
 *) Do not give stack trace in all other logging cases unless asked for

 Java's exception handling is so different from PostgreSQL's errors that I
 don't think there's much to be learned from that. But I'll bite:

 First of all, Java's exceptions always contain a stack trace. It's up to you
 when you catch an exception to decide whether to print it or not. try { ...
 } catch (Exception e) { e.printStackTrace() } is fairly common, actually.
 There is nothing like a NOTICE in Java, i.e. an exception that's thrown but
 doesn't affect the control flow. The best I can think of is
 System.out.println(), which of course has no stack trace attached to it.

exactly.

 Perhaps you're arguing that NOTICE is more like printing to stderr, and
 should never contain any context information. I don't think that would be an
 improvement. It's very handy to have the context information available if
 don't know where a NOTICE is coming from, even if in most cases you're not
 interested in it.

That's exactly what I'm arguing.  NOTICE (and WARNING) are for
printing out information to client side logging; it's really the only
tool we have for that purpose and it fits that role perfectly.  Of
course, you may want to have NOTICE print context, especially when
debugging, but some control over that would be nice and in most cases
it's really not necessary.  I really don't understand the objection to
offering control over that behavior although I certainly understand
wanting to keep the default behavior as it currently is.

 This is really quite different from a programming language's exception
 handling. First, there's a server, which produces the errors, and a separate
 client, which displays them. You cannot catch an exception in the client.

 BTW, let me throw in one use case to consider. We've been talking about
 psql, and what to print, but imagine a more sophisticated client like
 pgAdmin. It's not limited to either printing the context or not. It could
 e.g. hide the context information of all messages when they occur, but if
 you double-click on it, it's expanded to show all the context, location and
 all. You can't do that if the server doesn't send the context information in
 the first place.

 I would be happy to show you the psql redirected output logs from my
 nightly server processes that spew into the megabytes because of
 logging various high level steps (did this, did that).

 Oh, I believe you. I understand what the problem is, we're only talking
 about how best to address it.

Yeah.  For posterity, a psql based solution would work fine for me,
but a server side solution has a lot of advantages (less protocol
chatter, more configurability, keeping libpq/psql light).

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-07 18:15 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  what is RAISE EXCEPTION - first or second case?

 First: RAISE (unless caught) is no different than any other kind of error.

 On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 07/07/2015 04:56 PM, Merlin Moncure wrote:
  It doesn't have to if the behavior is guarded with a GUC.  I just
  don't understand what all the fuss is about.  The default behavior of
  logging that is well established by other languages (for example java)
  that manage error stack for you should be to:
 
  *) Give stack trace when an uncaught exception is thrown
  *) Do not give stack trace in all other logging cases unless asked for
 
  Java's exception handling is so different from PostgreSQL's errors that I
  don't think there's much to be learned from that. But I'll bite:
 
  First of all, Java's exceptions always contain a stack trace. It's up to
 you
  when you catch an exception to decide whether to print it or not. try {
 ...
  } catch (Exception e) { e.printStackTrace() } is fairly common,
 actually.
  There is nothing like a NOTICE in Java, i.e. an exception that's thrown
 but
  doesn't affect the control flow. The best I can think of is
  System.out.println(), which of course has no stack trace attached to it.

 exactly.

  Perhaps you're arguing that NOTICE is more like printing to stderr, and
  should never contain any context information. I don't think that would
 be an
  improvement. It's very handy to have the context information available if
  don't know where a NOTICE is coming from, even if in most cases you're
 not
  interested in it.

 That's exactly what I'm arguing.  NOTICE (and WARNING) are for
 printing out information to client side logging; it's really the only
 tool we have for that purpose and it fits that role perfectly.  Of
 course, you may want to have NOTICE print context, especially when
 debugging, but some control over that would be nice and in most cases
 it's really not necessary.  I really don't understand the objection to
 offering control over that behavior although I certainly understand
 wanting to keep the default behavior as it currently is.

  This is really quite different from a programming language's exception
  handling. First, there's a server, which produces the errors, and a
 separate
  client, which displays them. You cannot catch an exception in the client.
 
  BTW, let me throw in one use case to consider. We've been talking about
  psql, and what to print, but imagine a more sophisticated client like
  pgAdmin. It's not limited to either printing the context or not. It could
  e.g. hide the context information of all messages when they occur, but if
  you double-click on it, it's expanded to show all the context, location
 and
  all. You can't do that if the server doesn't send the context
 information in
  the first place.
 
  I would be happy to show you the psql redirected output logs from my
  nightly server processes that spew into the megabytes because of
  logging various high level steps (did this, did that).
 
  Oh, I believe you. I understand what the problem is, we're only talking
  about how best to address it.

 Yeah.  For posterity, a psql based solution would work fine for me,
 but a server side solution has a lot of advantages (less protocol
 chatter, more configurability, keeping libpq/psql light).


I prefer a server side solution too. With it I can have (as plpgsql
developer) bigger control of expected output.

Client can change this behave on global (min_context) or on language level
(plpgsql.min_context). If somebody afraid about security, we can to enforce
rule so min_context = error always.

The possibility to enable or disable context per any RAISE statement is
nice to have, but it is not fundamental.

Other variant is a implementation of min_context on client side -  but then
we cannot to ensure current behave and fix plpgsql raise exception issue
together.

Pavel



 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-07-07 15:56 GMT+02:00 Merlin Moncure mmonc...@gmail.com:

 On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas hlinn...@iki.fi
 wrote:
  On 01/26/2015 05:14 PM, Tom Lane wrote:
 
  Pavel Stehule pavel.steh...@gmail.com writes:
 
  2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
  I am thinking, so solution
 
 
/* if we are doing RAISE, don't report its location */
   if (estate-err_text == raise_skip_msg)
   return;
 
 
  is too simple, and this part should be fixed. This change can be done
 by
  on
  plpgsql or libpq side. This is bug, and it should be fixed.
 
 
  Doing this in libpq is utterly insane.  It has not got sufficient
 context
  to do anything intelligent.  The fact that it's not intelligent is
 exposed
  by the regression test changes that the proposed patch causes, most of
  which do not look like improvements.
 
  How can the server know if the client wants to display context
 information?

 It doesn't have to if the behavior is guarded with a GUC.  I just
 don't understand what all the fuss is about.  The default behavior of
 logging that is well established by other languages (for example java)
 that manage error stack for you should be to:

 *) Give stack trace when an uncaught exception is thrown
 *) Do not give stack trace in all other logging cases unless asked for


what is RAISE EXCEPTION - first or second case?



 I would be happy to show you the psql redirected output logs from my
 nightly server processes that spew into the megabytes because of
 logging various high level steps (did this, did that).   I can't throw
 the verbose switch to terse because if the error happens to be
 'Division by Zero', or some other difficult to trace problem then I'm
 sunk.  I believe the protocol decision to 'always send context' needs
 to be revisited; if your server-side codebase is large and heavily
 nested it makes logging an expensive operation even if the client
 strips off the log.

 plpgsql.min_context seems like the ideal solution to this problem; it
 can be managed on the server or the client and does not require new
 syntax.  If we require syntax to slip and and out of debugging type
 operations the solution has missed the mark IMNSHO.

 merlin

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Heikki Linnakangas]

On 01/26/2015 05:14 PM, Tom Lane wrote:

Pavel Stehule pavel.steh...@gmail.com writes:

2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
I am thinking, so solution



  /* if we are doing RAISE, don't report its location */
 if (estate-err_text == raise_skip_msg)
 return;



is too simple, and this part should be fixed. This change can be done by on
plpgsql or libpq side. This is bug, and it should be fixed.


Doing this in libpq is utterly insane.  It has not got sufficient context
to do anything intelligent.  The fact that it's not intelligent is exposed
by the regression test changes that the proposed patch causes, most of
which do not look like improvements.


I think doing this in libpq (or psql) is the way to go. How can the 
server know if the client wants to display context information? We just 
have to make sure the client has enough information to make a smart 
decision. If the client doesn't have enough information today, then 
let's work on that.


Note that Marko's patch didn't change libpq's default printing mode, 
which is why you got all the extra CONTEXT lines in the regression tests 
that were not there before. Just as if we just removed the suppression 
from PL/pgSQL and did nothing else. I think we need to also change the 
default behaviour to not print CONTEXT lines for NOTICE-level messages, 
getting us closer to the current behaviour again.


If you run the regression tests in the compact verbosity, the 
regression test output changes look quite sensible to me. See attached.



Another problem is that past requests to change this behavior have
generally been to the effect that people wanted *more* context suppressed
not less, ie they didn't want any CONTEXT lines at all on certain
messages.  So the proposed patch seems to me to be going in exactly the
wrong direction.


After changing the default to compact, it prints less CONTEXT lines.


The design I thought had been agreed on was to add some new option to
plpgsql's RAISE command which would cause suppression of all CONTEXT lines
not just the most closely nested one.


I don't understand how you came to that conclusion. In particular, see 
http://www.postgresql.org/message-id/6656.1377100...@sss.pgh.pa.us. If 
you changed your mind, you forgot to tell why.


- Heikki
*** 
/home/heikki/git-sandbox-pgsql/master/src/test/regress/expected/triggers.out
2015-07-07 15:40:38.697861317 +0300
--- /home/heikki/git-sandbox-pgsql/master/src/test/regress/results/triggers.out 
2015-07-07 15:50:34.885805473 +0300
***
*** 958,968 
  NOTICE:  main_view INSTEAD OF INSERT ROW (instead_of_ins)
  NOTICE:  NEW: (20,30)
  NOTICE:  trigger_func(before_ins_stmt) called: action = INSERT, when = 
BEFORE, level = STATEMENT
- CONTEXT:  SQL statement INSERT INTO main_table VALUES (NEW.a, NEW.b)
- PL/pgSQL function view_trigger() line 17 at SQL statement
  NOTICE:  trigger_func(after_ins_stmt) called: action = INSERT, when = AFTER, 
level = STATEMENT
- CONTEXT:  SQL statement INSERT INTO main_table VALUES (NEW.a, NEW.b)
- PL/pgSQL function view_trigger() line 17 at SQL statement
  NOTICE:  main_view AFTER INSERT STATEMENT (after_view_ins_stmt)
  INSERT 0 1
  INSERT INTO main_view VALUES (21, 31) RETURNING a, b;
--- 958,964 
***
*** 970,980 
  NOTICE:  main_view INSTEAD OF INSERT ROW (instead_of_ins)
  NOTICE:  NEW: (21,31)
  NOTICE:  trigger_func(before_ins_stmt) called: action = INSERT, when = 
BEFORE, level = STATEMENT
- CONTEXT:  SQL statement INSERT INTO main_table VALUES (NEW.a, NEW.b)
- PL/pgSQL function view_trigger() line 17 at SQL statement
  NOTICE:  trigger_func(after_ins_stmt) called: action = INSERT, when = AFTER, 
level = STATEMENT
- CONTEXT:  SQL statement INSERT INTO main_table VALUES (NEW.a, NEW.b)
- PL/pgSQL function view_trigger() line 17 at SQL statement
  NOTICE:  main_view AFTER INSERT STATEMENT (after_view_ins_stmt)
   a  | b  
  +
--- 966,972 
***
*** 988,1004 
  NOTICE:  main_view INSTEAD OF UPDATE ROW (instead_of_upd)
  NOTICE:  OLD: (20,30), NEW: (20,31)
  NOTICE:  trigger_func(before_upd_a_stmt) called: action = UPDATE, when = 
BEFORE, level = STATEMENT
- CONTEXT:  SQL statement UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = 
OLD.a AND b = OLD.b
- PL/pgSQL function view_trigger() line 23 at SQL statement
  NOTICE:  trigger_func(before_upd_a_row) called: action = UPDATE, when = 
BEFORE, level = ROW
- CONTEXT:  SQL statement UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = 
OLD.a AND b = OLD.b
- PL/pgSQL function view_trigger() line 23 at SQL statement
  NOTICE:  trigger_func(after_upd_b_stmt) called: action = UPDATE, when = 
AFTER, level = STATEMENT
- CONTEXT:  SQL statement UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = 
OLD.a AND b = OLD.b
- PL/pgSQL function view_trigger() line 23 at SQL statement
  NOTICE:  trigger_func(after_upd_stmt) called: action = UPDATE, when = AFTER, 
level = STATEMENT
- CONTEXT:  SQL statement 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas hlinn...@iki.fi wrote:
 On 01/26/2015 05:14 PM, Tom Lane wrote:

 Pavel Stehule pavel.steh...@gmail.com writes:

 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
 I am thinking, so solution


   /* if we are doing RAISE, don't report its location */
  if (estate-err_text == raise_skip_msg)
  return;


 is too simple, and this part should be fixed. This change can be done by
 on
 plpgsql or libpq side. This is bug, and it should be fixed.


 Doing this in libpq is utterly insane.  It has not got sufficient context
 to do anything intelligent.  The fact that it's not intelligent is exposed
 by the regression test changes that the proposed patch causes, most of
 which do not look like improvements.

 How can the server know if the client wants to display context information?

It doesn't have to if the behavior is guarded with a GUC.  I just
don't understand what all the fuss is about.  The default behavior of
logging that is well established by other languages (for example java)
that manage error stack for you should be to:

*) Give stack trace when an uncaught exception is thrown
*) Do not give stack trace in all other logging cases unless asked for

I would be happy to show you the psql redirected output logs from my
nightly server processes that spew into the megabytes because of
logging various high level steps (did this, did that).   I can't throw
the verbose switch to terse because if the error happens to be
'Division by Zero', or some other difficult to trace problem then I'm
sunk.  I believe the protocol decision to 'always send context' needs
to be revisited; if your server-side codebase is large and heavily
nested it makes logging an expensive operation even if the client
strips off the log.

plpgsql.min_context seems like the ideal solution to this problem; it
can be managed on the server or the client and does not require new
syntax.  If we require syntax to slip and and out of debugging type
operations the solution has missed the mark IMNSHO.

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Heikki Linnakangas]

On 07/07/2015 04:56 PM, Merlin Moncure wrote:

On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas hlinn...@iki.fi wrote:

On 01/26/2015 05:14 PM, Tom Lane wrote:


Pavel Stehule pavel.steh...@gmail.com writes:


2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
I am thinking, so solution




   /* if we are doing RAISE, don't report its location */
  if (estate-err_text == raise_skip_msg)
  return;




is too simple, and this part should be fixed. This change can be done by
on
plpgsql or libpq side. This is bug, and it should be fixed.



Doing this in libpq is utterly insane.  It has not got sufficient context
to do anything intelligent.  The fact that it's not intelligent is exposed
by the regression test changes that the proposed patch causes, most of
which do not look like improvements.


How can the server know if the client wants to display context information?


It doesn't have to if the behavior is guarded with a GUC.  I just
don't understand what all the fuss is about.  The default behavior of
logging that is well established by other languages (for example java)
that manage error stack for you should be to:

*) Give stack trace when an uncaught exception is thrown
*) Do not give stack trace in all other logging cases unless asked for


Java's exception handling is so different from PostgreSQL's errors that 
I don't think there's much to be learned from that. But I'll bite:


First of all, Java's exceptions always contain a stack trace. It's up to 
you when you catch an exception to decide whether to print it or not. 
try { ... } catch (Exception e) { e.printStackTrace() } is fairly 
common, actually. There is nothing like a NOTICE in Java, i.e. an 
exception that's thrown but doesn't affect the control flow. The best I 
can think of is System.out.println(), which of course has no stack trace 
attached to it.


Perhaps you're arguing that NOTICE is more like printing to stderr, and 
should never contain any context information. I don't think that would 
be an improvement. It's very handy to have the context information 
available if don't know where a NOTICE is coming from, even if in most 
cases you're not interested in it.


This is really quite different from a programming language's exception 
handling. First, there's a server, which produces the errors, and a 
separate client, which displays them. You cannot catch an exception in 
the client.


BTW, let me throw in one use case to consider. We've been talking about 
psql, and what to print, but imagine a more sophisticated client like 
pgAdmin. It's not limited to either printing the context or not. It 
could e.g. hide the context information of all messages when they occur, 
but if you double-click on it, it's expanded to show all the context, 
location and all. You can't do that if the server doesn't send the 
context information in the first place.



I would be happy to show you the psql redirected output logs from my
nightly server processes that spew into the megabytes because of
logging various high level steps (did this, did that).


Oh, I believe you. I understand what the problem is, we're only talking 
about how best to address it.

- Heikki



--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-30 10:24 GMT+02:00 Marko Tiikkaja ma...@joh.to:

 Hi Pavel,

 This doesn't seem to be what I thought we had agreed on.  For example:

 =# create function barf() returns void as $$ begin raise notice without
 context 'hello world'; end $$ language plpgsql;
 CREATE FUNCTION
 =# create function foof() returns void as $$ begin perform barf(); end $$
 language plpgsql;
 CREATE FUNCTION
 =# select foof();
 NOTICE:  hello world
 CONTEXT:  SQL statement SELECT barf()
 PL/pgSQL function foof() line 1 at PERFORM

 It's not only clear that WITHOUT CONTEXT didn't really work here, but it
 also had absolutely no effect since the context within barf() is also
 displayed.


It doesn't look well - because it should be solve by errhidecontext(true)

yes, there is a issue in send_message_to_frontend - this ignore
edata-hide_ctx field. After fixing, it working as expected - so this is a
bug in implementation of errhidecontext()

should be

if (edata-context  !edata-hide_ctx)
{
pq_sendbyte(msgbuf, PG_DIAG_CONTEXT);
err_sendstring(msgbuf, edata-context);
}

and probably getting stack in err_finish should be fixed too:

if (!edata-hide_ctx)
for (econtext = error_context_stack;
 econtext != NULL;
 econtext = econtext-previous)
(*econtext-callback) (econtext-arg);

Regards

Pavel



I'll look on this issue.

Regards

Pavel




 .m

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-28 19:44 GMT+02:00 Jim Nasby jim.na...@bluetreble.com:

 On 4/28/15 1:16 AM, Pavel Stehule wrote:


 I think it can't be any clearer than the proposed
 plpgsql.display_context_min_messages


 client_min_context. It's doing the same thing as min_messages does,
 just for context instead of the message.

 Or does this affect client and log the same way?


 it affect client and log together

 maybe min_context


 +1


third variant with GUC plpgsql.min_context

Regards

Pavel



 --
 Jim Nasby, Data Architect, Blue Treble Consulting
 Data in Trouble? Get it in Treble! http://BlueTreble.com

commit c2f49938f636864234d03994d2f64f8095392d11
Author: Pavel Stehule pavel.steh...@gooddata.com
Date:   Sat Apr 25 22:09:28 2015 +0200

initial implementation of (WITH|WITHOUT) CONTEXT clause to plpgsql RAISE statement.

initial implementation of plpgsql GUC plpgsql.min_context

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d36acf6..ffc3eb8 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3406,10 +3406,10 @@ END LOOP optional replaceablelabel/replaceable /optional;
 raise errors.
 
 synopsis
-RAISE optional replaceable class=parameterlevel/replaceable /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
 RAISE ;
 /synopsis
 
@@ -3431,6 +3431,18 @@ RAISE ;
/para
 
para
+The options literalWITH CONTEXT/literal or literalWITHOUT CONTEXT/literal
+can enforce or suppress context information related to error or notice. This possibility
+can be forced by settings of configuration parameter literalplpgsql.min_context/.
+This allows same values like replaceable class=parameterlevel/replaceable option plus
+value literalnone/literal that is a default. When it is changed, then all errors and notices
+with higher than specified severity are raised with context info.
+programlisting
+RAISE NOTICE WITH CONTEXT 'This message will have a context';
+/programlisting
+   /para
+
+   para
 After replaceable class=parameterlevel/replaceable if any,
 you can write a replaceable class=parameterformat/replaceable
 (which must be a simple string literal, not an expression).  The
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index deefb1f..eaee5a7 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2921,6 +2921,7 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 	char	   *err_table = NULL;
 	char	   *err_schema = NULL;
 	ListCell   *lc;
+	bool			hide_ctx = true;		/* suppress context by default */
 
 	/* RAISE with no parameters: re-throw current exception */
 	if (stmt-condname == NULL  stmt-message == NULL 
@@ -3080,10 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

Hi Pavel,

This doesn't seem to be what I thought we had agreed on.  For example:

=# create function barf() returns void as $$ begin raise notice without 
context 'hello world'; end $$ language plpgsql;

CREATE FUNCTION
=# create function foof() returns void as $$ begin perform barf(); end 
$$ language plpgsql;

CREATE FUNCTION
=# select foof();
NOTICE:  hello world
CONTEXT:  SQL statement SELECT barf()
PL/pgSQL function foof() line 1 at PERFORM

It's not only clear that WITHOUT CONTEXT didn't really work here, but it 
also had absolutely no effect since the context within barf() is also 
displayed.



.m


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-30 10:50 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-04-30 10:24 GMT+02:00 Marko Tiikkaja ma...@joh.to:

 Hi Pavel,

 This doesn't seem to be what I thought we had agreed on.  For example:

 =# create function barf() returns void as $$ begin raise notice without
 context 'hello world'; end $$ language plpgsql;
 CREATE FUNCTION
 =# create function foof() returns void as $$ begin perform barf(); end $$
 language plpgsql;
 CREATE FUNCTION
 =# select foof();
 NOTICE:  hello world
 CONTEXT:  SQL statement SELECT barf()
 PL/pgSQL function foof() line 1 at PERFORM

 It's not only clear that WITHOUT CONTEXT didn't really work here, but it
 also had absolutely no effect since the context within barf() is also
 displayed.


 It doesn't look well - because it should be solve by errhidecontext(true)

 yes, there is a issue in send_message_to_frontend - this ignore
 edata-hide_ctx field. After fixing, it working as expected - so this is a
 bug in implementation of errhidecontext()

 should be

 if (edata-context  !edata-hide_ctx)
 {
 pq_sendbyte(msgbuf, PG_DIAG_CONTEXT);
 err_sendstring(msgbuf, edata-context);
 }

 and probably getting stack in err_finish should be fixed too:

 if (!edata-hide_ctx)
 for (econtext = error_context_stack;
  econtext != NULL;
  econtext = econtext-previous)
 (*econtext-callback) (econtext-arg);

 Regards

 Pavel


I am sending patch




 I'll look on this issue.

 Regards

 Pavel




 .m

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Jim Nasby]

On 4/28/15 1:16 AM, Pavel Stehule wrote:


I think it can't be any clearer than the proposed
plpgsql.display_context_min_messages


client_min_context. It's doing the same thing as min_messages does,
just for context instead of the message.

Or does this affect client and log the same way?


it affect client and log together

maybe min_context


+1
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-27 22:53 GMT+02:00 Jim Nasby jim.na...@bluetreble.com:

 On 4/27/15 11:47 AM, Joel Jacobson wrote:

 On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja ma...@joh.to
 mailto:ma...@joh.to wrote:

 That sounds weird.  log_min_messages are the messages sent to the
 log; client_min_messages are sent to the client.
 context_min_messages are not sent to a context, whatever that
 would mean.


 Good point.

 I think it can't be any clearer than the proposed
 plpgsql.display_context_min_messages


 client_min_context. It's doing the same thing as min_messages does, just
 for context instead of the message.

 Or does this affect client and log the same way?


it affect client and log together

maybe min_context

Pavel



 --
 Jim Nasby, Data Architect, Blue Treble Consulting
 Data in Trouble? Get it in Treble! http://BlueTreble.com

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Jim Nasby]

On 4/27/15 11:47 AM, Joel Jacobson wrote:

On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja ma...@joh.to
mailto:ma...@joh.to wrote:

That sounds weird.  log_min_messages are the messages sent to the
log; client_min_messages are sent to the client.
context_min_messages are not sent to a context, whatever that
would mean.


Good point.

I think it can't be any clearer than the proposed
plpgsql.display_context_min_messages


client_min_context. It's doing the same thing as min_messages does, just 
for context instead of the message.


Or does this affect client and log the same way?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] PL/pgSQL, RAISE and error context

[Joel Jacobson]

Looks good Pavel!

May I just suggest you add the default case
to src/test/regress/sql/plpgsql.sql
and src/test/regress/expected/plpgsql.out, to make it easier for the
reviewer to compare the difference between what happens in the default
case, when not using the raise-syntax and not using the GUCs?

Suggested addition to the beginning of src/test/regress/sql/plpgsql.sql:
+do $$
+begin
+  raise notice 'hello';
+end;
+$$;
+
+do $$
+begin
+  raise exception 'hello';
+end;
+$$;

Many thanks for this patch! I will pray to the PL/pgSQL God it will be
accepted. :)

Best regards,

Joel


On Sun, Apr 26, 2015 at 9:19 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:

 Hi

 I reduced this patch, little bit cleaned - now it is based on plpgsql GUC
 display_context_min_messages - like client_min_messages, log_min_messages.

 Documentation added.

 Regards

 Pavel

 2015-04-25 22:23 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd
 config. :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


 I played with some prototype and I am thinking so we need only one GUC

 plpgsql.display_context_messages = 'none'; -- compatible with current
 plpgsql.display_context_messages = 'all';
 plpgsql.display_context_messages = 'exception, log'; -- what I prefer

 I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

 RAISE NOTICE WITH CONTEXT 'some message';
 RAISE NOTICE WITH CONTEXT USING message = 'some message';
 RAISE EXCEPTION WITHOUT CONTEXT 'other message';

 The patch is very small with full functionality (without documentation) -
 I am thinking so it can work. This patch is back compatible - and allow to
 change default behave simply.

 plpgsql.display_context_messages can be simplified to some like
 plpgsql.display_context_min_messages

 What do you think about it?

 Regards

 Pavel

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-27 16:05 GMT+02:00 Joel Jacobson j...@trustly.com:

 Looks good Pavel!

 May I just suggest you add the default case
 to src/test/regress/sql/plpgsql.sql
 and src/test/regress/expected/plpgsql.out, to make it easier for the
 reviewer to compare the difference between what happens in the default
 case, when not using the raise-syntax and not using the GUCs?

 Suggested addition to the beginning of src/test/regress/sql/plpgsql.sql:
 +do $$
 +begin
 +  raise notice 'hello';
 +end;
 +$$;
 +
 +do $$
 +begin
 +  raise exception 'hello';
 +end;
 +$$;


done



 Many thanks for this patch! I will pray to the PL/pgSQL God it will be
 accepted. :)


:) -- please, do review, or fix documentation in this patch.

I hope, so it will be merged early in 9.6 cycle. It is relatively simple.

Pavel



 Best regards,

 Joel


 On Sun, Apr 26, 2015 at 9:19 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:

 Hi

 I reduced this patch, little bit cleaned - now it is based on plpgsql GUC
 display_context_min_messages - like client_min_messages, log_min_messages.

 Documentation added.

 Regards

 Pavel

 2015-04-25 22:23 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd
 config. :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


 I played with some prototype and I am thinking so we need only one GUC

 plpgsql.display_context_messages = 'none'; -- compatible with current
 plpgsql.display_context_messages = 'all';
 plpgsql.display_context_messages = 'exception, log'; -- what I prefer

 I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

 RAISE NOTICE WITH CONTEXT 'some message';
 RAISE NOTICE WITH CONTEXT USING message = 'some message';
 RAISE EXCEPTION WITHOUT CONTEXT 'other message';

 The patch is very small with full functionality (without documentation)
 - I am thinking so it can work. This patch is back compatible - and allow
 to change default behave simply.

 plpgsql.display_context_messages can be simplified to some like
 plpgsql.display_context_min_messages

 What do you think about it?

 Regards

 Pavel




commit d60c21fb798cf25609dc37a4bda3ec7822f790e1
Author: Pavel Stehule pavel.steh...@gooddata.com
Date:   Sat Apr 25 22:09:28 2015 +0200

initial implementation of (WITH|WITHOUT) CONTEXT clause to plpgsql RAISE statement.

initial implementation of plpgsql GUC plpgsql.display_context_messages

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d36acf6..8aebb87 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3406,10 +3406,10 @@ END LOOP optional replaceablelabel/replaceable /optional;
 raise errors.
 
 synopsis
-RAISE optional replaceable class=parameterlevel/replaceable /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 4/27/15 6:08 PM, Fabrízio de Royes Mello wrote:

On Sun, Apr 26, 2015 at 4:19 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:

I reduced this patch, little bit cleaned - now it is based on plpgsql GUC

display_context_min_messages - like client_min_messages, log_min_messages.

What you think just context_min_messages ?


That sounds weird.  log_min_messages are the messages sent to the log; 
client_min_messages are sent to the client.  context_min_messages are 
not sent to a context, whatever that would mean.



.m


--
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] PL/pgSQL, RAISE and error context

[Fabrízio de Royes Mello]

On Sun, Apr 26, 2015 at 4:19 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:

 Hi

 I reduced this patch, little bit cleaned - now it is based on plpgsql GUC
display_context_min_messages - like client_min_messages, log_min_messages.


What you think just context_min_messages ?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joel Jacobson]

On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja ma...@joh.to wrote:

 That sounds weird.  log_min_messages are the messages sent to the log;
 client_min_messages are sent to the client.  context_min_messages are not
 sent to a context, whatever that would mean.


Good point.

I think it can't be any clearer than the proposed
plpgsql.display_context_min_messages

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

I reduced this patch, little bit cleaned - now it is based on plpgsql GUC
display_context_min_messages - like client_min_messages, log_min_messages.

Documentation added.

Regards

Pavel

2015-04-25 22:23 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd
 config. :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


 I played with some prototype and I am thinking so we need only one GUC

 plpgsql.display_context_messages = 'none'; -- compatible with current
 plpgsql.display_context_messages = 'all';
 plpgsql.display_context_messages = 'exception, log'; -- what I prefer

 I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

 RAISE NOTICE WITH CONTEXT 'some message';
 RAISE NOTICE WITH CONTEXT USING message = 'some message';
 RAISE EXCEPTION WITHOUT CONTEXT 'other message';

 The patch is very small with full functionality (without documentation) -
 I am thinking so it can work. This patch is back compatible - and allow to
 change default behave simply.

 plpgsql.display_context_messages can be simplified to some like
 plpgsql.display_context_min_messages

 What do you think about it?

 Regards

 Pavel


commit 33951bc23365029ee94af5ec43e90893dcd737a8
Author: Pavel Stehule pavel.steh...@gooddata.com
Date:   Sat Apr 25 22:09:28 2015 +0200

initial implementation of (WITH|WITHOUT) CONTEXT clause to plpgsql RAISE statement.

initial implementation of plpgsql GUC plpgsql.display_context_messages

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d36acf6..8aebb87 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3406,10 +3406,10 @@ END LOOP optional replaceablelabel/replaceable /optional;
 raise errors.
 
 synopsis
-RAISE optional replaceable class=parameterlevel/replaceable /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
-RAISE optional replaceable class=parameterlevel/replaceable /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
+RAISE optional replaceable class=parameterlevel/replaceable /optional optional ( WITH | WITHOUT ) CONTEXT /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
 RAISE ;
 /synopsis
 
@@ -3431,6 +3431,18 @@ RAISE ;
/para
 
para
+The options literalWITH CONTEXT/literal or literalWITHOUT CONTEXT/literal
+can enforce or suppress context information related to error or notice. This possibility
+can be forced by settings of configuration parameter literalplpgsql.display_context_min_messages/.
+This allows same values like replaceable class=parameterlevel/replaceable option plus
+value 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd config.
 :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


I played with some prototype and I am thinking so we need only one GUC

plpgsql.display_context_messages = 'none'; -- compatible with current
plpgsql.display_context_messages = 'all';
plpgsql.display_context_messages = 'exception, log'; -- what I prefer

I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

RAISE NOTICE WITH CONTEXT 'some message';
RAISE NOTICE WITH CONTEXT USING message = 'some message';
RAISE EXCEPTION WITHOUT CONTEXT 'other message';

The patch is very small with full functionality (without documentation) - I
am thinking so it can work. This patch is back compatible - and allow to
change default behave simply.

plpgsql.display_context_messages can be simplified to some like
plpgsql.display_context_min_messages

What do you think about it?

Regards

Pavel
commit cf9e23a29162ac55fcab1ac4d9e7a24492de0736
Author: Pavel Stehule pavel.steh...@gooddata.com
Date:   Sat Apr 25 22:09:28 2015 +0200

initial implementation of (WITH|WITHOUT) CONTEXT clause to plpgsql RAISE statement.

initial implementation of plpgsql GUC plpgsql.display_context_messages

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index deefb1f..ea0dac5 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2921,6 +2921,7 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 	char	   *err_table = NULL;
 	char	   *err_schema = NULL;
 	ListCell   *lc;
+	bool			hide_ctx;
 
 	/* RAISE with no parameters: re-throw current exception */
 	if (stmt-condname == NULL  stmt-message == NULL 
@@ -3080,10 +3081,42 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 			err_message = pstrdup(unpack_sql_state(err_code));
 	}
 
-	/*
-	 * Throw the error (may or may not come back)
-	 */
-	estate-err_text = raise_skip_msg;	/* suppress traceback of raise */
+	if (stmt-context_info == PLPGSQL_CONTEXT_DISPLAY)
+		hide_ctx = false;
+	else if (stmt-context_info == PLPGSQL_CONTEXT_SUPPRESS)
+	{
+		hide_ctx = true;
+	}
+	else
+	{
+		/* we display a messages via plpgsql_display_context_messages */
+		switch (stmt-elog_level)
+		{
+			case ERROR:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_ERROR);
+break;
+			case WARNING:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_WARNING);
+break;
+			case NOTICE:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_NOTICE);
+break;
+			case INFO:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_INFO);
+break;
+			case LOG:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_LOG);
+break;
+			case DEBUG1:
+hide_ctx = !(plpgsql_display_context_messages  PLPGSQL_DISPLAY_CONTEXT_DEBUG);
+break;
+			default:
+elog(ERROR, unexpected RAISE statement level);
+		}
+	}
+
+	if (hide_ctx)
+		estate-err_text = raise_skip_msg;
 
 	ereport(stmt-elog_level,
 			(err_code ? errcode(err_code) : 0,
@@ -3099,7 +3132,8 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 			 (err_table != NULL) ?
 			 err_generic_string(PG_DIAG_TABLE_NAME, err_table) : 0,
 			 (err_schema != NULL) ?
-			 err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0));
+			 err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0,
+			 errhidecontext(hide_ctx)));
 
 	estate-err_text = NULL;	/* un-suppress... */
 
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 4026e41..48914a7 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -259,6 +259,7 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token keyword	K_CONSTANT
 %token keyword	K_CONSTRAINT
 %token keyword	K_CONSTRAINT_NAME
+%token keyword	K_CONTEXT
 %token keyword	K_CONTINUE
 %token keyword	K_CURRENT
 %token keyword	K_CURSOR
@@ -341,6 +342,8 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token keyword	K_WARNING
 %token keyword	K_WHEN
 %token keyword	K_WHILE
+%token keyword	K_WITH
+%token keyword	K_WITHOUT
 
 %%
 
@@ -1716,6 +1719,7 @@ stmt_raise		: K_RAISE
 		new-cmd_type	= PLPGSQL_STMT_RAISE;
 		new-lineno		= plpgsql_location_to_lineno(@1);
 		new-elog_level = ERROR;	/* default */
+		new-context_info = PLPGSQL_CONTEXT_DEFAULT;
 		new-condname	= NULL;
 		new-message	= NULL;
 		new-params		= NIL;
@@ -1773,6 +1777,21 @@ stmt_raise		: K_RAISE
 			if (tok == 0)
 

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joel Jacobson]

+1


On Sat, Apr 25, 2015 at 10:23 PM, Pavel Stehule pavel.steh...@gmail.com
wrote:

 Hi

 2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd
 config. :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


 I played with some prototype and I am thinking so we need only one GUC

 plpgsql.display_context_messages = 'none'; -- compatible with current
 plpgsql.display_context_messages = 'all';
 plpgsql.display_context_messages = 'exception, log'; -- what I prefer

 I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

 RAISE NOTICE WITH CONTEXT 'some message';
 RAISE NOTICE WITH CONTEXT USING message = 'some message';
 RAISE EXCEPTION WITHOUT CONTEXT 'other message';

 The patch is very small with full functionality (without documentation) -
 I am thinking so it can work. This patch is back compatible - and allow to
 change default behave simply.

 plpgsql.display_context_messages can be simplified to some like
 plpgsql.display_context_min_messages

 What do you think about it?

 Regards

 Pavel

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-24 12:11 GMT+02:00 Joel Jacobson j...@trustly.com:

 Entering the discussion because this is a huge pain for me in my daily
 work as well.

 This is not a reply to any specific post in this thread, but my first
 message in the thread.

 I see a great value in providing both a GUC and a new RAISE syntax.
 The different benefits of the two are maybe obvious, but perhaps worth
 pointing out:
 GUC: Good because you don't have to change any existing code.
 RAISE syntax: Good because you can control exactly what message should
 be emitted or not be emitted at that line of code.

 I think preserving backwards compatibility is very important.
 Not changing the default is not a problem for me, as long as it can be
 overridden.

 Whatever the default behaviour is, I think the need expressed by all
 users in this thread boils down to any of these two sentences:

 I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
 RAISE (NOTICE|WARNING|ERROR)
 OR
 I don't want to change the default current behaviour of CONTEXT

 So we basically need a boolean setting value, where:
 NULL means the default behaviour
 TRUE means DISPLAY CONTEXT
 FALSE means SUPPRESS CONTEXT

 And the (ALL|ONLY THIS) part translates into using,
 * a GUC to change behaviour for ALL lines of code,
 * or using the RAISE syntax to change the behaviour of ONLY THIS line of
 code.

 And then we have the different message levels, for which CONTEXT is
 sometimes desirable in some situations:
 * The RAISE syntax allows controlling any message level in a natural
 way, as the message level is part of the syntax.
 * Allowing the same control using GUC would mean the message level
 would need to be part of the GUC key name, which means either add
 multiple GUCs, one for each message level, or only allow controlling
 the most important one and ignore the possibly need to control the
 other message levels.

 If it would be possible to somehow combine multiple message levels in
 the same GUC, that would solve the latter problem.

 We already have comma separated values for many GUCs, so maybe we
 could use that approach here as well.

 It looks like adding these two GUCs would meet the demands of all users:




 suppress_context_messages (enum)
 display_context_messages (enum)



This proposal looks very practical - it can be very good start point - and
it doesn't block any next discuss about enhancing RAISE statement, what I
would to have too (bat can be separate issue). I like it.

Regards

Pavel




 This would allow doing something crazy as:

 suppress_context_messages = warning,error
 display_context_messages = notice

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-24 13:16 GMT+02:00 Robert Haas robertmh...@gmail.com:

 On Fri, Apr 24, 2015 at 6:11 AM, Joel Jacobson j...@trustly.com wrote:
  Entering the discussion because this is a huge pain for me in my daily
  work as well.
 
  This is not a reply to any specific post in this thread, but my first
  message in the thread.
 
  I see a great value in providing both a GUC and a new RAISE syntax.
  The different benefits of the two are maybe obvious, but perhaps worth
  pointing out:
  GUC: Good because you don't have to change any existing code.
  RAISE syntax: Good because you can control exactly what message should
  be emitted or not be emitted at that line of code.
 
  I think preserving backwards compatibility is very important.
  Not changing the default is not a problem for me, as long as it can be
  overridden.
 
  Whatever the default behaviour is, I think the need expressed by all
  users in this thread boils down to any of these two sentences:
 
  I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
  RAISE (NOTICE|WARNING|ERROR)
  OR
  I don't want to change the default current behaviour of CONTEXT
 
  So we basically need a boolean setting value, where:
  NULL means the default behaviour
  TRUE means DISPLAY CONTEXT
  FALSE means SUPPRESS CONTEXT
 
  And the (ALL|ONLY THIS) part translates into using,
  * a GUC to change behaviour for ALL lines of code,
  * or using the RAISE syntax to change the behaviour of ONLY THIS line of
 code.
 
  And then we have the different message levels, for which CONTEXT is
  sometimes desirable in some situations:
  * The RAISE syntax allows controlling any message level in a natural
  way, as the message level is part of the syntax.
  * Allowing the same control using GUC would mean the message level
  would need to be part of the GUC key name, which means either add
  multiple GUCs, one for each message level, or only allow controlling
  the most important one and ignore the possibly need to control the
  other message levels.
 
  If it would be possible to somehow combine multiple message levels in
  the same GUC, that would solve the latter problem.
 
  We already have comma separated values for many GUCs, so maybe we
  could use that approach here as well.
 
  It looks like adding these two GUCs would meet the demands of all users:
 
  suppress_context_messages (enum)
  display_context_messages (enum)
 
  This would allow doing something crazy as:
 
  suppress_context_messages = warning,error
  display_context_messages = notice

 This is a very flexible proposal, but it's a tremendous amount of
 machinery for what's really a very minor issue.  If we added two GUCs
 for every comparably important issue, we'd have about 40,000 of them.


It can be PLpgSQL only GUC. Probably it is a most problematic case.


 I suggest we add the RAISE syntax first, because everybody agrees on
 that.  Then, we can argue about the other stuff.


There is a agreement about it - but I am expecting a harder discussion
about what will be default, and the discussion about syntax should not be
simple too.





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

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joel Jacobson]

Entering the discussion because this is a huge pain for me in my daily
work as well.

This is not a reply to any specific post in this thread, but my first
message in the thread.

I see a great value in providing both a GUC and a new RAISE syntax.
The different benefits of the two are maybe obvious, but perhaps worth
pointing out:
GUC: Good because you don't have to change any existing code.
RAISE syntax: Good because you can control exactly what message should
be emitted or not be emitted at that line of code.

I think preserving backwards compatibility is very important.
Not changing the default is not a problem for me, as long as it can be
overridden.

Whatever the default behaviour is, I think the need expressed by all
users in this thread boils down to any of these two sentences:

I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
RAISE (NOTICE|WARNING|ERROR)
OR
I don't want to change the default current behaviour of CONTEXT

So we basically need a boolean setting value, where:
NULL means the default behaviour
TRUE means DISPLAY CONTEXT
FALSE means SUPPRESS CONTEXT

And the (ALL|ONLY THIS) part translates into using,
* a GUC to change behaviour for ALL lines of code,
* or using the RAISE syntax to change the behaviour of ONLY THIS line of code.

And then we have the different message levels, for which CONTEXT is
sometimes desirable in some situations:
* The RAISE syntax allows controlling any message level in a natural
way, as the message level is part of the syntax.
* Allowing the same control using GUC would mean the message level
would need to be part of the GUC key name, which means either add
multiple GUCs, one for each message level, or only allow controlling
the most important one and ignore the possibly need to control the
other message levels.

If it would be possible to somehow combine multiple message levels in
the same GUC, that would solve the latter problem.

We already have comma separated values for many GUCs, so maybe we
could use that approach here as well.

It looks like adding these two GUCs would meet the demands of all users:

suppress_context_messages (enum)
display_context_messages (enum)

This would allow doing something crazy as:

suppress_context_messages = warning,error
display_context_messages = notice


-- 
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] PL/pgSQL, RAISE and error context

[Robert Haas]

On Fri, Apr 24, 2015 at 6:11 AM, Joel Jacobson j...@trustly.com wrote:
 Entering the discussion because this is a huge pain for me in my daily
 work as well.

 This is not a reply to any specific post in this thread, but my first
 message in the thread.

 I see a great value in providing both a GUC and a new RAISE syntax.
 The different benefits of the two are maybe obvious, but perhaps worth
 pointing out:
 GUC: Good because you don't have to change any existing code.
 RAISE syntax: Good because you can control exactly what message should
 be emitted or not be emitted at that line of code.

 I think preserving backwards compatibility is very important.
 Not changing the default is not a problem for me, as long as it can be
 overridden.

 Whatever the default behaviour is, I think the need expressed by all
 users in this thread boils down to any of these two sentences:

 I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
 RAISE (NOTICE|WARNING|ERROR)
 OR
 I don't want to change the default current behaviour of CONTEXT

 So we basically need a boolean setting value, where:
 NULL means the default behaviour
 TRUE means DISPLAY CONTEXT
 FALSE means SUPPRESS CONTEXT

 And the (ALL|ONLY THIS) part translates into using,
 * a GUC to change behaviour for ALL lines of code,
 * or using the RAISE syntax to change the behaviour of ONLY THIS line of code.

 And then we have the different message levels, for which CONTEXT is
 sometimes desirable in some situations:
 * The RAISE syntax allows controlling any message level in a natural
 way, as the message level is part of the syntax.
 * Allowing the same control using GUC would mean the message level
 would need to be part of the GUC key name, which means either add
 multiple GUCs, one for each message level, or only allow controlling
 the most important one and ignore the possibly need to control the
 other message levels.

 If it would be possible to somehow combine multiple message levels in
 the same GUC, that would solve the latter problem.

 We already have comma separated values for many GUCs, so maybe we
 could use that approach here as well.

 It looks like adding these two GUCs would meet the demands of all users:

 suppress_context_messages (enum)
 display_context_messages (enum)

 This would allow doing something crazy as:

 suppress_context_messages = warning,error
 display_context_messages = notice

This is a very flexible proposal, but it's a tremendous amount of
machinery for what's really a very minor issue.  If we added two GUCs
for every comparably important issue, we'd have about 40,000 of them.

I suggest we add the RAISE syntax first, because everybody agrees on
that.  Then, we can argue about the other stuff.

-- 
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] PL/pgSQL, RAISE and error context

[Joel Jacobson]

On Fri, Apr 24, 2015 at 1:16 PM, Robert Haas robertmh...@gmail.com wrote:
 This would allow doing something crazy as:

 suppress_context_messages = warning,error
 display_context_messages = notice

 This is a very flexible proposal, but it's a tremendous amount of
 machinery for what's really a very minor issue.  If we added two GUCs
 for every comparably important issue, we'd have about 40,000 of them.

I agree. The one-dimensional GUC syntax is not well suited for
multi-dimensional config settings. And that's a good thing mostly I
think. It would be a nightmare if the config file values could in JSON
format, it's good they are simple.

But I'm thinking maybe we could improve the config file syntax for the
general case when you have multiple things you want to control, in
this case the message levels, and for each such thing, you want to
turn something on/off, in this case the CONTEXT. Maybe we could simply
use plus + and minus - to mean on and off?

Example:

context_messages = -warning, -error, +notice


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-24 16:02 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 1:16 PM, Robert Haas robertmh...@gmail.com
 wrote:
  This would allow doing something crazy as:
 
  suppress_context_messages = warning,error
  display_context_messages = notice
 
  This is a very flexible proposal, but it's a tremendous amount of
  machinery for what's really a very minor issue.  If we added two GUCs
  for every comparably important issue, we'd have about 40,000 of them.

 I agree. The one-dimensional GUC syntax is not well suited for
 multi-dimensional config settings. And that's a good thing mostly I
 think. It would be a nightmare if the config file values could in JSON
 format, it's good they are simple.

 But I'm thinking maybe we could improve the config file syntax for the
 general case when you have multiple things you want to control, in
 this case the message levels, and for each such thing, you want to
 turn something on/off, in this case the CONTEXT. Maybe we could simply
 use plus + and minus - to mean on and off?

 Example:

 context_messages = -warning, -error, +notice


I prefer your first proposal - and there is a precedent for plpgsql -
plpgsql_extra_checks

It is clean for anybody. +-identifiers looks like horrible httpd config. :)

Regards

Pavel

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joel Jacobson]

On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
 Example:

 context_messages = -warning, -error, +notice


 I prefer your first proposal - and there is a precedent for plpgsql -
 plpgsql_extra_checks

 It is clean for anybody. +-identifiers looks like horrible httpd config. :)

I have to agree on that :) Just thought this is the best we can do if
we want to reduce the number of GUCs to a minimum.


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-24 19:16 GMT+02:00 Joel Jacobson j...@trustly.com:

 On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  Example:
 
  context_messages = -warning, -error, +notice
 
 
  I prefer your first proposal - and there is a precedent for plpgsql -
  plpgsql_extra_checks
 
  It is clean for anybody. +-identifiers looks like horrible httpd config.
 :)

 I have to agree on that :) Just thought this is the best we can do if
 we want to reduce the number of GUCs to a minimum.


It looks like discussion KDE x GNOME.

GUC that has simply effect on behave without performance impact should not
be problem - like log_lock_wait, log_min_duration and similar. I am sure so
we would it.

The problematic GUC are a performance, planner, bgwriter, checkpoint
related.

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-23 16:12 GMT+02:00 Robert Haas robertmh...@gmail.com:

 On Thu, Apr 23, 2015 at 9:55 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule pavel.steh...@gmail.com
 
  wrote:
   I don't see a contradiction. There is clean agreement, so ERROR level
   should
   to show the context. NOTICE and WARNINGs doesn't need it - and there
 is
   a
   backward compatibility and usability reasons don't do it.
 
  Whether notices and warnings need it is a matter of opinion.  I don't
  think your idea is bad, and it might be a good rule of thumb in many
  cases, but I slightly prefer Marko's approach of adding a new option.
 
  I am not sure if I understand to you.
 
  please, can you write more about your idea?

 Your idea, as I understand it, is that for logs at severity levels
 lower than ERROR, we can always emit the context, because it's not
 necessary.  But I'm not sure that's right: some people might find that
 context helpful.  If, as Marko proposes, we add an explicit option,
 then everyone can choose the behavior that is right for them.


I am not sure, if explained it well. I would to emit context for ERROR and
higher by default. And I would not to emit context for any less than ERROR
by default (I am not sure about WARNING level).

But it can be changed by some option in RAISE statement like Marko proposes
- possible to change by GUC globally, because it doesn't change a behave of
application.

For current behave I have a problem with ERROR level in plpgsql where the
context is missing now.  On second hand I am thinking so current behave is
ok for NOTICE level .

I am not against to any new option in RAISE statement.

If there is some collision between me and Marko, then it is in opinion what
have to be default behave for NOTICE level. I strongly prefer don't show
context there. But I can accept some global switch too.

Regards

Pavel



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

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-23 15:47 GMT+02:00 Robert Haas robertmh...@gmail.com:

 On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  I don't see a contradiction. There is clean agreement, so ERROR level
 should
  to show the context. NOTICE and WARNINGs doesn't need it - and there is a
  backward compatibility and usability reasons don't do it.

 Whether notices and warnings need it is a matter of opinion.  I don't
 think your idea is bad, and it might be a good rule of thumb in many
 cases, but I slightly prefer Marko's approach of adding a new option.


I am not sure if I understand to you.

please, can you write more about your idea?





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

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Robert Haas]

On Thu, Apr 23, 2015 at 9:55 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule pavel.steh...@gmail.com
 wrote:
  I don't see a contradiction. There is clean agreement, so ERROR level
  should
  to show the context. NOTICE and WARNINGs doesn't need it - and there is
  a
  backward compatibility and usability reasons don't do it.

 Whether notices and warnings need it is a matter of opinion.  I don't
 think your idea is bad, and it might be a good rule of thumb in many
 cases, but I slightly prefer Marko's approach of adding a new option.

 I am not sure if I understand to you.

 please, can you write more about your idea?

Your idea, as I understand it, is that for logs at severity levels
lower than ERROR, we can always emit the context, because it's not
necessary.  But I'm not sure that's right: some people might find that
context helpful.  If, as Marko proposes, we add an explicit option,
then everyone can choose the behavior that is right for them.

-- 
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 4/2/15 9:37 AM, Pavel Stehule wrote:

estate-err_text = stmt-elog_level == ERROR ? estate-err_text :
raise_skip_msg  ;


Can we do this simple change? It will produce a stackinfo for exceptions
and it will not to make mad developers by lot of useless content.


I'm not sure everyone agrees with this to be honest, myself included.

I think the best way to do this would be to have an option for RAISE to 
suppress the context *regardless of nesting depth*, but show the full 
context by default for ERRORs.  For NOTICEs and WARNINGs I don't care 
much what the default will be; perhaps just full backwards compatibility 
could work there.



.m


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-04-23 9:53 GMT+02:00 Marko Tiikkaja ma...@joh.to:

 On 4/2/15 9:37 AM, Pavel Stehule wrote:

 estate-err_text = stmt-elog_level == ERROR ? estate-err_text :
 raise_skip_msg  ;


 Can we do this simple change? It will produce a stackinfo for exceptions
 and it will not to make mad developers by lot of useless content.


 I'm not sure everyone agrees with this to be honest, myself included.

 I think the best way to do this would be to have an option for RAISE to
 suppress the context *regardless of nesting depth*, but show the full
 context by default for ERRORs.  For NOTICEs and WARNINGs I don't care much
 what the default will be; perhaps just full backwards compatibility could
 work there.


I don't see a contradiction. There is clean agreement, so ERROR level
should to show the context. NOTICE and WARNINGs doesn't need it - and there
is a backward compatibility and usability reasons don't do it.

I am not to against to any special option to RAISE statement. Have you some
idea?

What about a enhancing a USING clause?

example:

RAISE NOTICE USING message = '', with_context = true
RAISE EXCEPTION USING message = '', with_context = false


Regards

Pavel





 .m

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Robert Haas]

On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 I don't see a contradiction. There is clean agreement, so ERROR level should
 to show the context. NOTICE and WARNINGs doesn't need it - and there is a
 backward compatibility and usability reasons don't do it.

Whether notices and warnings need it is a matter of opinion.  I don't
think your idea is bad, and it might be a good rule of thumb in many
cases, but I slightly prefer Marko's approach of adding a new option.

-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-26 16:46 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-01-26 16:14 GMT+01:00 Tom Lane t...@sss.pgh.pa.us:

 Pavel Stehule pavel.steh...@gmail.com writes:
  2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
  I am thinking, so solution

   /* if we are doing RAISE, don't report its location */
  if (estate-err_text == raise_skip_msg)
  return;

  is too simple, and this part should be fixed. This change can be done
 by on
  plpgsql or libpq side. This is bug, and it should be fixed.

 Doing this in libpq is utterly insane.  It has not got sufficient context
 to do anything intelligent.  The fact that it's not intelligent is exposed
 by the regression test changes that the proposed patch causes, most of
 which do not look like improvements.


 I don't understand. There can be a overhead due useless transformation
 some data to client side. But all what it need - errcontext and errlevel is
 possible.


 Another problem is that past requests to change this behavior have
 generally been to the effect that people wanted *more* context suppressed
 not less, ie they didn't want any CONTEXT lines at all on certain
 messages.  So the proposed patch seems to me to be going in exactly the
 wrong direction.

 The design I thought had been agreed on was to add some new option to
 plpgsql's RAISE command which would cause suppression of all CONTEXT lines
 not just the most closely nested one.  You could argue about whether the
 behavior needs to be 100% backwards compatible or not --- if so, perhaps
 it could be a three-way option all, none, or one line, defaulting to the
 last for backwards compatibility.


  I see a problem what should be default behave. When I raise NOTICE, then
 I don't need (don't would) to see CONTEXT lines, When I raise EXCEPTION,
 then I usually would to see CONTEXT lines.

 Cannot be solution?


I would to wakeup this thread.



 estate-err_text = stmt-elog_level == ERROR ? estate-err_text :
 raise_skip_msg  ;


Can we do this simple change? It will produce a stackinfo for exceptions
and it will not to make mad developers by lot of useless content.

Regards

Pavel



 Regards

 Pavel





 regards, tom lane

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 1/26/15 1:14 PM, Pavel Stehule wrote:

2015-01-26 13:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:

I can see where it's a lot nicer not to have the context visible for
people who only care about the contents of the message, but the way it's
done in PL/PgSQL right now is just not good enough.  On the other hand, the
backwards compatibility breakage of doing this in libpq is quite
extensive.  The most simple option seems to be to just allow a GUC to
change PL/PgSQL's behavior to match what all other PLs are doing.



libpq was changed more time - there is still a open task about a protocol
change.

I afraid about some unexpected side effects of your proposal if somebody
mix languages - these side effects should not be critical


I have no idea what you're talking about.  What kind of side effects?


- but on second
hand current behave is not critical too - we can wait.


I think the current behavior is almost unacceptable.  It makes debugging 
in some cases really, really difficult.



.marko


--
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 1/26/15 1:44 PM, Pavel Stehule wrote:

2015-01-26 13:39 GMT+01:00 Marko Tiikkaja ma...@joh.to:

On 1/26/15 1:14 PM, Pavel Stehule wrote:

I afraid about some unexpected side effects of your proposal if somebody
mix languages - these side effects should not be critical



I have no idea what you're talking about.  What kind of side effects?



what will be a error context if plpgsql calls a plperl function that raises
a exception
what will be a error context if plperl calls a plpgsql functions that
raises a exception


I fail to see the point.  How would that be different from what happens 
today?  Remember, PL/PgSQL only suppresses the *topmost* stack frame, 
and only when using RAISE from within a PL/PgSQL function.



.m


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-26 13:39 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 On 1/26/15 1:14 PM, Pavel Stehule wrote:

 2015-01-26 13:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 I can see where it's a lot nicer not to have the context visible for
 people who only care about the contents of the message, but the way it's
 done in PL/PgSQL right now is just not good enough.  On the other hand,
 the
 backwards compatibility breakage of doing this in libpq is quite
 extensive.  The most simple option seems to be to just allow a GUC to
 change PL/PgSQL's behavior to match what all other PLs are doing.


 libpq was changed more time - there is still a open task about a protocol
 change.

 I afraid about some unexpected side effects of your proposal if somebody
 mix languages - these side effects should not be critical


 I have no idea what you're talking about.  What kind of side effects?


what will be a error context if plpgsql calls a plperl function that raises
a exception
what will be a error context if plperl calls a plpgsql functions that
raises a exception


  - but on second
 hand current behave is not critical too - we can wait.


 I think the current behavior is almost unacceptable.  It makes debugging
 in some cases really, really difficult.


if it is necessary, then we can modify libpq

Regards

Pavel





 .marko

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Tom Lane]

Pavel Stehule pavel.steh...@gmail.com writes:
 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
 I am thinking, so solution

  /* if we are doing RAISE, don't report its location */
 if (estate-err_text == raise_skip_msg)
 return;

 is too simple, and this part should be fixed. This change can be done by on
 plpgsql or libpq side. This is bug, and it should be fixed.

Doing this in libpq is utterly insane.  It has not got sufficient context
to do anything intelligent.  The fact that it's not intelligent is exposed
by the regression test changes that the proposed patch causes, most of
which do not look like improvements.

Another problem is that past requests to change this behavior have
generally been to the effect that people wanted *more* context suppressed
not less, ie they didn't want any CONTEXT lines at all on certain
messages.  So the proposed patch seems to me to be going in exactly the
wrong direction.

The design I thought had been agreed on was to add some new option to
plpgsql's RAISE command which would cause suppression of all CONTEXT lines
not just the most closely nested one.  You could argue about whether the
behavior needs to be 100% backwards compatible or not --- if so, perhaps
it could be a three-way option all, none, or one line, defaulting to the
last for backwards compatibility.

regards, tom lane


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-26 16:14 GMT+01:00 Tom Lane t...@sss.pgh.pa.us:

 Pavel Stehule pavel.steh...@gmail.com writes:
  2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:
  I am thinking, so solution

   /* if we are doing RAISE, don't report its location */
  if (estate-err_text == raise_skip_msg)
  return;

  is too simple, and this part should be fixed. This change can be done by
 on
  plpgsql or libpq side. This is bug, and it should be fixed.

 Doing this in libpq is utterly insane.  It has not got sufficient context
 to do anything intelligent.  The fact that it's not intelligent is exposed
 by the regression test changes that the proposed patch causes, most of
 which do not look like improvements.


I don't understand. There can be a overhead due useless transformation some
data to client side. But all what it need - errcontext and errlevel is
possible.


 Another problem is that past requests to change this behavior have
 generally been to the effect that people wanted *more* context suppressed
 not less, ie they didn't want any CONTEXT lines at all on certain
 messages.  So the proposed patch seems to me to be going in exactly the
 wrong direction.

 The design I thought had been agreed on was to add some new option to
 plpgsql's RAISE command which would cause suppression of all CONTEXT lines
 not just the most closely nested one.  You could argue about whether the
 behavior needs to be 100% backwards compatible or not --- if so, perhaps
 it could be a three-way option all, none, or one line, defaulting to the
 last for backwards compatibility.


 I see a problem what should be default behave. When I raise NOTICE, then I
don't need (don't would) to see CONTEXT lines, When I raise EXCEPTION, then
I usually would to see CONTEXT lines.

Cannot be solution?

estate-err_text = stmt-elog_level == ERROR ? estate-err_text :
raise_skip_msg  ;

Regards

Pavel





 regards, tom lane

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-26 14:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 On 1/26/15 1:44 PM, Pavel Stehule wrote:

 2015-01-26 13:39 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 On 1/26/15 1:14 PM, Pavel Stehule wrote:

 I afraid about some unexpected side effects of your proposal if somebody
 mix languages - these side effects should not be critical


 I have no idea what you're talking about.  What kind of side effects?


 what will be a error context if plpgsql calls a plperl function that
 raises
 a exception
 what will be a error context if plperl calls a plpgsql functions that
 raises a exception


 I fail to see the point.  How would that be different from what happens
 today?  Remember, PL/PgSQL only suppresses the *topmost* stack frame, and
 only when using RAISE from within a PL/PgSQL function.


I had to though little bit more - and I am thinking so we should to return
back to start of this thread.

Current state:

1. RAISE in plpgsql doesn't show a context - what we want in RAISE NOTICE
and we don't want in RAISE EXCEPTION

I am thinking, so solution

 /* if we are doing RAISE, don't report its location */
if (estate-err_text == raise_skip_msg)
return;

is too simple, and this part should be fixed. This change can be done by on
plpgsql or libpq side. This is bug, and it should be fixed.

2. Personally I prefer a little bit conceptual solution, that needs a libpq
change because I wish some mode between terse and verbose mode - I would
not to see context for NOTICEs, but I would to see context for errors. This
request is generic - independent on used PL. @2 is my feature request and
it is possible independent on @1.

3. your proposal plpgsql.suppress_simple_error_context fix the @2 partially
- just I prefer a generic solution that will be available for all PL -
exception processing is same for all PL, so filtering should be common too.

Regards

Pavel




 .m

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Jim Nasby]

On 1/26/15 9:46 AM, Pavel Stehule wrote:


The design I thought had been agreed on was to add some new option to
plpgsql's RAISE command which would cause suppression of all CONTEXT lines
not just the most closely nested one.  You could argue about whether the
behavior needs to be 100% backwards compatible or not --- if so, perhaps
it could be a three-way option all, none, or one line, defaulting to the
last for backwards compatibility.


  I see a problem what should be default behave. When I raise NOTICE, then I 
don't need (don't would) to see CONTEXT lines, When I raise EXCEPTION, then I 
usually would to see CONTEXT lines.


FWIW, that's the case I almost always run into: I turn on some debugging which 
means I know where the RAISE is coming from, but now I'm flooded with CONTEXT 
lines. You could do that with an option to RAISE, but that seems like a lot of 
extra coding work for little gain. Perhaps it'd be worth creating 
client_min_context and log_min_context GUCs...

Another option that I think would work well is that you only provide context for the 
first call within a block of code. For plpgsql that would be a function, but 
maybe it'd be better to just do this per-subtransaction.

I do agree that this needs to work across the board, not just for plpgsql.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 1/22/15 6:03 PM, Pavel Stehule wrote:

2015-01-22 12:37 GMT+01:00 Marko Tiikkaja ma...@joh.to:

Or is that a stupid idea?  I just think hacking libpq for something like
this is a huge overkill.



I don't think so only plpgsql  solution is satisfactory idea. There are
some mix plpgsql / plperl ... application - and it isn't possible to remove
error context from only one language.


Yeah, not in libpq it isn't.  Thing is, PL/PgSQL already is the 
exception here, since it's the only language which does this error 
message suppression.  So if people did think this suppression was a good 
idea, only the people using PL/PgSQL were vocal enough to get the 
behavior changed.  I'm not looking to change that.


I can see where it's a lot nicer not to have the context visible for 
people who only care about the contents of the message, but the way it's 
done in PL/PgSQL right now is just not good enough.  On the other hand, 
the backwards compatibility breakage of doing this in libpq is quite 
extensive.  The most simple option seems to be to just allow a GUC to 
change PL/PgSQL's behavior to match what all other PLs are doing.



.marko


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-26 13:02 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 On 1/22/15 6:03 PM, Pavel Stehule wrote:

 2015-01-22 12:37 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 Or is that a stupid idea?  I just think hacking libpq for something like
 this is a huge overkill.


 I don't think so only plpgsql  solution is satisfactory idea. There are
 some mix plpgsql / plperl ... application - and it isn't possible to
 remove
 error context from only one language.


 Yeah, not in libpq it isn't.  Thing is, PL/PgSQL already is the exception
 here, since it's the only language which does this error message
 suppression.  So if people did think this suppression was a good idea, only
 the people using PL/PgSQL were vocal enough to get the behavior changed.
 I'm not looking to change that.


 I can see where it's a lot nicer not to have the context visible for
 people who only care about the contents of the message, but the way it's
 done in PL/PgSQL right now is just not good enough.  On the other hand, the
 backwards compatibility breakage of doing this in libpq is quite
 extensive.  The most simple option seems to be to just allow a GUC to
 change PL/PgSQL's behavior to match what all other PLs are doing.



libpq was changed more time - there is still a open task about a protocol
change.

I afraid about some unexpected side effects of your proposal if somebody
mix languages - these side effects should not be critical - but on second
hand current behave is not critical too - we can wait.




 .marko

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

Hello,

I just heard that there's going to be a fifth CF for 9.5 so I'm trying 
to gather all the patches I'd like to see in 9.5..


On 8/23/13 10:36 AM, I wrote:

My opinion at this very moment is that we should leave the the DEFAULT
verbosity alone and add a new one (call it COMPACT or such) with the
suppressed context for non-ERRORs.


I wonder if a better option would be to add a GUC to control this from 
the server side.  plpgsql.suppress_simple_error_context or such, 
defaulting to false to maintain full backwards compatibility.  That 
could be set to true for development installations and for client 
programs which only care about having all information available, rather 
than readability or aesthetics.


Or is that a stupid idea?  I just think hacking libpq for something like 
this is a huge overkill.



.marko


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2015-01-22 12:37 GMT+01:00 Marko Tiikkaja ma...@joh.to:

 Hello,

 I just heard that there's going to be a fifth CF for 9.5 so I'm trying to
 gather all the patches I'd like to see in 9.5..

 On 8/23/13 10:36 AM, I wrote:

 My opinion at this very moment is that we should leave the the DEFAULT
 verbosity alone and add a new one (call it COMPACT or such) with the
 suppressed context for non-ERRORs.


 I wonder if a better option would be to add a GUC to control this from the
 server side.  plpgsql.suppress_simple_error_context or such, defaulting
 to false to maintain full backwards compatibility.  That could be set to
 true for development installations and for client programs which only care
 about having all information available, rather than readability or
 aesthetics.

 Or is that a stupid idea?  I just think hacking libpq for something like
 this is a huge overkill.


I don't think so only plpgsql  solution is satisfactory idea. There are
some mix plpgsql / plperl ... application - and it isn't possible to remove
error context from only one language.

Regards

Pavel




 .marko

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Jeevan Chalke]

Hi Marko,

I have reviewed this patch.

1. Patch applies well.
2. make and make install is fine
3. make check is fine too.

But as Peter pointed out plperl regression tests are failing.

I just did grep on .sql files and found following files which has RAISE
statement into it. These files too need expected output changes. Please run
these testcases to get diffs.

./src/pl/plperl/sql/plperl_elog.sql
./src/pl/plpython/sql/plpython_error.sql
./src/pl/plpython/sql/plpython_setof.sql
./src/pl/plpython/sql/plpython_quote.sql
./contrib/sepgsql/sql/label.sql
./contrib/sepgsql/sql/ddl.sql

Code changes looks fine to me.

Thanks


-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 15/09/2013 13:50, I wrote:

On 15/09/2013 04:05, Peter Eisentraut wrote:

On Sat, 2013-09-14 at 04:58 +0200, Marko Tiikkaja wrote:

The attached patch (based on Pavel's patch) changes the default to be
slightly more verbose (the CONTEXT lines which were previously
omitted
will be visible), but adds a new PGVerbosity called COMPACT which
suppresses CONTEXT in non-error messages.  Now DEFAULT will be more
useful when debugging PL/PgSQL, and people who are annoyed by the new
behaviour can use the COMPACT mode.


Your patch fails the regression tests.


Attached is an updated patch with the regression test fixes.  No other
changes included.


Hmm.  I just noticed there's something weird going on in the select_view 
test.  I'm investigating this currently.



Regards,
Marko Tiikkaja


--
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] PL/pgSQL, RAISE and error context

[Peter Eisentraut]

On Sun, 2013-09-15 at 14:28 +0200, Marko Tiikkaja wrote:
 On 15/09/2013 13:58, I wrote:
  Hmm.  I just noticed there's something weird going on in the select_view
  test.  I'm investigating this currently.
 
 Seems that there's some magic going on and I overwrote the expected 
 results of the wrong file.  However, I can't figure out how one is 
 supposed to be getting the output of expected/select_views.out, nor do I 
 find this documented anywhere (I know xml has a similar thing so I tried 
 grepping around for XML, to no avail).
 
 Here's an updated patch, but I think expected/select_views.out is still 
 broken.

You patch still fails the plperl regression tests.

I don't see a failure with select_views.  Your issue might be that you
updated expected/select_views_1.out but not expected/select_views.out.
This documentation might help:
http://www.postgresql.org/docs/devel/static/regress-variant.html



-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/9/14 Marko Tiikkaja ma...@joh.to

 On 23/08/2013 10:36, I wrote:

 On 8/23/13 8:38 AM, Pavel Stehule wrote:

 do you prepare patch ?


 I should have the time to produce one for the September commitfest, but
 if you (or anyone else) want to work on this, I won't object.

 My opinion at this very moment is that we should leave the the DEFAULT
 verbosity alone and add a new one (call it COMPACT or such) with the
 suppressed context for non-ERRORs.


 Well, turns out there isn't really any way to preserve complete backwards
 compatibility if we want to do this change.

 The attached patch (based on Pavel's patch) changes the default to be
 slightly more verbose (the CONTEXT lines which were previously omitted will
 be visible), but adds a new PGVerbosity called COMPACT which suppresses
 CONTEXT in non-error messages.  Now DEFAULT will be more useful when
 debugging PL/PgSQL, and people who are annoyed by the new behaviour can use
 the COMPACT mode.

 Any thoughts?


+1

Regards

Pavel




 Regards,
 Marko Tiikkaja

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Peter Eisentraut]

On Sat, 2013-09-14 at 04:58 +0200, Marko Tiikkaja wrote:
 The attached patch (based on Pavel's patch) changes the default to be 
 slightly more verbose (the CONTEXT lines which were previously
 omitted 
 will be visible), but adds a new PGVerbosity called COMPACT which 
 suppresses CONTEXT in non-error messages.  Now DEFAULT will be more 
 useful when debugging PL/PgSQL, and people who are annoyed by the new 
 behaviour can use the COMPACT mode. 

Your patch fails the regression tests.



-- 
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 23/08/2013 10:36, I wrote:

On 8/23/13 8:38 AM, Pavel Stehule wrote:

do you prepare patch ?


I should have the time to produce one for the September commitfest, but
if you (or anyone else) want to work on this, I won't object.

My opinion at this very moment is that we should leave the the DEFAULT
verbosity alone and add a new one (call it COMPACT or such) with the
suppressed context for non-ERRORs.


Well, turns out there isn't really any way to preserve complete 
backwards compatibility if we want to do this change.


The attached patch (based on Pavel's patch) changes the default to be 
slightly more verbose (the CONTEXT lines which were previously omitted 
will be visible), but adds a new PGVerbosity called COMPACT which 
suppresses CONTEXT in non-error messages.  Now DEFAULT will be more 
useful when debugging PL/PgSQL, and people who are annoyed by the new 
behaviour can use the COMPACT mode.


Any thoughts?



Regards,
Marko Tiikkaja

*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
***
*** 5418,5423  int PQsetClientEncoding(PGconn 
*replaceableconn/replaceable, const char *re
--- 5418,5424 
  typedef enum
  {
  PQERRORS_TERSE,
+ PQERRORS_COMPACT,
  PQERRORS_DEFAULT,
  PQERRORS_VERBOSE
  } PGVerbosity;
***
*** 5430,5439  PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity 
verbosity);
returned messages include severity, primary text, and position only;
this will normally fit on a single line.  The default mode produces
messages that include the above plus any detail, hint, or context
!   fields (these might span multiple lines).  The firsttermVERBOSE/
!   mode includes all available fields.  Changing the verbosity does not
!   affect the messages available from already-existing
!   structnamePGresult/ objects, only subsequently-created ones.
   /para
  /listitem
 /varlistentry
--- 5431,5442 
returned messages include severity, primary text, and position only;
this will normally fit on a single line.  The default mode produces
messages that include the above plus any detail, hint, or context
!   fields (these might span multiple lines).  The COMPACT mode is otherwise
!   the same as the default, except the context field will be omitted for
!   non-error messages.  The firsttermVERBOSE/ mode includes all
!   available fields.  Changing the verbosity does not affect the messages
!   available from already-existing structnamePGresult/ objects, only
!   subsequently-created ones.
   /para
  /listitem
 /varlistentry
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
***
*** 796,801  verbosity_hook(const char *newval)
--- 796,803 
pset.verbosity = PQERRORS_DEFAULT;
else if (strcmp(newval, terse) == 0)
pset.verbosity = PQERRORS_TERSE;
+   else if (strcmp(newval, compact) == 0)
+   pset.verbosity = PQERRORS_COMPACT;
else if (strcmp(newval, verbose) == 0)
pset.verbosity = PQERRORS_VERBOSE;
else
*** a/src/interfaces/libpq/fe-protocol3.c
--- b/src/interfaces/libpq/fe-protocol3.c
***
*** 915,920  pqGetErrorNotice3(PGconn *conn, bool isError)
--- 915,924 
if (val)
appendPQExpBuffer(workBuf, libpq_gettext(QUERY:  
%s\n), val);
val = PQresultErrorField(res, PG_DIAG_CONTEXT);
+   }
+   if (isError || (conn-verbosity != PQERRORS_TERSE 
+   conn-verbosity != PQERRORS_COMPACT))
+   {
if (val)
appendPQExpBuffer(workBuf, libpq_gettext(CONTEXT:  
%s\n), val);
}
*** a/src/interfaces/libpq/libpq-fe.h
--- b/src/interfaces/libpq/libpq-fe.h
***
*** 106,111  typedef enum
--- 106,112 
  typedef enum
  {
PQERRORS_TERSE, /* single-line error messages */
+   PQERRORS_COMPACT,   /* single-line error messages 
on non-error messags */
PQERRORS_DEFAULT,   /* recommended style */
PQERRORS_VERBOSE/* all the facts, ma'am */
  } PGVerbosity;
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***
*** 39,46 
  #include utils/typcache.h
  
  
- static const char *const raise_skip_msg = RAISE;
- 
  typedef struct
  {
int nargs;  /* number of arguments 
*/
--- 39,44 
***
*** 867,876  plpgsql_exec_error_callback(void *arg)
  {
PLpgSQL_execstate *estate = (PLpgSQL_execstate *) arg;
  
-   /* if we are doing RAISE, don't report its location */
-   if (estate-err_text == raise_skip_msg)
-   return;
- 
if (estate-err_text != NULL)
{
/*
--- 865,870 
***
*** 3032,3038  

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/8/22 Marko Tiikkaja ma...@joh.to

 On 8/22/13 9:08 AM, Pavel Stehule wrote:

 Probably we can introduce a new level of verbosity, but I am thinking so
 this behave is reasonable. Everybody who use a VERBOSE level expect lot of
 balast and it show expected info (context of error)

 Can be this design good enough for you?


 I like the idea, but I think this should be a new verbosity level.  With
 this patch you would have to go full VERBOSE just to debug PL/pgSQL code
 with NOTICEs and DEBUGs in it, and that output then becomes harder to parse
 with the useless C-code information.


do you prepare patch ?

Pavel




 Regards,
 Marko Tiikkaja

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/23/13 8:38 AM, Pavel Stehule wrote:

2013/8/22 Marko Tiikkaja ma...@joh.to

I like the idea, but I think this should be a new verbosity level.  With
this patch you would have to go full VERBOSE just to debug PL/pgSQL code
with NOTICEs and DEBUGs in it, and that output then becomes harder to parse
with the useless C-code information.



do you prepare patch ?


I should have the time to produce one for the September commitfest, but 
if you (or anyone else) want to work on this, I won't object.


My opinion at this very moment is that we should leave the the DEFAULT 
verbosity alone and add a new one (call it COMPACT or such) with the 
suppressed context for non-ERRORs.



Regards,
Marko Tiikkaja


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/8/23 Marko Tiikkaja ma...@joh.to

 On 8/23/13 8:38 AM, Pavel Stehule wrote:

  2013/8/22 Marko Tiikkaja ma...@joh.to

 I like the idea, but I think this should be a new verbosity level.  With
 this patch you would have to go full VERBOSE just to debug PL/pgSQL code
 with NOTICEs and DEBUGs in it, and that output then becomes harder to
 parse
 with the useless C-code information.


 do you prepare patch ?


 I should have the time to produce one for the September commitfest, but if
 you (or anyone else) want to work on this, I won't object.

 My opinion at this very moment is that we should leave the the DEFAULT
 verbosity alone and add a new one (call it COMPACT or such) with the
 suppressed context for non-ERRORs.



The name  is not important. What I would, for DEFAULT verbosity, to see a
context when RAISE EXCEPTION is used. It is a bug now, I think

Regards

Pavel




 Regards,
 Marko Tiikkaja

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hello

I played with this topic little bit

If I understand, the main problem is in console (or pgAdmin) output.

create or replace function foo()
returns void as $$
begin
  for i in 1..5
  loop
raise notice ' *';
  end loop;
  raise exception '***';
end;
$$ language plpgsql;

postgres=# select foo();
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
ERROR:  ***
Time: 2.024 ms
postgres=# \set VER
VERBOSITY  VERSION
postgres=# \set VERBOSITY

postgres=# \set VERBOSITY

postgres=# \set VERBOSITY terse
postgres=# select foo();
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
ERROR:  ***
Time: 0.908 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
NOTICE:  0:  *
LOCATION:  exec_stmt_raise, pl_exec.c:3051
NOTICE:  0:  *
LOCATION:  exec_stmt_raise, pl_exec.c:3051
NOTICE:  0:  *
LOCATION:  exec_stmt_raise, pl_exec.c:3051
NOTICE:  0:  *
LOCATION:  exec_stmt_raise, pl_exec.c:3051
NOTICE:  0:  *
LOCATION:  exec_stmt_raise, pl_exec.c:3051
ERROR:  P0001: ***
LOCATION:  exec_stmt_raise, pl_exec.c:3051

Time: 0.314 ms

I see a two little bit not nice issues:

a) in terse mode missing a CONTEXT for RAISED error
b) in verbose mode missing a CONTEXT for messages, for error too, and
useless LOCATION is showed.

LOCATION is absolutely useless for custom messages.

so I removed a context filtering

 postgres=# select foo();
NOTICE:   *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
NOTICE:   *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
NOTICE:   *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
NOTICE:   *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
NOTICE:   *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
ERROR:  ***
CONTEXT:  PL/pgSQL function foo() line 7 at RAISE
Time: 3.842 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
ERROR:  P0001: ***
CONTEXT:  PL/pgSQL function foo() line 7 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
Time: 0.761 ms

We should not see a CONTEXT for DEFAULT verbosity and NOTICE level, after
little bit change I got a satisfied output


postgres=# select foo();
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
NOTICE:   *
ERROR:  ***
CONTEXT:  PL/pgSQL function foo() line 7 at RAISE
Time: 2.434 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
NOTICE:  0:  *
CONTEXT:  PL/pgSQL function foo() line 5 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
ERROR:  P0001: ***
CONTEXT:  PL/pgSQL function foo() line 7 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3046
Time: 0.594 ms

Probably we can introduce a new level of verbosity, but I am thinking so
this behave is reasonable. Everybody who use a VERBOSE level expect lot of
balast and it show expected info (context of error)

Can be this design good enough for you?

Regards

Pavel


plpgsql_raise_context.patch
Description: Binary data

-- 
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] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Thu, Aug 22, 2013 at 2:08 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 Probably we can introduce a new level of verbosity, but I am thinking so
 this behave is reasonable. Everybody who use a VERBOSE level expect lot of
 balast and it show expected info (context of error)

 Can be this design good enough for you?

yep :-).

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/22/13 9:08 AM, Pavel Stehule wrote:

Probably we can introduce a new level of verbosity, but I am thinking so
this behave is reasonable. Everybody who use a VERBOSE level expect lot of
balast and it show expected info (context of error)

Can be this design good enough for you?


I like the idea, but I think this should be a new verbosity level.  With 
this patch you would have to go full VERBOSE just to debug PL/pgSQL code 
with NOTICEs and DEBUGs in it, and that output then becomes harder to 
parse with the useless C-code information.



Regards,
Marko Tiikkaja



--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/8/22 Marko Tiikkaja ma...@joh.to

 On 8/22/13 9:08 AM, Pavel Stehule wrote:

 Probably we can introduce a new level of verbosity, but I am thinking so
 this behave is reasonable. Everybody who use a VERBOSE level expect lot of
 balast and it show expected info (context of error)

 Can be this design good enough for you?


 I like the idea, but I think this should be a new verbosity level.  With
 this patch you would have to go full VERBOSE just to debug PL/pgSQL code
 with NOTICEs and DEBUGs in it, and that output then becomes harder to parse
 with the useless C-code information.



word DEBUG is not good - it is used for Postgres debugging as log level

Pavel



 Regards,
 Marko Tiikkaja

[HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

Hi,

By default, PL/pgSQL does not print the error context of a RAISE 
statement, for example:


=# create function foof() returns void as $$ begin raise exception 
'foo'; end $$ language plpgsql;

CREATE FUNCTION

=# create function bar() returns void as $$ begin perform foof(); end $$ 
language plpgsql;

CREATE FUNCTION

=# select bar();
ERROR:  foo
CONTEXT:  SQL statement SELECT foof()
PL/pgSQL function bar line 1 at PERFORM


I find this extremely surprising, since if you raise the same exception 
(or a DEBUG/NOTICE message) in multiple places, the error context is 
missing valuable information.  With a trivial change the last error 
could be:


=# select bar();
ERROR:  foo
CONTEXT:  PL/pgSQL function foof line 1 RAISE
SQL statement SELECT foof()
PL/pgSQL function bar line 1 at PERFORM

which I find a lot better.

Thoughts?


Regards,
Marko Tiikkaja


--
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/8/21 Marko Tiikkaja ma...@joh.to

 Hi,

 By default, PL/pgSQL does not print the error context of a RAISE
 statement, for example:

 =# create function foof() returns void as $$ begin raise exception 'foo';
 end $$ language plpgsql;
 CREATE FUNCTION

 =# create function bar() returns void as $$ begin perform foof(); end $$
 language plpgsql;
 CREATE FUNCTION

 =# select bar();
 ERROR:  foo
 CONTEXT:  SQL statement SELECT foof()
 PL/pgSQL function bar line 1 at PERFORM


 I find this extremely surprising, since if you raise the same exception
 (or a DEBUG/NOTICE message) in multiple places, the error context is
 missing valuable information.  With a trivial change the last error could
 be:

 =# select bar();
 ERROR:  foo
 CONTEXT:  PL/pgSQL function foof line 1 RAISE
 SQL statement SELECT foof()
 PL/pgSQL function bar line 1 at PERFORM

 which I find a lot better.


+1

Pavel


 Thoughts?


 Regards,
 Marko Tiikkaja


 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/**mailpref/pgsql-hackershttp://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/21/13 2:28 PM, I wrote:

By default, PL/pgSQL does not print the error context of a RAISE
statement, for example:


An even worse example:

=# create function foof() returns void as $$ begin raise exception 
'foo'; end $$ language plpgsql;

CREATE FUNCTION

=# create function barf() returns void as $$ declare _ record; begin for 
_ in execute 'select foof()' loop end loop; end $$ language plpgsql;

CREATE FUNCTION

=# select barf();
ERROR:  foo
CONTEXT:  PL/pgSQL function barf line 1 at FOR over EXECUTE statement

Notice how there's no mention at all about the function the error came 
from, and compare that to:


=# select barf();
ERROR:  foo
CONTEXT:  PL/pgSQL function foof line 1 RAISE
PL/pgSQL function barf line 1 at FOR over EXECUTE statement



Regards,
Marko Tiikkaja


--
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] PL/pgSQL, RAISE and error context

[Tom Lane]

Marko Tiikkaja ma...@joh.to writes:
 By default, PL/pgSQL does not print the error context of a RAISE 
 statement, for example:

It used to do so, in the beginning when we first added context-printing.
There were complaints that the result was too verbose; for instance if you
had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
get two lines for every one you wanted.  I think if we undid this we'd
get the same complaints again.  I agree that in complicated nests of
functions the location info is more interesting than it is in trivial
cases, but that doesn't mean you're not going to hear such complaints from
people with trivial functions.

regards, tom lane


-- 
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/21/13 4:22 PM, Tom Lane wrote:

Marko Tiikkaja ma...@joh.to writes:

By default, PL/pgSQL does not print the error context of a RAISE
statement, for example:


It used to do so, in the beginning when we first added context-printing.
There were complaints that the result was too verbose; for instance if you
had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
get two lines for every one you wanted.  I think if we undid this we'd
get the same complaints again.  I agree that in complicated nests of
functions the location info is more interesting than it is in trivial
cases, but that doesn't mean you're not going to hear such complaints from
people with trivial functions.


They have an option: they can reduce verbosity in their client.  I 
currently don't have any real options.



Regards,
Marko Tiikkaja


--
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] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Marko Tiikkaja ma...@joh.to writes:
 By default, PL/pgSQL does not print the error context of a RAISE
 statement, for example:

 It used to do so, in the beginning when we first added context-printing.
 There were complaints that the result was too verbose; for instance if you
 had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
 get two lines for every one you wanted.  I think if we undid this we'd
 get the same complaints again.  I agree that in complicated nests of
 functions the location info is more interesting than it is in trivial
 cases, but that doesn't mean you're not going to hear such complaints from
 people with trivial functions.

It *is* (apologies for the hijack) too verbose but whatever context
suppressing we added doesn't work in pretty much any interesting case.
 What is basically needed is for the console to honor
log_error_verbosity (which I would prefer) or a separate GUC in manage
the console logging verbosity:

set log_error_verbosity = 'terse';
SET

CREATE OR REPLACE FUNCTION Notice(_msg TEXT) RETURNS VOID AS
$$
BEGIN
  RAISE NOTICE '[%] %', clock_timestamp()::timestamp(0)::text, _msg;
END;
$$ LANGUAGE PLPGSQL;

CREATE OR REPLACE FUNCTION foo() RETURNS VOID AS
$$
BEGIN
  PERFORM Notice('test');
END;
$$ LANGUAGE PLPGSQL;

-- context will print
postgres=# select foo();
NOTICE:  [2013-08-21 09:52:08] test
CONTEXT:  SQL statement SELECT Notice('test')
PL/pgSQL function foo() line 4 at PERFORM

CREATE OR REPLACE FUNCTION bar() RETURNS VOID AS
$$
  SELECT Notice('test');
$$ LANGUAGE SQL;

-- context will not print
postgres=# select bar();
NOTICE:  [2013-08-21 09:54:55] test

-- context will  print
CREATE OR REPLACE FUNCTION baz() RETURNS VOID AS
$$
  select 0;
  SELECT Notice('test');
$$ LANGUAGE SQL;

postgres=# select baz();
NOTICE:  [2013-08-21 09:55:26] test
CONTEXT:  SQL function baz statement 2

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 8/21/13 5:05 PM, Merlin Moncure wrote:

On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote:

Marko Tiikkaja ma...@joh.to writes:

By default, PL/pgSQL does not print the error context of a RAISE
statement, for example:


It used to do so, in the beginning when we first added context-printing.
There were complaints that the result was too verbose; for instance if you
had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
get two lines for every one you wanted.  I think if we undid this we'd
get the same complaints again.  I agree that in complicated nests of
functions the location info is more interesting than it is in trivial
cases, but that doesn't mean you're not going to hear such complaints from
people with trivial functions.


It *is* (apologies for the hijack) too verbose but whatever context
suppressing we added doesn't work in pretty much any interesting case.
  What is basically needed is for the console to honor
log_error_verbosity (which I would prefer) or a separate GUC in manage
the console logging verbosity:


Why does  \set VERBOSITY 'terse'  not work for you?



Regards,
Marko Tiikkaja



--
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] PL/pgSQL, RAISE and error context

[Merlin Moncure]

On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote:
 On 8/21/13 5:05 PM, Merlin Moncure wrote:

 On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 Marko Tiikkaja ma...@joh.to writes:

 By default, PL/pgSQL does not print the error context of a RAISE
 statement, for example:


 It used to do so, in the beginning when we first added context-printing.
 There were complaints that the result was too verbose; for instance if
 you
 had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
 get two lines for every one you wanted.  I think if we undid this we'd
 get the same complaints again.  I agree that in complicated nests of
 functions the location info is more interesting than it is in trivial
 cases, but that doesn't mean you're not going to hear such complaints
 from
 people with trivial functions.


 It *is* (apologies for the hijack) too verbose but whatever context
 suppressing we added doesn't work in pretty much any interesting case.
   What is basically needed is for the console to honor
 log_error_verbosity (which I would prefer) or a separate GUC in manage
 the console logging verbosity:


 Why does  \set VERBOSITY 'terse'  not work for you?

Because it can't be controlled mid-function...that would suppress all
context of errors as well as messages and so it's useless.  Also psql
directives for this purpose is a hack anyways -- what if I'm using a
non-psql client?

what I really want is:
SET LOCAL log_console_verbosity = 'x'

merlin


-- 
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] PL/pgSQL, RAISE and error context

[Pavel Stehule]

2013/8/21 Merlin Moncure mmonc...@gmail.com

 On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote:
  On 8/21/13 5:05 PM, Merlin Moncure wrote:
 
  On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 
  Marko Tiikkaja ma...@joh.to writes:
 
  By default, PL/pgSQL does not print the error context of a RAISE
  statement, for example:
 
 
  It used to do so, in the beginning when we first added
 context-printing.
  There were complaints that the result was too verbose; for instance if
  you
  had a RAISE NOTICE inside a loop for progress-monitoring purposes,
 you'd
  get two lines for every one you wanted.  I think if we undid this we'd
  get the same complaints again.  I agree that in complicated nests of
  functions the location info is more interesting than it is in trivial
  cases, but that doesn't mean you're not going to hear such complaints
  from
  people with trivial functions.
 
 
  It *is* (apologies for the hijack) too verbose but whatever context
  suppressing we added doesn't work in pretty much any interesting case.
What is basically needed is for the console to honor
  log_error_verbosity (which I would prefer) or a separate GUC in manage
  the console logging verbosity:
 
 
  Why does  \set VERBOSITY 'terse'  not work for you?

 Because it can't be controlled mid-function...that would suppress all
 context of errors as well as messages and so it's useless.  Also psql
 directives for this purpose is a hack anyways -- what if I'm using a
 non-psql client?

 what I really want is:
 SET LOCAL log_console_verbosity = 'x'


it is not bad idea

Pavel


  merlin


 --
 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] PL/pgSQL, RAISE and error context

[Marko Tiikkaja]

On 2013-08-21 17:18, Merlin Moncure wrote:

On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote:

Why does  \set VERBOSITY 'terse'  not work for you?


Because it can't be controlled mid-function...that would suppress all
context of errors as well as messages and so it's useless.


Fair enough.


what I really want is:
SET LOCAL log_console_verbosity = 'x'


log_min_messages vs. client_min_messages, so client_error_verbosity 
sounds more appropriate IMO.



Regards,
Marko Tiikkaja


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers