From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Catalin Iacob <iacobcatalin(at)gmail(dot)com> |
Cc: | Craig Ringer <craig(at)2ndquadrant(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: proposal: PL/Pythonu - function ereport |
Date: | 2015-11-03 11:49:15 |
Message-ID: | CAFj8pRC4wXp9hN7H2fCEJ_bidQCLUhW96bm6T3L47X4hLSOtZA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2015-11-02 17:01 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:
> Hello,
>
> Here's a detailed review:
>
> 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
done
>
> 2. a test with just plpy.SPIError() is still missing, this would have
> caught 1.
>
please, can you write some example - I am not able raise described error
>
> 3. the tests have "possibility to set all accessable fields in custom
> exception" above a test that doesn't set all fields, it's confusing
> (and accessible is spelled wrong)
>
>
done
> 4. in the docs, "The constructor of SPIError exception (class)
> supports keyword parameters." sounds better as "An SPIError instance
> can be constructed using keyword parameters."
>
done
>
> 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.
>
> 6. __init__ can use METH_VARARGS | METH_KEYWORDS in both Python2 and
> 3, no need for the #if
>
>
done
> 7. "could not create dictionary for SPI exception" would be more
> precise as "could not create dictionary for SPIError" right?
>
done
>
> 8. in PLy_add_methods_to_dictionary I would avoid import since it
> makes one think of importing modules; maybe "could not create
> functionwrapper for \"%s\"", "could not create method wrapper for \"%s\""
done
>
> 9. also in PLy_add_methods_to_dictionary "could public method \"%s\"
> in dictionary" is not proper English and is missing not, maybe "could
> not add method \"%s\" to class dictionary"?
>
done
>
> 10. in PLy_add_methods_to_dictionary if PyCFunction_New succeeds but
> PyMethod_New fails, func will leak
>
done
>
> 11. it would be nice to have a test for the invalid SQLSTATE code part
>
done
>
> 12. similar to 10, in PLy_spi_error__init__ taking the "invalid
> SQLSTATE" branch leaks exc_args, in general early returns via PLy_elog
> will leave the intermediate Python objects leaking
>
dome
>
>
> Will mark the patch as Waiting for author.
>
attached new update
Regards
Pavel
Attachment | Content-Type | Size |
---|---|---|
plpythonu-spierror-keyword-params-04.patch | text/x-patch | 16.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2015-11-03 12:16:03 | Re: WIP: Rework access method interface |
Previous Message | Andres Freund | 2015-11-03 11:36:12 | Re: OS X El Capitan and DYLD_LIBRARY_PATH |