Re: PL/pgSQL, RAISE and error context

Hi

second version of this patch

make check-world passed

Regards

Pavel

2015-07-08 23:05 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> 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 <mmoncure(at)gmail(dot)com>:
>
>> On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> >
>> > 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> >>
>> >> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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 <hlinnaka(at)iki(dot)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 difficult in definition what plpgsql.min_context should to
>> really do
>> > - and what is relation to client_min_context and log_min_context, but I
>> can
>> > prepare configuration, that is fully compatible.
>> >
>> > Comments, any other ideas?
>> >
>> > Personally, I prefer @1 as general solution, that will work for all PL
>>
>> +1
>>
>> merlin
>>
>
>