Re: proposal: PL/Pythonu - function ereport

<p dir="ltr">hi<p dir="ltr">I am sorry, I am writing from mobile phone and I cannot to cutting text well.<p
dir="ltr">Dne29. 1. 2016 18:09 napsal uživatel "Catalin Iacob" <<a
href="mailto:iacobcatalin@gmail.com">iacobcatalin@gmail.com</a>>:<br/> This interface is new generation of
previous<br/> > > functions: info, notice, warning, error with keyword parameters interface. I<br /> > >
didn'tchanged older functions due keeping compatibility.<br /> ><br /> > Hi,<br /> ><br /> > Even without
theOOP changes, the exception classes are still there as<br /> > the underlying mechanism that error and raise_error
use.<br/> ><br /> > I looked at the patch and what I don't like about it is that<br /> > raise_error uses
SPIErrorto transport detail, hint etc while error<br /> > uses Error. This is inconsistent and confusing.<br />
><br/> > Take this example:<br /> ><br /> > CREATE OR REPLACE FUNCTION err()<br /> >   RETURNS int<br />
>AS $$<br /> >   plpy.error('only msg from plpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also
intuple')<br /> > $$ LANGUAGE plpython3u;<br /> ><br /> > SELECT err();<br /> ><br /> > CREATE OR
REPLACEFUNCTION raise_err()<br /> >   RETURNS int<br /> > AS $$<br /> >   plpy.raise_error('only msg from
plpy.raise_error','arg2 for<br /> > raise_error is detail', 'arg3 is hint')<br /> > $$ LANGUAGE plpython3u;<br />
><br/> > SELECT raise_err();<br /> ><br /> > With your patch, this results in:<br /> ><br /> > CREATE
FUNCTION<br/> > psql:plpy_error_raise_error_difference:9: ERROR:  plpy.Error: ('only<br /> > msg from
plpy.error','arg2 for error is part of tuple', 'arg3 also in<br /> > tuple')<br /> > CONTEXT:  Traceback (most
recentcall last):<br /> >   PL/Python function "err", line 2, in <module><br /> >     plpy.error('only msg
fromplpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also in tuple')<br /> > PL/Python function
"err"<br/> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:17: ERROR:  plpy.SPIError: only<br />
>msg from plpy.raise_error<br /> > DETAIL:  arg2 for raise_error is detail<br /> > HINT:  arg3 is hint<br />
>CONTEXT:  Traceback (most recent call last):<br /> >   PL/Python function "raise_err", line 2, in
<module><br/> >     plpy.raise_error('only msg from plpy.raise_error', 'arg2 for<br /> > raise_error is
detail','arg3 is hint')<br /> > PL/Python function "raise_err"<br /> ><br /> > From the output you can already
seethat plpy.error and<br /> > plpy.raise_error (even with a single argument) don't use the same<br /> >
exception:plpy.error uses Error while raise_error uses SPIError. I<br /> > think with a single argument they should
beidentical and with<br /> > multiple arguments raise_error should still use Error but use the<br /> > arguments
asdetail, hint etc. In code you had to export a function to<br /> > plpy_spi.h to get to the details in SPIError
whileplpy.error worked<br /> > just fine without that.<br /> ><p dir="ltr">yes, using SPIError is unhappy, I used
it,because this exception supported structured exceptions already, but In python code it has not sense and raising
Erroris better.<p dir="ltr">> I've attached two patches on top of yours: first is a comment fix, the<br /> > next
oneis a rough proof of concept for using plpy.Error to carry the<br /> > details. This allows undoing the change to
export<br/> > PLy_spi_exception_set to plpy_spi.h. And then I realized, the changes<br /> > you did to SPIError
areactually a separate enhancement (report more<br /> > details like schema name, table name and so on for the query
executed<br/> > by SPI). They should be split into a separate patch. <p dir="ltr">This patch is simple, and
maintainingmore patches has not sense.<br /><p dir="ltr">With these<br /> > patches the output of the above test
is:<br/> ><br /> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:9: ERROR:  plpy.Error:
('only<br/> > msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in<br /> > tuple')<br /> >
CONTEXT: Traceback (most recent call last):<br /> >   PL/Python function "err", line 2, in <module><br /> >
   plpy.error('only msg from plpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also in tuple')<br />
>PL/Python function "err"<br /> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:17: ERROR: 
plpy.Error:only<br /> > msg from plpy.raise_error<br /> > DETAIL:  arg2 for raise_error is detail<br /> >
HINT: arg3 is hint<br /> > CONTEXT:  Traceback (most recent call last):<br /> >   PL/Python function "raise_err",
line2, in <module><br /> >     plpy.raise_error('only msg from plpy.raise_error', 'arg2 for<br /> >
raise_erroris detail', 'arg3 is hint')<br /> > PL/Python function "raise_err"<br /> ><br /> > The two
approachesare consistent now, both create a plpy.Error. The<br /> > patch is not complete, it only handles detail
andhint not the others<br /> > but it should illustrate the idea.<br /> ><br /> > Looking at the output above,
Idon't see who would rely on calling<br /> > plpy.error with multiple arguments and getting the tuple so I'm<br />
>actually in favor of just breaking backward compatibility. Note that<br /> > passing multiple arguments isn't
evendocumented. So I would just<br /> > change debug, info, error and friends to do what raise_debug,<br /> >
raise_info,raise_error do. With a single argument behavior stays the<br /> > same, with multiple arguments one gets
moreuseful behavior (detail,<br /> > hint) instead of the useless tuple. That's my preference but we can<br /> >
leavethe patch with raise and leave the decision to the committer.<br /> ><p dir="ltr">if breaking compatibility,
thenraise* functions are useless, and should be removed.<p dir="ltr">> What do you think? Jim doesn't like the
separateError being raised. I<br /> > don't agree, but even if I would, it's not this patch's job to change<br />
>that and Error is already raised today. This patch should be<br /> > consistent with the status quo and
thereforebe similar to plpy.error.<br /> > If Error is bad, it should be replaced by SPIError everywhere<br /> >
separately.<pdir="ltr">ok<p dir="ltr">The wrong is duality of Error and SPIError, because it is only one real
exception.<pdir="ltr">The correct naming for current situation are names: PoorError (message only) and RichError (full
setof Edata fields). This is strange from Pg perspective and still is strange from Python view.<p dir="ltr">I agree, we
shouldto unify a exception from (raise, elog) functions and Error has little bit better sense for me. The disadvantage
isredundant code for filling and reading exception properties (I have to support SPIError), but in this case it is less
wrongthan strange behave.<p dir="ltr">Regards<p dir="ltr">Pavel<br /><p dir="ltr">><br /> > Next week I'll send a
patchto improve the docs.<br />