>From 666145e5aa24811cc0dccc9231535b23a57b3b32 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 25 Jan 2013 12:52:09 +0100 Subject: [PATCH] Fix plpython's caching of trigger functions plpython's procedure cache only used a function's oid as its key which is insufficient in the case of trigger functions which have different type conversions depending on the table they are triggered on. plpython was half-aware of the problem and already used a separate cache for trigger functions in contrast to plain functions but failed to account for the above case. Fix that by only using one cache but making the key (fn_oid, fn_relid) where fn_relid is InvalidOid for plain functions. The caching now isn't used during validation of trigger functions as at that point the relation is not known so no proper lookup in the cache can be made. As function validation only happens infrequently this is not a problem. This got broken in the course of 46211da1b84bc3537e799ee1126098e71c2428e8. Bug reported by Sandro Santilli --- src/pl/plpython/expected/plpython_trigger.out | 24 ++++++++ src/pl/plpython/plpython.c | 83 ++++++++++++++++----------- src/pl/plpython/sql/plpython_trigger.sql | 16 ++++++ 3 files changed, 90 insertions(+), 33 deletions(-) diff --git a/src/pl/plpython/expected/plpython_trigger.out b/src/pl/plpython/expected/plpython_trigger.out index 58aa24b..672ae1d 100644 --- 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 OR REPLACE FUNCTION t() 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 t(); +INSERT INTO a DEFAULT VALUES; +SELECT * FROM a; + data +------ + 1234 +(1 row) + +DROP TABLE a; +CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE t(); +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 index c6f8c38..f5e8f0c 100644 --- a/src/pl/plpython/plpython.c +++ b/src/pl/plpython/plpython.c @@ -227,11 +227,17 @@ 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; @@ -371,7 +377,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 +433,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 +533,7 @@ plpython_validator(PG_FUNCTION_ARGS) ReleaseSysCache(tuple); - PLy_procedure_get(funcoid, is_trigger); + PLy_procedure_get(funcoid, InvalidOid, is_trigger); PG_RETURN_VOID(); } @@ -555,19 +560,22 @@ plpython_call_handler(PG_FUNCTION_ARGS) PG_TRY(); { PLyProcedure *proc; + Oid funcoid = fcinfo->flinfo->fn_oid; if (CALLED_AS_TRIGGER(fcinfo)) { HeapTuple trv; + Relation tgrel = ((TriggerData *) fcinfo->context)->tg_relation; + Oid tgoid = RelationGetRelid(tgrel); - proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, true); + proc = PLy_procedure_get(funcoid, tgoid, 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); } @@ -1517,61 +1525,77 @@ PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup) */ /* 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 get validated they don't belong to a relation, 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) { HeapTuple procTup; + PLyProcedureKey key; PLyProcedureEntry *volatile entry; bool found; + bool use_cache = !is_trigger || fn_rel != InvalidOid; + PLyProcedure *volatile proc; 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 + /* + * 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, - &fn_oid, HASH_ENTER, &found); + &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); + proc = PLy_procedure_create(procTup, fn_oid, is_trigger); + if (use_cache) + entry->proc = proc; } else if (!PLy_procedure_valid(entry->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); + 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 + if (use_cache) hash_search(PLy_procedure_cache, - &fn_oid, HASH_REMOVE, NULL); + &key, HASH_REMOVE, NULL); PG_RE_THROW(); } PG_END_TRY(); ReleaseSysCache(procTup); - return entry->proc; + return proc; } /* @@ -4115,19 +4139,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 index f60565c..f6a0969 100644 --- a/src/pl/plpython/sql/plpython_trigger.sql +++ b/src/pl/plpython/sql/plpython_trigger.sql @@ -382,3 +382,19 @@ 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 OR REPLACE FUNCTION t() 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 t(); +INSERT INTO a DEFAULT VALUES; +SELECT * FROM a; +DROP TABLE a; +CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE t(); +INSERT INTO b DEFAULT VALUES; +SELECT * FROM b; -- 1.7.12.289.g0ce9864.dirty