>From b1178af5b40de01962b2cf7bab4ec0c71ed5b9f8 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/plpy_main.c | 10 +++-- src/pl/plpython/plpy_procedure.c | 64 +++++++++++++++------------ src/pl/plpython/plpy_procedure.h | 11 ++++- src/pl/plpython/sql/plpython_trigger.sql | 16 +++++++ 5 files changed, 92 insertions(+), 33 deletions(-) diff --git a/src/pl/plpython/expected/plpython_trigger.out b/src/pl/plpython/expected/plpython_trigger.out index 25060b0..a4aadd3 100644 --- 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 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/plpy_main.c b/src/pl/plpython/plpy_main.c index 8b61f1a..781e822 100644 --- a/src/pl/plpython/plpy_main.c +++ b/src/pl/plpython/plpy_main.c @@ -14,6 +14,7 @@ #include "miscadmin.h" #include "utils/guc.h" #include "utils/memutils.h" +#include "utils/rel.h" #include "utils/syscache.h" #include "plpython.h" @@ -174,7 +175,7 @@ plpython_validator(PG_FUNCTION_ARGS) ReleaseSysCache(tuple); - PLy_procedure_get(funcoid, is_trigger); + PLy_procedure_get(funcoid, InvalidOid, is_trigger); PG_RETURN_VOID(); } @@ -216,19 +217,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); 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 a28cb9a..8e43a2e 100644 --- a/src/pl/plpython/plpy_procedure.c +++ b/src/pl/plpython/plpy_procedure.c @@ -24,7 +24,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); @@ -38,18 +37,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); } /* @@ -69,61 +61,77 @@ 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 get validated they don't belong to a relation, 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) { 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; } /* diff --git a/src/pl/plpython/plpy_procedure.h b/src/pl/plpython/plpy_procedure.h index 40a0314..2483e50 100644 --- 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 9727f44..d2b14de 100644 --- a/src/pl/plpython/sql/plpython_trigger.sql +++ b/src/pl/plpython/sql/plpython_trigger.sql @@ -388,3 +388,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