Fix plpython's handling of functions used as triggers on multiple tables.

tglsfdc · tglsfdc · commit 17dee323e7ff · 2013-01-25T16:59:05.000-05:00
plpython tried to use a single cache entry for a trigger function, but it needs a separate cache entry for each table the trigger is applied to, because there is table-dependent data in there. This was done correctly before 9.1, but commit 46211da broke it by simplifying the lookup key from "function OID and triggered table OID" to "function OID and is-trigger boolean". Go back to using both OIDs as the lookup key. Per bug report from Sandro Santilli. Andres Freund
diff --git a/src/pl/plpython/expected/plpython_trigger.out b/src/pl/plpython/expected/plpython_trigger.out
@@ -604,3 +604,27 @@ SELECT * FROM composite_trigger_nested_test;
  ("(,t)","(1,f)",)
 (3 rows)
 
+-- check that using a function as a trigger over two tables works correctly
+CREATE FUNCTION trig1234() RETURNS trigger LANGUAGE plpythonu AS $$
+    TD["new"]["data"] = '1234'
+    return 'MODIFY'
+$$;
+CREATE TABLE a(data text);
+CREATE TABLE b(data int); -- different type conversion
+CREATE TRIGGER a_t BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE trig1234();
+CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE trig1234();
+INSERT INTO a DEFAULT VALUES;
+SELECT * FROM a;
+ data 
+------
+ 1234
+(1 row)
+
+DROP TABLE a;
+INSERT INTO b DEFAULT VALUES;
+SELECT * FROM b;
+ data 
+------
+ 1234
+(1 row)
+
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
@@ -228,10 +228,17 @@ typedef struct PLyProcedure
 } PLyProcedure;
 
 
+/* the procedure cache key */
+typedef struct PLyProcedureKey
+{
+	Oid			fn_oid;			/* function OID */
+	Oid			fn_rel;			/* triggered-on relation or InvalidOid */
+} PLyProcedureKey;
+
 /* the procedure cache entry */
 typedef struct PLyProcedureEntry
 {
-	Oid			fn_oid;			/* hash key */
+	PLyProcedureKey key;		/* hash key */
 	PLyProcedure *proc;
 } PLyProcedureEntry;
 
@@ -371,7 +378,7 @@ static HeapTuple PLy_modify_tuple(PLyProcedure *, PyObject *,
 
 static PyObject *PLy_procedure_call(PLyProcedure *, char *, PyObject *);
 
-static PLyProcedure *PLy_procedure_get(Oid fn_oid, bool is_trigger);
+static PLyProcedure *PLy_procedure_get(Oid fn_oid, Oid fn_rel, bool is_trigger);
 
 static PLyProcedure *PLy_procedure_create(HeapTuple procTup,
 					 Oid fn_oid, bool is_trigger);
@@ -427,7 +434,6 @@ static List *explicit_subtransactions = NIL;
 static PyObject *PLy_interp_globals = NULL;
 static PyObject *PLy_interp_safe_globals = NULL;
 static HTAB *PLy_procedure_cache = NULL;
-static HTAB *PLy_trigger_cache = NULL;
 
 /* Python exceptions */
 static PyObject *PLy_exc_error = NULL;
@@ -528,7 +534,8 @@ plpython_validator(PG_FUNCTION_ARGS)
 
 	ReleaseSysCache(tuple);
 
-	PLy_procedure_get(funcoid, is_trigger);
+	/* We can't validate triggers against any particular table ... */
+	PLy_procedure_get(funcoid, InvalidOid, is_trigger);
 
 	PG_RETURN_VOID();
 }
@@ -554,20 +561,22 @@ plpython_call_handler(PG_FUNCTION_ARGS)
 
 	PG_TRY();
 	{
+		Oid			funcoid = fcinfo->flinfo->fn_oid;
 		PLyProcedure *proc;
 
 		if (CALLED_AS_TRIGGER(fcinfo))
 		{
+			Relation	tgrel = ((TriggerData *) fcinfo->context)->tg_relation;
 			HeapTuple	trv;
 
-			proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, true);
+			proc = PLy_procedure_get(funcoid, RelationGetRelid(tgrel), true);
 			PLy_curr_procedure = proc;
 			trv = PLy_trigger_handler(fcinfo, proc);
 			retval = PointerGetDatum(trv);
 		}
 		else
 		{
-			proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, false);
+			proc = PLy_procedure_get(funcoid, InvalidOid, false);
 			PLy_curr_procedure = proc;
 			retval = PLy_function_handler(fcinfo, proc);
 		}
@@ -1516,62 +1525,76 @@ PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup)
  * PLyProcedure functions
  */
 
