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

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 46211da1b84bc3537e799ee1126098e71c2428e8 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
index 25060b09b78d713fbfd750b7d42b6e821444476c..80e478b3f20fada30bd09523b7950e8de5838659 100644 (file)
--- a/src/pl/plpython/expected/plpython_trigger.out
+++ b/src/pl/plpython/expected/plpython_trigger.out
@@ -610,3 +610,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/plpy_main.c b/src/pl/plpython/plpy_main.c
index 494ec37ea7cfada29752f2f75b9225880b9aae15..c4de7622a85fc557ef9a12400095da37d746c012 100644 (file)
--- a/src/pl/plpython/plpy_main.c
+++ b/src/pl/plpython/plpy_main.c
@@ -13,6 +13,7 @@
 #include "miscadmin.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"
 #include "utils/syscache.h"
 
 #include "plpython.h"
@@ -173,7 +174,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();
 }
@@ -214,20 +216,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);
                        exec_ctx->curr_proc = proc;
                        trv = PLy_exec_trigger(fcinfo, proc);
                        retval = PointerGetDatum(trv);
                }
                else
                {
-                       proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, false);
+                       proc = PLy_procedure_get(funcoid, InvalidOid, false);
                        exec_ctx->curr_proc = proc;
                        retval = PLy_exec_function(fcinfo, proc);
                }

diff --git a/src/pl/plpython/plpy_procedure.c b/src/pl/plpython/plpy_procedure.c
index 7fb5f00e0f2108f48508ed103fb25bae8e134f31..3083451595fa5b8110764437ab94d99424e9e68c 100644 (file)
--- a/src/pl/plpython/plpy_procedure.c
+++ b/src/pl/plpython/plpy_procedure.c
@@ -23,7 +23,6 @@
 
 
 static HTAB *PLy_procedure_cache = NULL;
-static HTAB *PLy_trigger_cache = NULL;
 
 static PLyProcedure *PLy_procedure_create(HeapTuple procTup, Oid fn_oid, bool is_trigger);
 static bool PLy_procedure_argument_valid(PLyTypeInfo *arg);
@@ -37,18 +36,11 @@ init_procedure_caches(void)
        HASHCTL         hash_ctl;
 
        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);
 }
 
 /*
@@ -68,61 +60,74 @@ PLy_procedure_name(PLyProcedure *proc)
 
 /*
  * 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.
+ * 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.
  */
 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;
 }
 
 /*

diff --git a/src/pl/plpython/plpy_procedure.h b/src/pl/plpython/plpy_procedure.h
index 40a0314cdfb8e5f271df378d4de606c3ccad9815..f1c8510dafc11c4684779e345acaaed2643d9d98 100644 (file)
--- a/src/pl/plpython/plpy_procedure.h
+++ b/src/pl/plpython/plpy_procedure.h
@@ -32,16 +32,23 @@ typedef struct PLyProcedure
        PyObject   *globals;            /* data saved across calls, global scope */
 } 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;
 
 /* PLyProcedure manipulation */
 extern char *PLy_procedure_name(PLyProcedure *proc);
-extern PLyProcedure *PLy_procedure_get(Oid fn_oid, bool is_trigger);
+extern PLyProcedure *PLy_procedure_get(Oid fn_oid, Oid fn_rel, bool is_trigger);
 extern void PLy_procedure_compile(PLyProcedure *proc, const char *src);
 extern void PLy_procedure_delete(PLyProcedure *proc);
 

diff --git a/src/pl/plpython/sql/plpython_trigger.sql b/src/pl/plpython/sql/plpython_trigger.sql
index 9727f44f8b484597cbf23c0b2036cfb49ca5af0c..a054fe729bcf644605d8ad925e1ff280a73a267b 100644 (file)
--- a/src/pl/plpython/sql/plpython_trigger.sql
+++ b/src/pl/plpython/sql/plpython_trigger.sql
@@ -388,3 +388,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;