Re: proposal: PL/Pythonu - function ereport

On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule <pavel.stehule@gmail.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.

>>
>> 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@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.

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.

> 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.

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.