Use HTABs instead of Python dictionary objects to cache procedures

Two separate hash tables are used for regular procedures and for
trigger procedures, since the way trigger procedures work is quite
different from normal stored procedures.  Change the signatures of
PLy_procedure_{get,create} to accept the function OID and a Boolean
flag indicating whether it's a trigger.  This should make implementing
a PL/Python validator easier.

Using HTABs instead of Python dictionaries makes error recovery
easier, and allows for procedures to be cached based on their OIDs,
not their names.  It also allows getting rid of the PyCObject field
that used to hold a pointer to PLyProcedure, since PyCObjects are
deprecated in Python 2.7 and replaced by Capsules in Python 3.

Jan Urbański

src/pl/plpython/plpython.c

@@ -102,6 +102,7 @@ typedef int Py_ssize_t;
 #include "parser/parse_type.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
+#include "utils/hsearch.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/syscache.h"
@@ -214,11 +215,17 @@ typedef struct PLyProcedure
 	PyObject   *code;			/* compiled procedure code */
 	PyObject   *statics;		/* data saved across calls, local scope */
 	PyObject   *globals;		/* data saved across calls, global scope */
-	PyObject   *me;				/* PyCObject containing pointer to this
-								 * PLyProcedure */
 } PLyProcedure;
 
 
+/* the procedure cache entry */
+typedef struct PLyProcedureEntry
+{
+	Oid			fn_oid;			/* hash key */
+	PLyProcedure *proc;
+} PLyProcedureEntry;
+
+
 /* Python objects */
 typedef struct PLyPlanObject
 {
@@ -311,11 +318,10 @@ static HeapTuple PLy_modify_tuple(PLyProcedure *, PyObject *,
 
 static PyObject *PLy_procedure_call(PLyProcedure *, char *, PyObject *);
 
-static PLyProcedure *PLy_procedure_get(FunctionCallInfo fcinfo,
+static PLyProcedure *PLy_procedure_get(Oid fn_oid, bool is_trigger);
-				  Oid tgreloid);
 
-static PLyProcedure *PLy_procedure_create(HeapTuple procTup, Oid tgreloid,
+static PLyProcedure *PLy_procedure_create(HeapTuple procTup,
-					 char *key);
+										  Oid fn_oid, bool is_trigger);
 
 static void PLy_procedure_compile(PLyProcedure *, const char *);
 static char *PLy_procedure_munge_source(const char *, const char *);
@@ -373,7 +379,8 @@ static ErrorData *PLy_error_in_progress = NULL;
 
 static PyObject *PLy_interp_globals = NULL;
 static PyObject *PLy_interp_safe_globals = NULL;
-static PyObject *PLy_procedure_cache = NULL;
+static HTAB *PLy_procedure_cache = NULL;
+static HTAB *PLy_trigger_cache = NULL;
 
 /* Python exceptions */
 static PyObject *PLy_exc_error = NULL;
@@ -444,7 +451,6 @@ plpython_call_handler(PG_FUNCTION_ARGS)
 {
 	Datum		retval;
 	PLyProcedure *save_curr_proc;
-	PLyProcedure *volatile proc = NULL;
 	ErrorContextCallback plerrcontext;
 
 	if (SPI_connect() != SPI_OK_CONNECT)
@@ -461,20 +467,20 @@ plpython_call_handler(PG_FUNCTION_ARGS)
 
 	PG_TRY();
 	{
+		PLyProcedure *proc;
+
 		if (CALLED_AS_TRIGGER(fcinfo))
 		{
-			TriggerData *tdata = (TriggerData *) fcinfo->context;
 			HeapTuple	trv;
 
-			proc = PLy_procedure_get(fcinfo,
+			proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, true);
-									 RelationGetRelid(tdata->tg_relation));
 			PLy_curr_procedure = proc;
 			trv = PLy_trigger_handler(fcinfo, proc);
 			retval = PointerGetDatum(trv);
 		}
 		else
 		{
-			proc = PLy_procedure_get(fcinfo, InvalidOid);
+			proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, false);
 			PLy_curr_procedure = proc;
 			retval = PLy_function_handler(fcinfo, proc);
 		}
@@ -482,11 +488,6 @@ plpython_call_handler(PG_FUNCTION_ARGS)
 	PG_CATCH();
 	{
 		PLy_curr_procedure = save_curr_proc;
-		if (proc)
-		{
-			/* note: Py_DECREF needs braces around it, as of 2003/08 */
-			Py_DECREF(proc->me);
-		}
 		PyErr_Clear();
 		PG_RE_THROW();
 	}
@@ -497,8 +498,6 @@ plpython_call_handler(PG_FUNCTION_ARGS)
 
 	PLy_curr_procedure = save_curr_proc;
 
-	Py_DECREF(proc->me);
-
 	return retval;
 }
 
@@ -575,6 +574,22 @@ PLy_trigger_handler(FunctionCallInfo fcinfo, PLyProcedure *proc)
 	HeapTuple	rv = NULL;
 	PyObject   *volatile plargs = NULL;
 	PyObject   *volatile plrv = NULL;
+	TriggerData	*tdata;
+
+	Assert(CALLED_AS_TRIGGER(fcinfo));
+
+	/*
+	 * Input/output conversion for trigger tuples.  Use the result
+	 * TypeInfo variable to store the tuple conversion info.  We do
+	 * this over again on each call to cover the possibility that the
+	 * relation's tupdesc changed since the trigger was last called.
+	 * PLy_input_tuple_funcs and PLy_output_tuple_funcs are
+	 * responsible for not doing repetitive work.
+	 */
+	tdata = (TriggerData *) fcinfo->context;
+
+	PLy_input_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
+	PLy_output_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
 
 	PG_TRY();
 	{
@@ -1285,6 +1300,19 @@ PLy_function_delete_args(PLyProcedure *proc)
 			PyDict_DelItemString(proc->globals, proc->argnames[i]);
 }
 
+/*
+ * Decide whether a cached PLyProcedure struct is still valid
+ */
+static bool
+PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup)
+{
+	Assert(proc != NULL);
+
+	/* If the pg_proc tuple has changed, it's not valid */
+	return (proc->fn_xmin == HeapTupleHeaderGetXmin(procTup->t_data) &&
+			ItemPointerEquals(&proc->fn_tid, &procTup->t_self));
+}
+
 
 /*
  * PLyProcedure functions
@@ -1296,73 +1324,63 @@ PLy_function_delete_args(PLyProcedure *proc)
  * function calls.
  */
 static PLyProcedure *
