2015-11-03 17:13 GMT+01:00 Catalin Iacob <iacobcata...@gmail.com>: > On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule <pavel.steh...@gmail.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
