On 17/08/11 11:40, Jan Urbański wrote:
> On 16/08/11 19:12, Jan Urbański wrote:
>> On 16/08/11 19:07, Jean-Baptiste Quenot wrote:
>>>
>>> [plpython is buggy]
> I'll have a patch ready soon.
Here are two patches that fix two separate bugs that you found
simultaneously. Because they're actually separate issues, it turned out
fixing them was a bit more tricky than I expected (fixing one was
unmasking the other one etc).
Thanks for the report!
Jan
>From 3c0bf7519cad735160d9d222d6f86f84987b38b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Urba=C5=84ski?= <wulc...@wulczer.org>
Date: Wed, 17 Aug 2011 16:07:54 +0200
Subject: [PATCH 2/2] Guard against return type changing in PL/Python
functions.
Functions cache their I/O routines and in case their return type is
composite, a change of the underlying type can cause the cache to
become invalid. PL/Python was already checking for composite type
changes for input arguments, now the check is extended to cover the
return type as well.
Per bug report from Jean-Baptiste Quenot.
---
src/pl/plpython/expected/plpython_record.out | 21 ++++++
src/pl/plpython/plpython.c | 93 ++++++++++++++++++-------
src/pl/plpython/sql/plpython_record.sql | 15 ++++
3 files changed, 103 insertions(+), 26 deletions(-)
diff --git a/src/pl/plpython/expected/plpython_record.out b/src/pl/plpython/expected/plpython_record.out
index 7c60089..0bcc46c 100644
--- a/src/pl/plpython/expected/plpython_record.out
+++ b/src/pl/plpython/expected/plpython_record.out
@@ -308,6 +308,27 @@ SELECT * FROM test_inout_params('test_in');
test_in_inout
(1 row)
+-- try changing the return types and call functions again
+ALTER TABLE table_record DROP COLUMN first;
+ALTER TABLE table_record DROP COLUMN second;
+ALTER TABLE table_record ADD COLUMN first text;
+ALTER TABLE table_record ADD COLUMN second int4;
+SELECT * FROM test_table_record_as('obj', 'one', 1, false);
+ first | second
+-------+--------
+ one | 1
+(1 row)
+
+ALTER TYPE type_record DROP ATTRIBUTE first;
+ALTER TYPE type_record DROP ATTRIBUTE second;
+ALTER TYPE type_record ADD ATTRIBUTE first text;
+ALTER TYPE type_record ADD ATTRIBUTE second int4;
+SELECT * FROM test_type_record_as('obj', 'one', 1, false);
+ first | second
+-------+--------
+ one | 1
+(1 row)
+
-- errors cases
CREATE FUNCTION test_type_record_error1() RETURNS type_record AS $$
return { 'first': 'first' }
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 90d3c47..a254ffa 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -1489,6 +1489,42 @@ PLy_function_delete_args(PLyProcedure *proc)
PyDict_DelItemString(proc->globals, proc->argnames[i]);
}
+static bool
+PLy_procedure_argument_valid(PLyTypeInfo *arg)
+{
+ Oid relid;
+ HeapTuple relTup;
+ bool valid;
+
+ /* Only check input arguments that are composite */
+ if (arg->is_rowtype != 1) {
+ return true;
+ }
+
+ /* An uninitialised typ_relid means that we got called on an output
+ * argument of a function returning a unnamed record type */
+ if (!OidIsValid(arg->typ_relid)) {
+ return true;
+ }
+
+ Assert(TransactionIdIsValid(arg->typrel_xmin));
+ Assert(ItemPointerIsValid(&arg->typrel_tid));
+
+ /* Get the pg_class tuple for the argument type */
+ relid = arg->typ_relid;
+ relTup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (!HeapTupleIsValid(relTup))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
+
+ /* If it has changed, the function is not valid */
+ valid = (arg->typrel_xmin == HeapTupleHeaderGetXmin(relTup->t_data) &&
+ ItemPointerEquals(&arg->typrel_tid, &relTup->t_self));
+
+ ReleaseSysCache(relTup);
+
+ return valid;
+}
+
/*
* Decide whether a cached PLyProcedure struct is still valid
*/
@@ -1509,33 +1545,16 @@ PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup)
/* If there are composite input arguments, they might have changed */
for (i = 0; i < proc->nargs; i++)
{
- Oid relid;
- HeapTuple relTup;
-
/* Short-circuit on first changed argument */
if (!valid)
break;
- /* Only check input arguments that are composite */
- if (proc->args[i].is_rowtype != 1)
- continue;
-
- Assert(OidIsValid(proc->args[i].typ_relid));
- Assert(TransactionIdIsValid(proc->args[i].typrel_xmin));
- Assert(ItemPointerIsValid(&proc->args[i].typrel_tid));
-
- /* Get the pg_class tuple for the argument type */
- relid = proc->args[i].typ_relid;
- relTup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
- if (!HeapTupleIsValid(relTup))
- elog(ERROR, "cache lookup failed for relation %u", relid);
-
- /* If it has changed, the function is not valid */
- if (!(proc->args[i].typrel_xmin == HeapTupleHeaderGetXmin(relTup->t_data) &&
- ItemPointerEquals(&proc->args[i].typrel_tid, &relTup->t_self)))
- valid = false;
+ valid = PLy_procedure_argument_valid(&proc->args[i]);
+ }
- ReleaseSysCache(relTup);
+ /* if the output argument is composite, it might have changed */
+ if (valid) {
+ valid = PLy_procedure_argument_valid(&proc->result);
}
return valid;
@@ -1701,10 +1720,9 @@ PLy_procedure_create(HeapTuple procTup, Oid fn_oid, bool is_trigger)
/*
* Now get information required for input conversion of the
- * procedure's arguments. Note that we ignore output arguments here
- * --- since we don't support returning record, and that was already
- * checked above, there's no need to worry about multiple output
- * arguments.
+ * procedure's arguments. Note that we ignore output arguments here,
+ * if the function is returning record, the I/O functions will be set
+ * up when the function is first called.
*/
if (procStruct->pronargs)
{
@@ -2050,6 +2068,29 @@ PLy_output_tuple_funcs(PLyTypeInfo *arg, TupleDesc desc)
arg->out.r.atts = PLy_malloc0(desc->natts * sizeof(PLyDatumToOb));
}
+ Assert(OidIsValid(desc->tdtypeid));
+
+ /*
+ * RECORDOID means we got called to create output functions for a unnamed
+ * record type
+ */
+ if (desc->tdtypeid != RECORDOID && !TransactionIdIsValid(arg->typrel_xmin))
+ {
+ HeapTuple relTup;
+
+ /* Get the pg_class tuple corresponding to the type of the output */
+ arg->typ_relid = typeidTypeRelid(desc->tdtypeid);
+ relTup = SearchSysCache1(RELOID, ObjectIdGetDatum(arg->typ_relid));
+ if (!HeapTupleIsValid(relTup))
+ elog(ERROR, "cache lookup failed for relation %u", arg->typ_relid);
+
+ /* Extract the XMIN value to later use it in PLy_procedure_valid */
+ arg->typrel_xmin = HeapTupleHeaderGetXmin(relTup->t_data);
+ arg->typrel_tid = relTup->t_self;
+
+ ReleaseSysCache(relTup);
+ }
+
for (i = 0; i < desc->natts; i++)
{
HeapTuple typeTup;
diff --git a/src/pl/plpython/sql/plpython_record.sql b/src/pl/plpython/sql/plpython_record.sql
index d727e60..8df65fb 100644
--- a/src/pl/plpython/sql/plpython_record.sql
+++ b/src/pl/plpython/sql/plpython_record.sql
@@ -112,6 +112,21 @@ SELECT * FROM test_in_out_params('test_in');
SELECT * FROM test_in_out_params_multi('test_in');
SELECT * FROM test_inout_params('test_in');
+-- try changing the return types and call functions again
+
+ALTER TABLE table_record DROP COLUMN first;
+ALTER TABLE table_record DROP COLUMN second;
+ALTER TABLE table_record ADD COLUMN first text;
+ALTER TABLE table_record ADD COLUMN second int4;
+
+SELECT * FROM test_table_record_as('obj', 'one', 1, false);
+
+ALTER TYPE type_record DROP ATTRIBUTE first;
+ALTER TYPE type_record DROP ATTRIBUTE second;
+ALTER TYPE type_record ADD ATTRIBUTE first text;
+ALTER TYPE type_record ADD ATTRIBUTE second int4;
+
+SELECT * FROM test_type_record_as('obj', 'one', 1, false);
-- errors cases
--
1.7.5.4
>From 1a08287b8bb3365c1b782458a7c5e28d5853d9f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Urba=C5=84ski?= <wulc...@wulczer.org>
Date: Wed, 17 Aug 2011 16:04:55 +0200
Subject: [PATCH 1/2] Pay attention to dropped columns when creating tuples in
PL/Python.
Just skipping over them is not enough and results in passing
unitialised values to the code that forms a heap tuple.
Noticed while investigating a bug report from Jean-Baptiste Quenot.
---
src/pl/plpython/plpython.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index da34a17..90d3c47 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -2670,7 +2670,11 @@ PLyMapping_ToTuple(PLyTypeInfo *info, TupleDesc desc, PyObject *mapping)
PLyObToDatum *att;
if (desc->attrs[i]->attisdropped)
+ {
+ values[i] = (Datum) NULL;
+ nulls[i] = true;
continue;
+ }
key = NameStr(desc->attrs[i]->attname);
value = NULL;
@@ -2756,7 +2760,11 @@ PLySequence_ToTuple(PLyTypeInfo *info, TupleDesc desc, PyObject *sequence)
PLyObToDatum *att;
if (desc->attrs[i]->attisdropped)
+ {
+ values[i] = (Datum) NULL;
+ nulls[i] = true;
continue;
+ }
value = NULL;
att = &info->out.r.atts[i];
@@ -2819,7 +2827,11 @@ PLyGenericObject_ToTuple(PLyTypeInfo *info, TupleDesc desc, PyObject *object)
PLyObToDatum *att;
if (desc->attrs[i]->attisdropped)
+ {
+ values[i] = (Datum) NULL;
+ nulls[i] = true;
continue;
+ }
key = NameStr(desc->attrs[i]->attname);
value = NULL;
--
1.7.5.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
