Re: proposal: PL/Pythonu - function ereport

2015-11-03 17:13 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> 1. in PLy_spi_error__init__ you need to check kw for NULL before doing
> >> PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
> >> call because PyDict_Size expects a real dictionary not NULL
> >
> >
> > PyDict_Size returns -1 when the dictionary is NULL
> >
> >
> http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return
>
> Yes, but it also sets the error indicator to BadInternalCall. In 2.7
> the code is:
> Py_ssize_t
> PyDict_Size(PyObject *mp)
> {
> if (mp == NULL || !PyDict_Check(mp)) {
> PyErr_BadInternalCall();
> return -1;
> }
> return ((PyDictObject *)mp)->ma_used;
> }
>
> I had a PLy_elog right after the PyDict_Size call for debugging and
> that one was raising BadInternalCall since it checked the error
> indicator. So the NULL check is needed.
>

I did it in last patch - PyDict_Size is not called when kw is NULL

>
> >> 2. a test with just plpy.SPIError() is still missing, this would have
> >> caught 1.
>

one test contains "x = plpy.SPIError()". Is it, what you want?

> >
> >
> > please, can you write some example - I am not able raise described error
>
> The example was plpy.SPIError() but I now realize that, in order to
> see a failure, you need the extra PLy_elog which I had in there.
> But this basic form of the constructor is still an important thing to
> test so please add this as well to the regression test.
>
> >> 5. there is conceptual code duplication between PLy_spi_exception_set
> >> in plpy_spi.c, since that code also constructs an SPIError but from C
> >> and with more info available (edata->internalquery and
> >> edata->internalpos). But making a tuple and assigning it to spidata is
> >> in both. Not sure how this can be improved.
> >
> >
> > I see it, but I don't think, so current code should be changed.
> > PLy_spi_exception_set is controlled directly by fully filled ErrorData
> > structure, __init__ is based only on keyword parameters with possibility
> use
> > inherited data. If I'll build ErrorData in __init__ function and call
> > PLy_spi_exception_set, then the code will be longer and more complex.
>
> Indeed, I don't really see how to improve this but it does bug me a bit.
>
> One more thing,
> + The <literal>plpy</literal> module provides several possibilities to
> + to raise a exception:
>
> This has "to" 2 times and is weird since it says it offers several
> possibilities but then shows only one (the SPIError constructor).
> And SPIError should be <literal>plpy.SPIError</literal> everywhere to
> be consistent.
>

I'll do it tomorrow

>
> Maybe something like (needs markup):
> A plpy.SPIError can be raised from PL/Python, the constructor accepts
> keyword parameters:
> plpy.SPIError([ message [, detail [, hint [, sqlstate [, schema [,
> table [, column [, datatype [, constraint ]]]]]]]]])
> then the example
>
> If you fix the doc and add the plpy.SPIError() test I'm happy. I'll
> give it one more test on Python2.7 and 3.5 and mark it Ready for
> Committer.
>

Regards

Pavel