Skip to content

Commit 08be00f

Browse files
tglsfdctglsfdc
committed
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 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
1 parent bb1e504 commit 08be00f

File tree

5 files changed

+99
-41
lines changed

5 files changed

+99
-41
lines changed

‎src/pl/plpython/expected/plpython_trigger.out

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,3 +610,27 @@ SELECT * FROM composite_trigger_nested_test;
610610
("(,t)","(1,f)",)
611611
(3 rows)
612612

613+
-- check that using a function as a trigger over two tables works correctly
614+
CREATE FUNCTION trig1234() RETURNS trigger LANGUAGE plpythonu AS $$
615+
TD["new"]["data"] = '1234'
616+
return 'MODIFY'
617+
$$;
618+
CREATE TABLE a(data text);
619+
CREATE TABLE b(data int); -- different type conversion
620+
CREATE TRIGGER a_t BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE trig1234();
621+
CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE trig1234();
622+
INSERT INTO a DEFAULT VALUES;
623+
SELECT * FROM a;
624+
data
625+
------
626+
1234
627+
(1 row)
628+
629+
DROP TABLE a;
630+
INSERT INTO b DEFAULT VALUES;
631+
SELECT * FROM b;
632+
data
633+
------
634+
1234
635+
(1 row)
636+

‎src/pl/plpython/plpy_main.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "miscadmin.h"
1515
#include "utils/guc.h"
1616
#include "utils/memutils.h"
17+
#include "utils/rel.h"
1718
#include "utils/syscache.h"
1819

1920
#include "plpython.h"
@@ -174,7 +175,8 @@ plpython_validator(PG_FUNCTION_ARGS)
174175

175176
ReleaseSysCache(tuple);
176177

177-
PLy_procedure_get(funcoid, is_trigger);
178+
/* We can't validate triggers against any particular table ... */
179+
PLy_procedure_get(funcoid, InvalidOid, is_trigger);
178180

179181
PG_RETURN_VOID();
180182
}
@@ -215,20 +217,22 @@ plpython_call_handler(PG_FUNCTION_ARGS)
215217

216218
PG_TRY();
217219
{
220+
Oid funcoid = fcinfo->flinfo->fn_oid;
218221
PLyProcedure *proc;
219222

220223
if (CALLED_AS_TRIGGER(fcinfo))
221224
{
225+
Relation tgrel = ((TriggerData *) fcinfo->context)->tg_relation;
222226
HeapTuple trv;
223227

224-
proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, true);
228+
proc = PLy_procedure_get(funcoid, RelationGetRelid(tgrel), true);
225229
exec_ctx->curr_proc = proc;
226230
trv = PLy_exec_trigger(fcinfo, proc);
227231
retval = PointerGetDatum(trv);
228232
}
229233
else
230234
{
231-
proc = PLy_procedure_get(fcinfo->flinfo->fn_oid, false);
235+
proc = PLy_procedure_get(funcoid, InvalidOid, false);
232236
exec_ctx->curr_proc = proc;
233237
retval = PLy_exec_function(fcinfo, proc);
234238
}

‎src/pl/plpython/plpy_procedure.c

Lines changed: 41 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525

2626
static HTAB *PLy_procedure_cache = NULL;
27-
static HTAB *PLy_trigger_cache = NULL;
2827

2928
static PLyProcedure *PLy_procedure_create(HeapTuple procTup, Oid fn_oid, bool is_trigger);
3029
static bool PLy_procedure_argument_valid(PLyTypeInfo *arg);
@@ -38,18 +37,11 @@ init_procedure_caches(void)
3837
HASHCTL hash_ctl;
3938

4039
memset(&hash_ctl, 0, sizeof(hash_ctl));
41-
hash_ctl.keysize = sizeof(Oid);
40+
hash_ctl.keysize = sizeof(PLyProcedureKey);
4241
hash_ctl.entrysize = sizeof(PLyProcedureEntry);
43-
hash_ctl.hash = oid_hash;
42+
hash_ctl.hash = tag_hash;
4443
PLy_procedure_cache = hash_create("PL/Python procedures", 32, &hash_ctl,
4544
HASH_ELEM | HASH_FUNCTION);
46-
47-
memset(&hash_ctl, 0, sizeof(hash_ctl));
48-
hash_ctl.keysize = sizeof(Oid);
49-
hash_ctl.entrysize = sizeof(PLyProcedureEntry);
50-
hash_ctl.hash = oid_hash;
51-
PLy_trigger_cache = hash_create("PL/Python triggers", 32, &hash_ctl,
52-
HASH_ELEM | HASH_FUNCTION);
5345
}
5446

5547
/*
@@ -69,61 +61,74 @@ PLy_procedure_name(PLyProcedure *proc)
6961

7062
/*
7163
* PLy_procedure_get: returns a cached PLyProcedure, or creates, stores and
72-
* returns a new PLyProcedure. fcinfo is the call info, tgreloid is the
73-
* relation OID when calling a trigger, or InvalidOid (zero) for ordinary
74-
* function calls.
64+
* returns a new PLyProcedure.
65+
*
66+
* fn_oid is the OID of the function requested
67+
* fn_rel is InvalidOid or the relation this function triggers on
68+
* is_trigger denotes whether the function is a trigger function
69+
*
70+
* The reason that both fn_rel and is_trigger need to be passed is that when
71+
* trigger functions get validated we don't know which relation(s) they'll
72+
* be used with, so no sensible fn_rel can be passed.
7573
*/
7674
PLyProcedure *
77-
PLy_procedure_get(Oid fn_oid, bool is_trigger)
75+
PLy_procedure_get(Oid fn_oid, Oid fn_rel, bool is_trigger)
7876
{
77+
bool use_cache = !(is_trigger && fn_rel == InvalidOid);
7978
HeapTuple procTup;
80-
PLyProcedureEntry *volatile entry;
81-
bool found;
79+
PLyProcedureKey key;
80+
PLyProcedureEntry *volatile entry = NULL;
81+
PLyProcedure *volatile proc = NULL;
82+
bool found = false;
8283

8384
procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(fn_oid));
8485
if (!HeapTupleIsValid(procTup))
8586
elog(ERROR, "cache lookup failed for function %u", fn_oid);
8687