-PLy_procedure_get(FunctionCallInfo fcinfo, Oid tgreloid)
+PLy_procedure_get(Oid fn_oid, bool is_trigger)
 {
-	Oid			fn_oid;
 	HeapTuple	procTup;
-	char		key[128];
+	PLyProcedureEntry *entry;
-	PyObject   *plproc;
+	bool		found;
-	PLyProcedure *proc = NULL;
-	int			rv;
 
-	fn_oid = fcinfo->flinfo->fn_oid;
 	procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(fn_oid));
 	if (!HeapTupleIsValid(procTup))
 		elog(ERROR, "cache lookup failed for function %u", fn_oid);
 
-	rv = snprintf(key, sizeof(key), "%u_%u", fn_oid, tgreloid);
+	/* Look for the function in the corresponding cache */
-	if (rv >= sizeof(key) || rv < 0)
+	if (is_trigger)
-		elog(ERROR, "key too long");
+		entry = hash_search(PLy_trigger_cache,
-
+							&fn_oid, HASH_ENTER, &found);
-	plproc = PyDict_GetItemString(PLy_procedure_cache, key);
+	else
+		entry = hash_search(PLy_procedure_cache,
+							&fn_oid, HASH_ENTER, &found);
 
-	if (plproc != NULL)
+	PG_TRY();
 	{
-		Py_INCREF(plproc);
+		if (!found)
-		if (!PyCObject_Check(plproc))
+		{
-			elog(FATAL, "expected a PyCObject, didn't get one");
+			/* Haven't found it, create a new cache entry */
-
+			entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
-		proc = PyCObject_AsVoidPtr(plproc);
+		}
-		if (!proc)
+		else if (!PLy_procedure_valid(entry->proc, procTup))
-			PLy_elog(ERROR, "PyCObject_AsVoidPtr() failed");
-		if (proc->me != plproc)
-			elog(FATAL, "proc->me != plproc");
-		/* did we find an up-to-date cache entry? */
-		if (proc->fn_xmin != HeapTupleHeaderGetXmin(procTup->t_data) ||
-			!ItemPointerEquals(&proc->fn_tid, &procTup->t_self))
 		{
-			Py_DECREF(plproc);
+			/* Found it, but it's invalid, free and reuse the cache entry */
-			proc = NULL;
+			PLy_procedure_delete(entry->proc);
+			PLy_free(entry->proc);
+			entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
 		}
+		/* Found it and it's valid, it's fine to use it */
 	}