-/* PLy_procedure_get: returns a cached PLyProcedure, or creates, stores and
- * returns a new PLyProcedure.	fcinfo is the call info, tgreloid is the
- * relation OID when calling a trigger, or InvalidOid (zero) for ordinary
- * function calls.
+/*
+ * PLy_procedure_get: returns a cached PLyProcedure, or creates, stores and
+ * returns a new PLyProcedure.
+ *
+ * fn_oid is the OID of the function requested
+ * fn_rel is InvalidOid or the relation this function triggers on
+ * is_trigger denotes whether the function is a trigger function
+ *
+ * The reason that both fn_rel and is_trigger need to be passed is that when
+ * trigger functions get validated we don't know which relation(s) they'll
+ * be used with, so no sensible fn_rel can be passed.
  */
 static PLyProcedure *
-PLy_procedure_get(Oid fn_oid, bool is_trigger)
+PLy_procedure_get(Oid fn_oid, Oid fn_rel, bool is_trigger)
 {
+	bool		use_cache = !(is_trigger && fn_rel == InvalidOid);
 	HeapTuple	procTup;
-	PLyProcedureEntry *volatile entry;
-	bool		found;
+	PLyProcedureKey key;
+	PLyProcedureEntry *volatile entry = NULL;
+	PLyProcedure *volatile proc = NULL;
+	bool		found = false;
 
 	procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(fn_oid));
 	if (!HeapTupleIsValid(procTup))
 		elog(ERROR, "cache lookup failed for function %u", fn_oid);
 
-	/* Look for the function in the corresponding cache */
-	if (is_trigger)
-		entry = hash_search(PLy_trigger_cache,
-							&fn_oid, HASH_ENTER, &found);
-	else
-		entry = hash_search(PLy_procedure_cache,
-							&fn_oid, HASH_ENTER, &found);
+	/*
+	 * Look for the function in the cache, unless we don't have the necessary
+	 * information (e.g. during validation). In that case we just don't cache
+	 * anything.
+	 */
+	if (use_cache)
+	{
+		key.fn_oid = fn_oid;
+		key.fn_rel = fn_rel;
+		entry = hash_search(PLy_procedure_cache, &key, HASH_ENTER, &found);
+		proc = entry->proc;
+	}
 
 	PG_TRY();
 	{
 		if (!found)
 		{
-			/* Haven't found it, create a new cache entry */
-			entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
+			/* Haven't found it, create a new procedure */
+			proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
+			if (use_cache)
+				entry->proc = proc;
 		}
-		else if (!PLy_procedure_valid(entry->proc, procTup))
+		else if (!PLy_procedure_valid(proc, procTup))
 		{
 			/* Found it, but it's invalid, free and reuse the cache entry */
-			PLy_procedure_delete(entry->proc);
-			PLy_free(entry->proc);
-			entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
+			PLy_procedure_delete(proc);
+			PLy_free(proc);
+			proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
+			entry->proc = proc;
 		}
 		/* Found it and it's valid, it's fine to use it */
 	}
 	PG_CATCH();
 	{
 		/* Do not leave an uninitialised entry in the cache */
-		if (is_trigger)
-			hash_search(PLy_trigger_cache,
-						&fn_oid, HASH_REMOVE, NULL);
-		else
-			hash_search(PLy_procedure_cache,
-						&fn_oid, HASH_REMOVE, NULL);
+		if (use_cache)
+			hash_search(PLy_procedure_cache, &key, HASH_REMOVE, NULL);
 		PG_RE_THROW();
 	}
 	PG_END_TRY();
 
 	ReleaseSysCache(procTup);
 
-	return entry->proc;
+	return proc;
 }
 
 /*
@@ -4115,19 +4138,12 @@ _PG_init(void)
 		PLy_elog(FATAL, "untrapped error in initialization");
 
 	memset(&hash_ctl, 0, sizeof(hash_ctl));
-	hash_ctl.keysize = sizeof(Oid);
+	hash_ctl.keysize = sizeof(PLyProcedureKey);
 	hash_ctl.entrysize = sizeof(PLyProcedureEntry);
-	hash_ctl.hash = oid_hash;
+	hash_ctl.hash = tag_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);
-
 	explicit_subtransactions = NIL;
 
 	inited = true;
diff --git a/src/pl/plpython/sql/plpython_trigger.sql b/src/pl/plpython/sql/plpython_trigger.sql
@@ -382,3 +382,21 @@ INSERT INTO composite_trigger_nested_test VALUES (NULL);
 INSERT INTO composite_trigger_nested_test VALUES (ROW(ROW(1, 'f'), NULL, 3));
 INSERT INTO composite_trigger_nested_test VALUES (ROW(ROW(NULL, 't'), ROW(1, 'f'), NULL));
 SELECT * FROM composite_trigger_nested_test;
+
+-- check that using a function as a trigger over two tables works correctly
+CREATE FUNCTION trig1234() RETURNS trigger LANGUAGE plpythonu AS $$
+    TD["new"]["data"] = '1234'
+    return 'MODIFY'
+$$;
+
+CREATE TABLE a(data text);
+CREATE TABLE b(data int); -- different type conversion
+
+CREATE TRIGGER a_t BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE trig1234();
+CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE trig1234();
+
+INSERT INTO a DEFAULT VALUES;
+SELECT * FROM a;
+DROP TABLE a;
+INSERT INTO b DEFAULT VALUES;
+SELECT * FROM b;
