2015-10-28 7:25 GMT+01:00 Catalin Iacob <iacobcata...@gmail.com>:

> On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule <pavel.steh...@gmail.com>
> wrote:
> > Hi
> >
> > 2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcata...@gmail.com>:
> >> The current code doesn't build on Python3 because the 3rd argument of
> >> PyMethod_New, the troubled one you need set to NULL has been removed.
> >> This has to do with the distinction between bound and unbound methods
> >> which is gone in Python3.
> >
> >
> > this part of is well isolated, and we can do switch for Python2 and
> Python3
> > without significant problems.
>
> I had a quick look at this and at least 2 things are needed for Python3:
>
> * using PyInstanceMethod_New instead of PyMethod_New; as
> http://bugs.python.org/issue1587 and
> https://docs.python.org/3/c-api/method.html explain that's the new way
> of binding a PyCFunction to a class, PyMethod_New(func, NULL) fails
>
> * in the PLy_spi_error_methods definition, __init__ has to take
> METH_VARARGS | METH_KEYWORDS not just METH_KEYWORDS; in Python2
> METH_KEYWORDS implied METH_VARARGS so it's not needed (it also doesn't
> hurt) but if I don't add it, in Python3 I get:
> ERROR:  SystemError: Bad call flags in PyCFunction_Call. METH_OLDARGS
> is no longer supported!
>
> >> Still, the above link shows a (more verbose but maybe better)
> >> alternative: don't use PyErr_NewException and instead define an
> >> SPIError type with each slot spelled out explicitly. This will remove
> >> the "is it safe to set the third argument to NULL" question.
> >
> > Should be there some problems, if we lost dependency on basic exception
> > class? Some compatibility issues? Or is possible to create full type with
> > inheritance support?
>
> It's possible to give it a base type, see at
> https://hg.python.org/cpython/rev/429cadbc5b10/ this line before
> calling PyType_Ready:
> PyComError_Type.tp_base = (PyTypeObject*)PyExc_Exception;
>
> PyErr_NewException is a shortcut for defining simple Exception
> deriving types, usually one defines a type with the full PyTypeObject
> definition.
>
> In the meantime, I had a deeper look at the 2.7.10 code and I trust
> that PyMethod_New with the last argument set to NULL is ok. Setting
> that to NULL will lead to the PyMethod representing __init__ im_class
> being NULL. But that PyMethod object is not held onto by C code, it's
> added to the SPIError class' dict. From there, it is always retrieved
> from Python via an instance or via the class (so SPIError().__init__
> or SPIError.__init__) which will lead to instancemethod_descr_get
> being called because it's the tp_descr_get slot of PyMethod_Type and
> that code knows the class you're retrieving the attribute from
> (SPIError in this case), checks if the PyMethod already has a not NULL
> im_class (which SPIError.__init__ doesn't) and, if not, calls
> PyMethod_New again and passes the class (SPIError) as the 3rd
> argument.
>
> Given this, I think it's ok to keep using PyErr_NewException rather
> than spelling out the type explicitly.
>

Thank you very much for your analyse. I am sending new version of proposed
patch with Python3 support. Fixed missing check of dictionary
initialization.

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..bf468e1
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*************** $$ LANGUAGE plpythonu;
*** 1205,1210 ****
--- 1205,1235 ----
      approximately the same functionality
     </para>
    </sect2>
+ 
+   <sect2 id="plpython-raising">
+    <title>Raising Errors</title>
+ 
+    <para>
+     The <literal>plpy</literal> module provides several possibilities to
+     to raise a exception:
+    </para>
+ 
+    <variablelist>
+     <varlistentry>
+      <term><literal><function>SPIError</function>([ <replaceable>message</replaceable> [, <replaceable>detail</replaceable> [, <replaceable>hint</replaceable> [, <replaceable>sqlstate</replaceable>  [, <replaceable>schema</replaceable>  [, <replaceable>table</replaceable>  [, <replaceable>column</replaceable>  [, <replaceable>datatype</replaceable>  [, <replaceable>constraint</replaceable> ]]]]]]]]])</literal></term>
+      <listitem>
+        <para>
+         The constructor of SPIError exception (class) supports keyword parameters. 
+ <programlisting>
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ </programlisting>
+        </para>
+      </listitem>
+     </varlistentry>
+    </variablelist>
+   </sect2>
   </sect1>
  
   <sect1 id="plpython-subtransaction">
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..ac985c6
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*************** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 ****
--- 422,473 ----
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* possibility to set all accessable fields in custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+                     detail = 'This is detail text',
+                     hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in <module>
+     hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+                     detail = 'This is detail text',
+                     hint = 'This is hint text.',
+                     sqlstate = 'SILLY',
+                     schema = 'any info about schema',
+                     table = 'any info about table',
+                     column = 'any info about column',
+                     datatype = 'any info about datatype',
+                     constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in <module>
+     constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in <module>
+     raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..a835af9
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*************** PyObject   *PLy_exc_spi_error = NULL;
*** 23,29 ****
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 					   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 23,32 ----
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 					   char **hint, char **query, int *position,
! 					   char **schema_name, char **table_name, char **column_name,
! 					   char **datatype_name, char **constraint_name);
! 
  static char *get_source_line(const char *src, int lineno);
  
  