87-
/* Look for the function in the corresponding cache */
88-
if (is_trigger)
89-
entry = hash_search(PLy_trigger_cache,
90-
&fn_oid, HASH_ENTER, &found);
91-
else
92-
entry = hash_search(PLy_procedure_cache,
93-
&fn_oid, HASH_ENTER, &found);
88+
/*
89+
* Look for the function in the cache, unless we don't have the necessary
90+
* information (e.g. during validation). In that case we just don't cache
91+
* anything.
92+
*/
93+
if (use_cache)
94+
{
95+
key.fn_oid = fn_oid;
96+
key.fn_rel = fn_rel;
97+
entry = hash_search(PLy_procedure_cache, &key, HASH_ENTER, &found);
98+
proc = entry->proc;
99+
}
94100

95101
PG_TRY();
96102
{
97103
if (!found)
98104
{
99-
/* Haven't found it, create a new cache entry */
100-
entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
105+
/* Haven't found it, create a new procedure */
106+
proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
107+
if (use_cache)
108+
entry->proc = proc;
101109
}
102-
else if (!PLy_procedure_valid(entry->proc, procTup))
110+
else if (!PLy_procedure_valid(proc, procTup))
103111
{
104112
/* Found it, but it's invalid, free and reuse the cache entry */
105-
PLy_procedure_delete(entry->proc);
106-
PLy_free(entry->proc);
107-
entry->proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
113+
PLy_procedure_delete(proc);
114+
PLy_free(proc);
115+
proc = PLy_procedure_create(procTup, fn_oid, is_trigger);
116+
entry->proc = proc;
108117
}
109118
/* Found it and it's valid, it's fine to use it */
110119
}
111120
PG_CATCH();
112121
{
113122
/* Do not leave an uninitialised entry in the cache */
114-
if (is_trigger)
115-
hash_search(PLy_trigger_cache,
116-
&fn_oid, HASH_REMOVE, NULL);
117-
else
118-
hash_search(PLy_procedure_cache,
119-
&fn_oid, HASH_REMOVE, NULL);
123+
if (use_cache)
124+
hash_search(PLy_procedure_cache, &key, HASH_REMOVE, NULL);
120125
PG_RE_THROW();
121126
}
122127
PG_END_TRY();
123128

124129
ReleaseSysCache(procTup);
125130

126-
return entry->proc;
131+
return proc;
127132
}
128133

129134
/*

‎src/pl/plpython/plpy_procedure.h

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,23 @@ typedef struct PLyProcedure
3232
PyObject *globals; /* data saved across calls, global scope */
3333
} PLyProcedure;
3434

35+
/* the procedure cache key */
36+
typedef struct PLyProcedureKey
37+
{
38+
Oid fn_oid; /* function OID */
39+
Oid fn_rel; /* triggered-on relation or InvalidOid */
40+
} PLyProcedureKey;
41+
3542
/* the procedure cache entry */
3643
typedef struct PLyProcedureEntry
3744
{
38-
Oid fn_oid; /* hash key */
45+
PLyProcedureKey key; /* hash key */
3946
PLyProcedure *proc;
4047
} PLyProcedureEntry;
4148

4249
/* PLyProcedure manipulation */
4350
extern char *PLy_procedure_name(PLyProcedure *proc);
44-
extern PLyProcedure *PLy_procedure_get(Oid fn_oid, bool is_trigger);
51+
extern PLyProcedure *PLy_procedure_get(Oid fn_oid, Oid fn_rel, bool is_trigger);
4552
extern void PLy_procedure_compile(PLyProcedure *proc, const char *src);
4653
extern void PLy_procedure_delete(PLyProcedure *proc);
4754

‎src/pl/plpython/sql/plpython_trigger.sql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,3 +388,21 @@ INSERT INTO composite_trigger_nested_test VALUES (NULL);
388388
INSERT INTO composite_trigger_nested_test VALUES (ROW(ROW(1, 'f'), NULL, 3));
389389
INSERT INTO composite_trigger_nested_test VALUES (ROW(ROW(NULL, 't'), ROW(1, 'f'), NULL));
390390
SELECT * FROM composite_trigger_nested_test;
391+
392+
-- check that using a function as a trigger over two tables works correctly
393+
CREATE FUNCTION trig1234() RETURNS trigger LANGUAGE plpythonu AS $$
394+
TD["new"]["data"] = '1234'
395+
return 'MODIFY'
396+
$$;
397+
398+
CREATE TABLE a(data text);
399+
CREATE TABLE b(data int); -- different type conversion
400+
401+
CREATE TRIGGER a_t BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE trig1234();
402+
CREATE TRIGGER b_t BEFORE INSERT ON b FOR EACH ROW EXECUTE PROCEDURE trig1234();
403+
404+
INSERT INTO a DEFAULT VALUES;
405+
SELECT * FROM a;
406+
DROP TABLE a;
407+
INSERT INTO b DEFAULT VALUES;
408+
SELECT * FROM b;

0 commit comments

Comments
 (0)