Re: proposal: PL/Pythonu - function ereport

Hi

2015-12-08 7:06 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > Don't understand - if Fatal has same behave as Error, then why it cannot
> be
> > inherited from Error?
> >
> > What can be broken?
>
> Existing code that did "except plpy.Error as exc" will now also be
> called if plpy.Fatal is raised. That wasn't the case so this changes
> meaning of existing code, therefore it introduces an incompatibility.
>

yes, there should be some common ancestor for plpy.Error and plpy.Fatal

Have you any idea, how this ancestor should be named?

> >>
> >> 5. all the reporting functions: plpy.debug...plpy.fatal functions
> >> should also be changed to allow more arguments than the message and
> >> allow them as keyword arguments
> >
> >
> > this is maybe bigger source of broken compatibility
> >
> > lot of people uses plpy.debug(var1, var2, var3)
> >
> > rich constructor use $1 for message, $2 for detail, $3 for hint. So it
> was a
> > reason, why didn't touch these functions
>
> No, if you use PyArg_ParseTupleAndKeywords you'll have Python accept
> all of these so previous ways are still accepted:
>
> plpy.error('a_msg', 'a_detail', 'a_hint')
> plpy.error'a_msg', 'a_detail', hint='a_hint')
> plpy.error('a_msg', detail='a_detail', hint='a_hint')
> plpy.error(msg='a_msg', detail='a_detail', hint='a_hint')
> plpy.error('a_msg')
> plpy.error(msg='a_msg')
> etc.
>
> But I now see that even though the docs say plpy.error and friends
> take a single msg argument, they actually allow any number of
> arguments. If multiple arguments are passed they are converted to a
> tuple and the string representation of that tuple goes into
> ereport/plpy.Error:
>
> CREATE FUNCTION test()
> RETURNS int
> AS $$
> try:
> plpy.error('an error message', 'detail for error', 'hint for error')
> except plpy.Error as exc:
> plpy.info('have exc {}'.format(exc))
> plpy.info('have exc.args |{}| of type {}'.format(exc.args,
> type(exc.args)))
> $$ LANGUAGE plpython3u;
> SELECT test();
> [catalin(at)archie pg]$ psql -f plpy test
> CREATE FUNCTION
> psql:plpy:13: INFO: have exc ('an error message', 'detail for error',
> 'hint for error')
> psql:plpy:13: INFO: have exc.args |("('an error message', 'detail for
> error', 'hint for error')",)| of type <class 'tuple'>
>
> In the last line note that exc.args (the args tuple passed in the
> constructor of plpy.Error) is a tuple with a single element which is a
> string which is a representation of the tuple passed into plpy.error.
> Don't know why this was thought useful, it was certainly surprising to
> me. Anyway, the separate $2, $3 etc are currently not detail and hint,
> they're just more stuff that gets appended to this tuple. They don't
> get passed to clients as hints. And you can pass lots of them not just
> detail and hint.
>

using tuple is less work for you, you don't need to concat more values into
one. I don't know, how often this technique is used - probably it has sense
only for NOTICE and lower levels - for adhoc debug messages. Probably not
used elsewhere massively.

>
> My proposal would change the meaning of this to actually interpret the
> second argument as detail, third as hint and to only allow a limited
> number (the ones with meaning to ereport). The hint would go to
> ereport so it would be a real hint: it would go to clients as HINT and
> so on. I think that's way more useful that what is done now. But I now
> see my proposal is also backward incompatible.
>

It was my point

>
> > I am not against to plpy.ereport function - it can live together with
> rich
> > exceptions class, and users can use what they prefer.
>
> I wasn't suggesting introducing ereport, I was saying the existing
> functions and exceptions are very similar to your proposed ereport.
> Enhancing them to take keyword arguments would make them a bit more
> powerful. Adding ereport would be another way of doing the same thing
> so that's more confusing than useful.
>

ok

>
> All in all it's hard to improve this cleanly. I'm still not sure the
> latest patch is a good idea but now I'm also not sure what I proposed
> is better than leaving it as it is.
>

We can use different names, we should not to implement all changes at once.

My main target is possibility to raise rich exception instead dirty
workaround. Changing current functions isn't necessary - although if we are
changing API, is better to do it once.

Regards

Pavel