*************** PLy_elog(int elevel, const char *fmt,...
*** 51,62 ****
  	char	   *hint = NULL;
  	char	   *query = NULL;
  	int			position = 0;
  
  	PyErr_Fetch(&exc, &val, &tb);
  	if (exc != NULL)
  	{
  		if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! 			PLy_get_spi_error_data(val, &sqlerrcode, &detail, &hint, &query, &position);
  		else if (PyErr_GivenExceptionMatches(val, PLy_exc_fatal))
  			elevel = FATAL;
  	}
--- 54,73 ----
  	char	   *hint = NULL;
  	char	   *query = NULL;
  	int			position = 0;
+ 	char	   *schema_name = NULL;
+ 	char	   *table_name = NULL;
+ 	char	   *column_name = NULL;
+ 	char	   *datatype_name = NULL;
+ 	char	   *constraint_name = NULL;
  
  	PyErr_Fetch(&exc, &val, &tb);
  	if (exc != NULL)
  	{
  		if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! 			PLy_get_spi_error_data(val, &sqlerrcode, &detail, &hint, &query, &position,
! 						&schema_name, &table_name, &column_name,
! 						&datatype_name, &constraint_name);
! 
  		else if (PyErr_GivenExceptionMatches(val, PLy_exc_fatal))
  			elevel = FATAL;
  	}
*************** PLy_elog(int elevel, const char *fmt,...
*** 103,109 ****
  				 (tb_depth > 0 && tbmsg) ? errcontext("%s", tbmsg) : 0,
  				 (hint) ? errhint("%s", hint) : 0,
  				 (query) ? internalerrquery(query) : 0,
! 				 (position) ? internalerrposition(position) : 0));
  	}
  	PG_CATCH();
  	{
--- 114,126 ----
  				 (tb_depth > 0 && tbmsg) ? errcontext("%s", tbmsg) : 0,
  				 (hint) ? errhint("%s", hint) : 0,
  				 (query) ? internalerrquery(query) : 0,
! 				 (position) ? internalerrposition(position) : 0,
! 				 (schema_name) ? err_generic_string(PG_DIAG_SCHEMA_NAME, schema_name) : 0,
! 				 (table_name) ? err_generic_string(PG_DIAG_TABLE_NAME, table_name) : 0,
! 				 (column_name) ? err_generic_string(PG_DIAG_COLUMN_NAME, column_name) : 0,
! 				 (datatype_name) ? err_generic_string(PG_DIAG_DATATYPE_NAME, datatype_name) : 0,
! 				 (constraint_name) ? err_generic_string(PG_DIAG_CONSTRAINT_NAME, constraint_name) : 0));
! 
  	}
  	PG_CATCH();
  	{
*************** PLy_get_spi_sqlerrcode(PyObject *exc, in
*** 365,371 ****
   * Extract the error data from a SPIError
   */
  static void
! PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail, char **hint, char **query, int *position)
  {
  	PyObject   *spidata = NULL;
  
--- 382,390 ----
   * Extract the error data from a SPIError
   */
  static void
! PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail, char **hint, char **query, int *position,
! 			char **schema_name, char **table_name, char **column_name,
! 			char **datatype_name, char **constraint_name)
  {
  	PyObject   *spidata = NULL;
  
*************** PLy_get_spi_error_data(PyObject *exc, in
*** 373,379 ****
  
  	if (spidata != NULL)
  	{
! 		PyArg_ParseTuple(spidata, "izzzi", sqlerrcode, detail, hint, query, position);
  	}
  	else
  	{
--- 392,400 ----
  
  	if (spidata != NULL)
  	{
! 		PyArg_ParseTuple(spidata, "izzzizzzzz", sqlerrcode, detail, hint, query, position,
! 						    schema_name, table_name, column_name,
! 						    datatype_name, constraint_name);
  	}
  	else
  	{
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
new file mode 100644
index a44b7fb..82e6c8f
*** a/src/pl/plpython/plpy_plpymodule.c
--- b/src/pl/plpython/plpy_plpymodule.c
*************** HTAB	   *PLy_spi_exceptions = NULL;
*** 26,31 ****
--- 26,32 ----
  
  static void PLy_add_exceptions(PyObject *plpy);
  static void PLy_generate_spi_exceptions(PyObject *mod, PyObject *base);
+ static void PLy_add_methods_to_dictionary(PyObject *dict, PyMethodDef *methods);
  
  /* module functions */
  static PyObject *PLy_debug(PyObject *self, PyObject *args);
*************** static PyObject *PLy_quote_literal(PyObj
*** 39,44 ****
--- 40,48 ----
  static PyObject *PLy_quote_nullable(PyObject *self, PyObject *args);
  static PyObject *PLy_quote_ident(PyObject *self, PyObject *args);
  
+ /* methods */
+ static PyObject *PLy_spi_error__init__(PyObject *self, PyObject *args, PyObject *kw);
+ 
  
  /* A list of all known exceptions, generated from backend/utils/errcodes.txt */
  typedef struct ExceptionMap
*************** static PyMethodDef PLy_exc_methods[] = {
*** 99,104 ****
--- 103,117 ----
  	{NULL, NULL, 0, NULL}
  };
  
+ static PyMethodDef PLy_spi_error_methods[] = {
+ #if PY_MAJOR_VERSION < 3
+ 	{"__init__", (PyCFunction) PLy_spi_error__init__, METH_KEYWORDS, NULL},
+ #else
+ 	{"__init__", (PyCFunction) PLy_spi_error__init__, METH_VARARGS | METH_KEYWORDS, NULL},
+ #endif
+ 	{NULL, NULL, 0, NULL}
+ };
+ 
  #if PY_MAJOR_VERSION >= 3
  static PyModuleDef PLy_module = {
  	PyModuleDef_HEAD_INIT,		/* m_base */
*************** static void
*** 185,190 ****
--- 198,204 ----
  PLy_add_exceptions(PyObject *plpy)
  {
  	PyObject   *excmod;
+ 	PyObject   *spi_error_dict;
  	HASHCTL		hash_ctl;
  
  #if PY_MAJOR_VERSION < 3
*************** PLy_add_exceptions(PyObject *plpy)
*** 207,215 ****
  	 */
  	Py_INCREF(excmod);
  
  	PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL);
  	PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL);
! 	PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, NULL);
  
  	if (PLy_exc_error == NULL ||
  		PLy_exc_fatal == NULL ||
--- 221,236 ----
  	 */
  	Py_INCREF(excmod);
  
+ 	/* prepare dictionary with __init__ method for SPIError class */
+ 	spi_error_dict = PyDict_New();
+ 	if (spi_error_dict == NULL)
+ 		PLy_elog(ERROR, "could not create dictionary for SPI exception");
+ 	PLy_add_methods_to_dictionary(spi_error_dict, PLy_spi_error_methods);
+ 
  	PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL);
  	PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL);
! 	PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, spi_error_dict);
! 	Py_DECREF(spi_error_dict);
  
  	if (PLy_exc_error == NULL ||
  		PLy_exc_fatal == NULL ||
*************** PLy_generate_spi_exceptions(PyObject *mo
*** 266,271 ****
--- 287,424 ----
  	}
  }
  
+ /*
+  * Returns dictionary with specified set of methods. It is used for
+  * definition __init__ method of SPIError class. Our __init__ method
+  * supports keyword parameters and allows to set all available PostgreSQL
+  * Error fields.
+  */
+ static void
+ PLy_add_methods_to_dictionary(PyObject *dict, PyMethodDef *methods)
+ {
+ 	PyMethodDef	*method;
+ 
+ 	for (method = methods; method->ml_name != NULL; method++)
+ 	{
+ 		PyObject   *func;
+ 		PyObject   *meth;
+ 
+ 		func = PyCFunction_New(method, NULL);
+ 		if (func == NULL)
+ 			PLy_elog(ERROR, "could not import function \"%s\"", method->ml_name);
+ 
+ #if PY_MAJOR_VERSION < 3
+ 		meth = PyMethod_New(func, NULL, NULL);
+ #else
+ 		meth =  PyInstanceMethod_New(func);
+ #endif
+ 		if (meth == NULL)
+ 			PLy_elog(ERROR, "could not import method \"%s\"", method->ml_name);
+ 
+ 		if (PyDict_SetItemString(dict, method->ml_name, meth))
+ 			PLy_elog(ERROR, "could public method \"%s\" in dictionary", method->ml_name);
+ 
+ 		Py_DECREF(meth);
+ 		Py_DECREF(func);
+ 	}
+ }
+ 
+ /*
+  * Init method for SPIError class.
+  *
+  * This constructor allows to enter all user fields in PostgreSQL exception.
+  * Keywords parameters are supported.
+  */
+ static PyObject *
+ PLy_spi_error__init__(PyObject *self, PyObject *args, PyObject *kw)
+ {
+ 	int sqlstate = 0;
+ 	const char *sqlstatestr = NULL;
+ 	const char *message = NULL;
+ 	const char *detail = NULL;
+ 	const char *hint = NULL;
+ 	const char *column = NULL;
+ 	const char *constraint = NULL;
+ 	const char *datatype = NULL;
+ 	const char *table = NULL;
+ 	const char *schema = NULL;
+ 
+ 	PyObject   *exc_args = NULL;
+ 	PyObject   *spidata = NULL;
+ 
+ 	static char *kwlist[] = { "self", "message", "detail", "hint",
+ 				  "sqlstate",
+ 				  "schema","table", "column",
+ 				  "datatype", "constraint",
+ 				  NULL };
+ 
+ 	/*
+ 	 * don't try to overwrite default sqlstate field, when constructor
+ 	 * is called without any parameter. Important for predefined
+ 	 * spiexception.* exceptions.
+ 	 */
+ 	if (PyTuple_Size(args) > 1 || PyDict_Size(kw) >= 1)
+ 	{
+ 		if (!PyArg_ParseTupleAndKeywords(args, kw, "O|sssssssss",
+ 						     kwlist, &self,
+ 							    &message, &detail, &hint,
+ 							    &sqlstatestr,
+ 							    &schema, &table, &column,
+ 							    &datatype, &constraint))
+ 			return NULL;
+ 
+ 		if (message != NULL)
+ 		{
+ 			exc_args = Py_BuildValue("(s)", message);
+ 			if (!exc_args)
+ 				goto failure;
+ 
+ 			if (PyObject_SetAttrString(self, "args", exc_args) == -1)
+ 				goto failure;
+ 		}
+ 
+ 		if (sqlstatestr != NULL)
+ 		{
+ 			if (strlen(sqlstatestr) != 5)
+ 				PLy_elog(ERROR, "invalid SQLSTATE code");
+ 
+ 			if (strspn(sqlstatestr, "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ") != 5)
+ 				PLy_elog(ERROR, "invalid SQLSTATE code");
+ 
+ 			sqlstate = MAKE_SQLSTATE(sqlstatestr[0],
+ 								  sqlstatestr[1],
+ 								  sqlstatestr[2],
+ 								  sqlstatestr[3],
+ 								  sqlstatestr[4]);
+ 		}
+ 
+ 		spidata = Py_BuildValue("(izzzizzzzz)",
+ 							    sqlstate, detail, hint,
+ 							    NULL, -1,
+ 							    schema, table, column,
+ 							    datatype, constraint);
+ 		if (!spidata)
+ 			goto failure;
+ 
+ 		if (PyObject_SetAttrString(self, "spidata", spidata) == -1)
+ 			goto failure;
+ 
+ 		Py_XDECREF(exc_args);
+ 		Py_DECREF(spidata);
+ 	}
+ 
+ 	Py_INCREF(Py_None);
+ 	return Py_None;
+ 
+ failure:
+ 	Py_XDECREF(exc_args);
+ 	Py_XDECREF(spidata);
+ 
+ 	PLy_elog(ERROR, "could not create SPIError object");
+ 
+ 	return NULL;
+ }
+ 
  
  /*
   * the python interface to the elog function
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
new file mode 100644
index d0e255f..4419099
*** a/src/pl/plpython/plpy_spi.c
--- b/src/pl/plpython/plpy_spi.c
*************** PLy_spi_exception_set(PyObject *excclass
*** 548,555 ****
  	if (!spierror)
  		goto failure;
  
! 	spidata = Py_BuildValue("(izzzi)", edata->sqlerrcode, edata->detail, edata->hint,
! 							edata->internalquery, edata->internalpos);
  	if (!spidata)
  		goto failure;
  
--- 548,558 ----
  	if (!spierror)
  		goto failure;
  
! 	spidata = Py_BuildValue("(izzzizzzzz)", edata->sqlerrcode, edata->detail, edata->hint,
! 							edata->internalquery, edata->internalpos,
! 							edata->schema_name, edata->table_name, edata->column_name,
! 							edata->datatype_name, edata->constraint_name);
! 
  	if (!spidata)
  		goto failure;
  
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
new file mode 100644
index d0df7e6..2d859af
*** a/src/pl/plpython/sql/plpython_error.sql
--- b/src/pl/plpython/sql/plpython_error.sql
*************** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 328,330 ****
--- 328,358 ----
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ 
+ /* possibility to set all accessable fields in custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+                     detail = 'This is detail text',
+                     hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ 
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+                     detail = 'This is detail text',
+                     hint = 'This is hint text.',
+                     sqlstate = 'SILLY',
+                     schema = 'any info about schema',
+                     table = 'any info about table',
+                     column = 'any info about column',
+                     datatype = 'any info about datatype',
+                     constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ 
+ \set VERBOSITY default
+ 
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ 
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to