-
+	PG_CATCH();
-	if (proc == NULL)
-		proc = PLy_procedure_create(procTup, tgreloid, key);
-
-	if (OidIsValid(tgreloid))
 	{
-		/*
+		/* Do not leave an uninitialised entry in the cache */
-		 * Input/output conversion for trigger tuples.	Use the result
+		if (is_trigger)
-		 * TypeInfo variable to store the tuple conversion info.  We do this
+			hash_search(PLy_trigger_cache,
-		 * over again on each call to cover the possibility that the
+						&fn_oid, HASH_REMOVE, NULL);
-		 * relation's tupdesc changed since the trigger was last called.
+		else
-		 * PLy_input_tuple_funcs and PLy_output_tuple_funcs are responsible
+			hash_search(PLy_procedure_cache,
-		 * for not doing repetitive work.
+						&fn_oid, HASH_REMOVE, NULL);
-		 */
+		PG_RE_THROW();
-		TriggerData *tdata = (TriggerData *) fcinfo->context;
-
-		Assert(CALLED_AS_TRIGGER(fcinfo));
-		PLy_input_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
-		PLy_output_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
 	}
+	PG_END_TRY();
 
 	ReleaseSysCache(procTup);
 
-	return proc;
+	return entry->proc;
 }
 
+/*
+ * Create a new PLyProcedure structure
+ */
 static PLyProcedure *
-PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
+PLy_procedure_create(HeapTuple procTup, Oid fn_oid, bool is_trigger)
 {
 	char		procName[NAMEDATALEN + 256];
 	Form_pg_proc procStruct;
@@ -1374,18 +1392,10 @@ PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
 				rv;
 
 	procStruct = (Form_pg_proc) GETSTRUCT(procTup);
-
+	rv = snprintf(procName, sizeof(procName),
-	if (OidIsValid(tgreloid))
+				  "__plpython_procedure_%s_%u",
-		rv = snprintf(procName, sizeof(procName),
+				  NameStr(procStruct->proname),
-					  "__plpython_procedure_%s_%u_trigger_%u",
+				  fn_oid);
-					  NameStr(procStruct->proname),
-					  HeapTupleGetOid(procTup),
-					  tgreloid);
-	else
-		rv = snprintf(procName, sizeof(procName),
-					  "__plpython_procedure_%s_%u",
-					  NameStr(procStruct->proname),
-					  HeapTupleGetOid(procTup));
 	if (rv >= sizeof(procName) || rv < 0)
 		elog(ERROR, "procedure name would overrun buffer");
 
@@ -1402,7 +1412,7 @@ PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
 		PLy_typeinfo_init(&proc->args[i]);
 	proc->nargs = 0;
 	proc->code = proc->statics = NULL;
-	proc->globals = proc->me = NULL;
+	proc->globals = NULL;
 	proc->is_setof = procStruct->proretset;
 	proc->setof = NULL;
 	proc->argnames = NULL;
@@ -1413,7 +1423,7 @@ PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
 		 * get information required for output conversion of the return value,
 		 * but only if this isn't a trigger.
 		 */
-		if (!OidIsValid(tgreloid))
+		if (!is_trigger)
 		{
 			HeapTuple	rvTypeTup;
 			Form_pg_type rvTypeStruct;
@@ -1550,11 +1560,6 @@ PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
 
 		pfree(procSource);
 		procSource = NULL;
-
-		proc->me = PyCObject_FromVoidPtr(proc, NULL);
-		if (!proc->me)
-			PLy_elog(ERROR, "PyCObject_FromVoidPtr() failed");
-		PyDict_SetItemString(PLy_procedure_cache, key, proc->me);
 	}
 	PG_CATCH();
 	{
@@ -1569,6 +1574,9 @@ PLy_procedure_create(HeapTuple procTup, Oid tgreloid, char *key)
 	return proc;
 }
 
+/*
+ * Insert the procedure into the Python interpreter
+ */
 static void
 PLy_procedure_compile(PLyProcedure *proc, const char *src)
 {
@@ -1591,7 +1599,7 @@ PLy_procedure_compile(PLyProcedure *proc, const char *src)
 	crv = PyRun_String(msrc, Py_file_input, proc->globals, NULL);
 	free(msrc);
 
-	if (crv != NULL && (!PyErr_Occurred()))
+	if (crv != NULL)
 	{
 		int			clen;
 		char		call[NAMEDATALEN + 256];
@@ -1605,11 +1613,9 @@ PLy_procedure_compile(PLyProcedure *proc, const char *src)
 		if (clen < 0 || clen >= sizeof(call))
 			elog(ERROR, "string would overflow buffer");
 		proc->code = Py_CompileString(call, "<string>", Py_eval_input);
-		if (proc->code != NULL && (!PyErr_Occurred()))
+		if (proc->code != NULL)
 			return;
 	}
-	else
-		Py_XDECREF(crv);
 
 	PLy_elog(ERROR, "could not compile PL/Python function \"%s\"", proc->proname);
 }
@@ -1667,7 +1673,6 @@ PLy_procedure_delete(PLyProcedure *proc)
 	Py_XDECREF(proc->code);
 	Py_XDECREF(proc->statics);
 	Py_XDECREF(proc->globals);
-	Py_XDECREF(proc->me);
 	if (proc->proname)
 		PLy_free(proc->proname);
 	if (proc->pyname)
@@ -1686,7 +1691,6 @@ PLy_procedure_delete(PLyProcedure *proc)
 	}
 	if (proc->argnames)
 		PLy_free(proc->argnames);
-	PLy_free(proc);
 }
 
 /*
@@ -3232,6 +3236,7 @@ _PG_init(void)
 	/* Be sure we do initialization only once (should be redundant now) */
 	static bool inited = false;
 	const int **version_ptr;
+	HASHCTL		hash_ctl;
 
 	if (inited)
 		return;
@@ -3263,9 +3268,20 @@ _PG_init(void)
 	PLy_init_plpy();
 	if (PyErr_Occurred())
 		PLy_elog(FATAL, "untrapped error in initialization");
-	PLy_procedure_cache = PyDict_New();
+
-	if (PLy_procedure_cache == NULL)
+	memset(&hash_ctl, 0, sizeof(hash_ctl));
-		PLy_elog(ERROR, "could not create procedure cache");
+	hash_ctl.keysize = sizeof(Oid);
+	hash_ctl.entrysize = sizeof(PLyProcedureEntry);
+	hash_ctl.hash = oid_hash;
+	PLy_procedure_cache = hash_create("PL/Python procedures", 32, &hash_ctl,
+									  HASH_ELEM | HASH_FUNCTION);
+
+	memset(&hash_ctl, 0, sizeof(hash_ctl));
+	hash_ctl.keysize = sizeof(Oid);
+	hash_ctl.entrysize = sizeof(PLyProcedureEntry);
+	hash_ctl.hash = oid_hash;
+	PLy_trigger_cache = hash_create("PL/Python triggers", 32, &hash_ctl,
+									HASH_ELEM | HASH_FUNCTION);
 
 	inited = true;
 }