[prev in list] [next in list] [prev in thread] [next in thread] 

List:       postgresql-general
Subject:    [HACKERS] pl/python do not delete function arguments
From:       Jan Urbański <wulczer () wulczer ! org>
Date:       2010-12-30 17:01:25
Message-ID: 4D1CBAE5.2090007 () wulczer ! org
[Download RAW message or body]

(continuing the flurry of patches)

Here's a patch that stops PL/Python from removing the function's
arguments from its globals dict after calling it. It's
an incremental patch on top of the plpython-refactor patch sent in
http://archives.postgresql.org/message-id/4D135170.3080705@wulczer.org.

Git branch for this patch:
https://github.com/wulczer/postgres/tree/dont-remove-arguments

Apart from being useless, as the whole dict is unreffed and thus freed
in PLy_procedure_delete, removing args actively breaks things for
recursive invocation of the same function. The recursive callee after
returning will remove the args from globals, and subsequent access to
the arguments in the caller will cause a NameError (see new regression
test in patch).

Cheers,
Jan

["plpython-dont-remove-arguments.diff" (text/x-patch)]

diff --git a/src/pl/plpython/expected/plpython_spi.out b/src/pl/plpython/expected/plpython_spi.out
index 7f4ae5c..cb11f60 100644
*** a/src/pl/plpython/expected/plpython_spi.out
--- b/src/pl/plpython/expected/plpython_spi.out
*************** CONTEXT:  PL/Python function "result_nro
*** 133,135 ****
--- 133,163 ----
                   2
  (1 row)
  
+ --
+ -- check recursion with same argument does not clobber globals
+ --
+ CREATE FUNCTION recursion_test(n integer) RETURNS integer
+ AS $$
+ if n in (0, 1):
+     return 1
+ 
+ return n * plpy.execute("select recursion_test(%d) as result" % (n - 1))[0]["result"]
+ $$ LANGUAGE plpythonu;
+ SELECT recursion_test(5);
+  recursion_test 
+ ----------------
+             120
+ (1 row)
+ 
+ SELECT recursion_test(4);
+  recursion_test 
+ ----------------
+              24
+ (1 row)
+ 
+ SELECT recursion_test(1);
+  recursion_test 
+ ----------------
+               1
+ (1 row)
+ 
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 67eb0f3..1827fc9 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** static Datum PLy_function_handler(Functi
*** 307,313 ****
  static HeapTuple PLy_trigger_handler(FunctionCallInfo fcinfo, PLyProcedure *);
  
  static PyObject *PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *);
- static void PLy_function_delete_args(PLyProcedure *);
  static PyObject *PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *,
  					   HeapTuple *);
  static HeapTuple PLy_modify_tuple(PLyProcedure *, PyObject *,
--- 307,312 ----
*************** PLy_function_handler(FunctionCallInfo fc
*** 988,1001 ****
  			 */
  			plargs = PLy_function_build_args(fcinfo, proc);
  			plrv = PLy_procedure_call(proc, "args", plargs);
- 			if (!proc->is_setof)
- 			{
- 				/*
- 				 * SETOF function parameters will be deleted when last row is
- 				 * returned
- 				 */
- 				PLy_function_delete_args(proc);
- 			}
  			Assert(plrv != NULL);
  		}
  
--- 987,992 ----
*************** PLy_function_handler(FunctionCallInfo fc
*** 1053,1060 ****
  				Py_XDECREF(plargs);
  				Py_XDECREF(plrv);
  
- 				PLy_function_delete_args(proc);
- 
  				if (has_error)
  					ereport(ERROR,
  							(errcode(ERRCODE_DATA_EXCEPTION),
--- 1044,1049 ----
*************** PLy_function_build_args(FunctionCallInfo
*** 1267,1287 ****
  	return args;
  }
  
- 
- static void
- PLy_function_delete_args(PLyProcedure *proc)
- {
- 	int			i;
- 
- 	if (!proc->argnames)
- 		return;
- 
- 	for (i = 0; i < proc->nargs; i++)
- 		if (proc->argnames[i])
- 			PyDict_DelItemString(proc->globals, proc->argnames[i]);
- }
- 
- 
  /* Decide if a cached PLyProcedure struct is still valid */
  static bool
  PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup)
--- 1256,1261 ----
diff --git a/src/pl/plpython/sql/plpython_spi.sql b/src/pl/plpython/sql/plpython_spi.sql
index 7f8f6a3..3b65f95 100644
*** a/src/pl/plpython/sql/plpython_spi.sql
--- b/src/pl/plpython/sql/plpython_spi.sql
*************** else:
*** 105,107 ****
--- 105,123 ----
  $$ LANGUAGE plpythonu;
  
  SELECT result_nrows_test();
+ 
+ 
+ --
+ -- check recursion with same argument does not clobber globals
+ --
+ CREATE FUNCTION recursion_test(n integer) RETURNS integer
+ AS $$
+ if n in (0, 1):
+     return 1
+ 
+ return n * plpy.execute("select recursion_test(%d) as result" % (n - 1))[0]["result"]
+ $$ LANGUAGE plpythonu;
+ 
+ SELECT recursion_test(5);
+ SELECT recursion_test(4);
+ SELECT recursion_test(1);


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[prev in list] [next in list] [prev in thread] [next in thread]