Thread: PLPGSQL OID Bug
This patch will resolve the oid retrieval bugs from plpgsql. There are however several other places where isnull=false was removed and replaced with isnull which may also need to be corrected.
Kevin McArthur
StormTide Digital Studios Inc.
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.149
diff -c -r1.149 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 26 Jun 2005 22:05:42 -0000 1.149
--- src/pl/plpgsql/src/pl_exec.c 27 Jul 2005 20:38:25 -0000
***************
*** 1143,1149 ****
{
PLpgSQL_diag_item *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
PLpgSQL_datum *var;
! bool isnull;
if (diag_item->target <= 0)
continue;
--- 1143,1149 ----
{
PLpgSQL_diag_item *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
PLpgSQL_datum *var;
! bool isnull=false;
if (diag_item->target <= 0)
continue;
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.149
diff -c -r1.149 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 26 Jun 2005 22:05:42 -0000 1.149
--- src/pl/plpgsql/src/pl_exec.c 27 Jul 2005 20:38:25 -0000
***************
*** 1143,1149 ****
{
PLpgSQL_diag_item *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
PLpgSQL_datum *var;
! bool isnull;
if (diag_item->target <= 0)
continue;
--- 1143,1149 ----
{
PLpgSQL_diag_item *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
PLpgSQL_datum *var;
! bool isnull=false;
if (diag_item->target <= 0)
continue;
Attachment
"Kevin McArthur" <Kevin@StormTide.ca> writes:
> This patch will resolve the oid retrieval bugs from plpgsql.
Applied.
> There are
> however several other places where isnull=false was removed and
> replaced with isnull which may also need to be corrected.
The other spots seem to be OK. Thanks for the report and fix!
regards, tom lane
Tom Lane wrote: > The other spots seem to be OK. Thanks for the report and fix! Woops, my apologies for introducing the bug. In general I think it's worth removing explicit initialization of out parameters unless that initialization is actually needed. I thought I had checked that `isnull=false' wasn't needed, but obviously I missed a case. BTW, is there a reason why exec_cast_value() and friends take a bool *, rather than a bool? -Neil
Neil Conway <neilc@samurai.com> writes:
> BTW, is there a reason why exec_cast_value() and friends take a bool *,
> rather than a bool?
I think the reason is that it's both an input and an output parameter,
to handle the case where the cast function returns NULL.
This is obviously a pretty bug-prone convention, however, and so maybe
we should rethink it.
regards, tom lane
Tom Lane wrote:
> I think the reason is that it's both an input and an output parameter,
> to handle the case where the cast function returns NULL.
The only reference to `isnull' in the body of exec_cast_value() is:
if (!*isnull)
{
/* ... */
}
i.e. it is never referenced again, let alone written through. Barring
any objections I'll apply the attached patch tomorrow.
-Neil
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.150
diff -c -r1.150 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 28 Jul 2005 00:26:30 -0000 1.150
--- src/pl/plpgsql/src/pl_exec.c 28 Jul 2005 05:29:21 -0000
***************
*** 173,182 ****
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
! bool *isnull);
static Datum exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
! bool *isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
--- 173,182 ----
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
! bool isnull);
static Datum exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
! bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
***************
*** 356,362 ****
&(func->fn_retinput),
func->fn_rettypioparam,
-1,
! &fcinfo->isnull);
/*
* If the function's return type isn't by value, copy the value
--- 356,362 ----
&(func->fn_retinput),
func->fn_rettypioparam,
-1,
! fcinfo->isnull);
/*
* If the function's return type isn't by value, copy the value
***************
*** 1348,1354 ****
value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
! var->datatype->atttypmod, &isnull);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1348,1354 ----
value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
! var->datatype->atttypmod, isnull);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
***************
*** 1364,1370 ****
value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
! var->datatype->atttypmod, &isnull);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1364,1370 ----
value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
! var->datatype->atttypmod, isnull);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
***************
*** 1868,1874 ****
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
! &isNull);
tuple = heap_form_tuple(tupdesc, &retval, &isNull);
--- 1868,1874 ----
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
! isNull);
tuple = heap_form_tuple(tupdesc, &retval, &isNull);
***************
*** 1934,1940 ****
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
! &isNull);
tuple = heap_form_tuple(tupdesc, &retval, &isNull);
--- 1934,1940 ----
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
! isNull);
tuple = heap_form_tuple(tupdesc, &retval, &isNull);
***************
*** 2995,3001 ****
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
! isNull);
if (*isNull && var->notnull)
ereport(ERROR,
--- 2995,3001 ----
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
! *isNull);
if (*isNull && var->notnull)
ereport(ERROR,
***************
*** 3194,3200 ****
valtype,
atttype,
atttypmod,
! &attisnull);
if (attisnull)
nulls[fno] = 'n';
else
--- 3194,3200 ----
valtype,
atttype,
atttypmod,
! attisnull);
if (attisnull)
nulls[fno] = 'n';
else
***************
*** 3340,3346 ****
valtype,
arrayelemtypeid,
-1,
! isNull);
/*
* Build the modified array value.
--- 3340,3346 ----
valtype,
arrayelemtypeid,
-1,
! *isNull);
/*
* Build the modified array value.
***************
*** 3564,3570 ****
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
INT4OID, -1,
! isNull);
return DatumGetInt32(exprdatum);
}
--- 3564,3570 ----
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
INT4OID, -1,
! *isNull);
return DatumGetInt32(exprdatum);
}
***************
*** 3586,3592 ****
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
BOOLOID, -1,
! isNull);
return DatumGetBool(exprdatum);
}
--- 3586,3592 ----
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
BOOLOID, -1,
! *isNull);
return DatumGetBool(exprdatum);
}
***************
*** 4060,4068 ****
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
! bool *isnull)
{
! if (!*isnull)
{
/*
* If the type of the queries return value isn't that of the
--- 4060,4068 ----
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
! bool isnull)
{
! if (!isnull)
{
/*
* If the type of the queries return value isn't that of the
***************
*** 4095,4103 ****
static Datum
exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
! bool *isnull)
{
! if (!*isnull)
{
if (valtype != reqtype || reqtypmod != -1)
{
--- 4095,4103 ----
static Datum
exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
! bool isnull)
{
! if (!isnull)
{
if (valtype != reqtype || reqtypmod != -1)
{
Neil Conway <neilc@samurai.com> writes:
> Tom Lane wrote:
>> I think the reason is that it's both an input and an output parameter,
>> to handle the case where the cast function returns NULL.
> [ no it ain't ]
In that case feel free to clean it up ...
regards, tom lane