Thread: Verbosity of Function Return Type Checks
Hi,
Yesterday I needed to fiddle with PostgreSQL internals to be able to
debug a PL/pgSQL procedure returning a set of records. I attached the
patch I used to increase the verbosity of error messages related with
function return type checks. I'll be appreciated if any developer could
commit this patch (or a similar one) into the core.
Regards.
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.216
diff -u -r1.216 pl_exec.c
--- src/pl/plpgsql/src/pl_exec.c 16 May 2008 18:34:51 -0000 1.216
+++ src/pl/plpgsql/src/pl_exec.c 8 Aug 2008 11:52:02 -0000
@@ -190,7 +190,7 @@
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 validate_tupdesc_compat(TupleDesc td1, TupleDesc td2);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
@@ -386,11 +386,12 @@
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
- if (estate.rettupdesc == NULL ||
- !compatible_tupdesc(estate.rettupdesc, tupdesc))
+ if (!estate.rettupdesc)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("returned record type does not match expected record type")));
+ errmsg("returned record type does not match "
+ "expected record type")));
+ validate_tupdesc_compat(tupdesc, estate.rettupdesc);
break;
case TYPEFUNC_RECORD:
@@ -707,11 +708,8 @@
rettup = NULL;
else
{
- if (!compatible_tupdesc(estate.rettupdesc,
- trigdata->tg_relation->rd_att))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("returned tuple structure does not match table of trigger event")));
+ validate_tupdesc_compat(trigdata->tg_relation->rd_att,
+ estate.rettupdesc);
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
@@ -2202,10 +2200,7 @@
errmsg("record \"%s\" is not assigned yet",
rec->refname),
errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
- if (!compatible_tupdesc(tupdesc, rec->tupdesc))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("wrong record type supplied in RETURN NEXT")));
+ validate_tupdesc_compat(rec->tupdesc, tupdesc);
tuple = rec->tup;
}
break;
@@ -2311,10 +2306,7 @@
stmt->params);
}
- if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("structure of query does not match function result type")));
+ validate_tupdesc_compat(portal->tupDesc, estate->rettupdesc);
while (true)
{
@@ -5138,23 +5130,32 @@
}
/*
- * Check two tupledescs have matching number and types of attributes
+ * Validates compatibility of supplied TupleDesc's by checking # and type of
+ * available arguments.
*/
-static bool
-compatible_tupdesc(TupleDesc td1, TupleDesc td2)
+static void
+validate_tupdesc_compat(TupleDesc td1, TupleDesc td2)
{
- int i;
+ int i;
if (td1->natts != td2->natts)
- return false;
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("Number of returned columns (%d) does not match "
+ "expected column count (%d).",
+ td1->natts, td2->natts)));
for (i = 0; i < td1->natts; i++)
- {
if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
- return false;
- }
-
- return true;
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("Returned record type (%s) does not match "
+ "expected record type (%s) in column %d (%s).",
+ format_type_with_typemod(td1->attrs[i]->atttypid,
+ td1->attrs[i]->atttypmod),
+ format_type_with_typemod(td2->attrs[i]->atttypid,
+ td2->attrs[i]->atttypmod),
+ (1+i), NameStr(td2->attrs[i]->attname))));
}
/* ----------
Volkan YAZICI wrote: > Yesterday I needed to fiddle with PostgreSQL internals to be able to > debug a PL/pgSQL procedure returning a set of records. I attached the > patch I used to increase the verbosity of error messages related with > function return type checks. I'll be appreciated if any developer could > commit this patch (or a similar one) into the core. I think this is a good idea, but the new error messages need more work. Have a look at the message style guidelines please, http://www.postgresql.org/docs/8.3/static/error-style-guide.html Particularly I think you need to keep the original errmsg() and add the new messages as errdetail(). (I notice that there's the slight problem that the error messages are different for the different callers.) Also, please use context diffs. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, 8 Aug 2008, Alvaro Herrera <alvherre@commandprompt.com> writes: > I think this is a good idea, but the new error messages need more work. > Have a look at the message style guidelines please, > http://www.postgresql.org/docs/8.3/static/error-style-guide.html Right. Done -- I hope. > Particularly I think you need to keep the original errmsg() and add the > new messages as errdetail(). (I notice that there's the slight problem > that the error messages are different for the different callers.) Done. > Also, please use context diffs. Done. Regards. Index: src/pl/plpgsql/src/pl_exec.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v retrieving revision 1.216 diff -r1.216 pl_exec.c 193c193 < static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2); --- > static void validate_tupdesc_compat(TupleDesc td1, TupleDesc td2); 389,393c389 < if (estate.rettupdesc == NULL || < !compatible_tupdesc(estate.rettupdesc, tupdesc)) < ereport(ERROR, < (errcode(ERRCODE_DATATYPE_MISMATCH), < errmsg("returned record type does not match expected record type"))); --- > validate_tupdesc_compat(tupdesc, estate.rettupdesc); 710,714c706,707 < if (!compatible_tupdesc(estate.rettupdesc, < trigdata->tg_relation->rd_att)) < ereport(ERROR, < (errcode(ERRCODE_DATATYPE_MISMATCH), < errmsg("returned tuple structure does not match table of trigger event"))); --- > validate_tupdesc_compat(trigdata->tg_relation->rd_att, > estate.rettupdesc); 2204,2208c2197,2199 < errdetail("The tuple structure of a not-yet-assigned record is indeterminate."))); < if (!compatible_tupdesc(tupdesc, rec->tupdesc)) < ereport(ERROR, < (errcode(ERRCODE_DATATYPE_MISMATCH), < errmsg("wrong record type supplied in RETURN NEXT"))); --- > errdetail("The tuple structure of a not-yet-assigned" > " record is indeterminate."))); > validate_tupdesc_compat(rec->tupdesc, tupdesc); 2314,2317c2305 < if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc)) < ereport(ERROR, < (errcode(ERRCODE_DATATYPE_MISMATCH), < errmsg("structure of query does not match function result type"))); --- > validate_tupdesc_compat(portal->tupDesc, estate->rettupdesc); 5141c5129,5130 < * Check two tupledescs have matching number and types of attributes --- > * Validates compatibility of supplied TupleDesc couple by checking # and type > * of available arguments. 5143,5144c5132,5133 < static bool < compatible_tupdesc(TupleDesc td1, TupleDesc td2) --- > static void > validate_tupdesc_compat(TupleDesc td1, TupleDesc td2) 5146c5135,5141 < int i; --- > int i; > > if (!td1 || !td2) > ereport(ERROR, > (errcode(ERRCODE_DATATYPE_MISMATCH), > errmsg("returned record type does not match expected " > "record type"))); 5149c5144,5150 < return false; --- > ereport(ERROR, > (errcode(ERRCODE_DATATYPE_MISMATCH), > errmsg("returned record type does not match expected " > "record type"), > errdetail("Number of returned columns (%d) does not match " > "expected column count (%d).", > td1->natts, td2->natts))); 5152d5152 < { 5154,5157c5154,5164 < return false; < } < < return true; --- > ereport(ERROR, > (errcode(ERRCODE_DATATYPE_MISMATCH), > errmsg("returned record type does not match expected " > "record type"), > errdetail("Returned record type (%s) does not match " > "expected record type (%s) in column %d (%s).", > format_type_with_typemod(td1->attrs[i]->atttypid, > td1->attrs[i]->atttypmod), > format_type_with_typemod(td2->attrs[i]->atttypid, > td2->attrs[i]->atttypmod), > (1+i), NameStr(td2->attrs[i]->attname))));
[Please ignore the previous reply.] On Fri, 8 Aug 2008, Alvaro Herrera <alvherre@commandprompt.com> writes: > I think this is a good idea, but the new error messages need more work. > Have a look at the message style guidelines please, > http://www.postgresql.org/docs/8.3/static/error-style-guide.html Right. Done -- I hope. > Particularly I think you need to keep the original errmsg() and add the > new messages as errdetail(). Made callers pass related error message as a string parameter, and appended required details using errdetail(). > (I notice that there's the slight problem > that the error messages are different for the different callers.) Above mentioned change should have addressed this issue too. > Also, please use context diffs. Done. Regards. Index: src/pl/plpgsql/src/pl_exec.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v retrieving revision 1.216 diff -c -r1.216 pl_exec.c *** src/pl/plpgsql/src/pl_exec.c 16 May 2008 18:34:51 -0000 1.216 --- src/pl/plpgsql/src/pl_exec.c 9 Aug 2008 10:10:32 -0000 *************** *** 190,196 **** 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); static void plpgsql_create_econtext(PLpgSQL_execstate *estate); static void free_var(PLpgSQL_var *var); --- 190,196 ---- Oid reqtype, int32 reqtypmod, bool isnull); static void exec_init_tuple_store(PLpgSQL_execstate *estate); ! static void validate_tupdesc_compat(TupleDesc td1, TupleDesc td2, char *msg); static void exec_set_found(PLpgSQL_execstate *estate, bool state); static void plpgsql_create_econtext(PLpgSQL_execstate *estate); static void free_var(PLpgSQL_var *var); *************** *** 386,396 **** { case TYPEFUNC_COMPOSITE: /* got the expected result rowtype, now check it */ ! if (estate.rettupdesc == NULL || ! !compatible_tupdesc(estate.rettupdesc, tupdesc)) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg("returned record type does not match expected record type"))); break; case TYPEFUNC_RECORD: --- 386,394 ---- { case TYPEFUNC_COMPOSITE: /* got the expected result rowtype, now check it */ ! validate_tupdesc_compat(tupdesc, estate.rettupdesc, ! "returned record type does not " ! "match expected record type"); break; case TYPEFUNC_RECORD: *************** *** 707,717 **** rettup = NULL; else { ! if (!compatible_tupdesc(estate.rettupdesc, ! trigdata->tg_relation->rd_att)) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg("returned tuple structure does not match table of trigger event"))); /* Copy tuple to upper executor memory */ rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval)); } --- 705,714 ---- rettup = NULL; else { ! validate_tupdesc_compat(trigdata->tg_relation->rd_att, ! estate.rettupdesc, ! "returned tuple structure does not match " ! "table of trigger event"); /* Copy tuple to upper executor memory */ rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval)); } *************** *** 2201,2211 **** (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("record \"%s\" is not assigned yet", rec->refname), ! errdetail("The tuple structure of a not-yet-assigned record is indeterminate."))); ! if (!compatible_tupdesc(tupdesc, rec->tupdesc)) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg("wrong record type supplied in RETURN NEXT"))); tuple = rec->tup; } break; --- 2198,2208 ---- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("record \"%s\" is not assigned yet", rec->refname), ! errdetail("The tuple structure of a not-yet-assigned" ! " record is indeterminate."))); ! validate_tupdesc_compat(rec->tupdesc, tupdesc, ! "wrong record type supplied in " ! "RETURN NEXT"); tuple = rec->tup; } break; *************** *** 2311,2320 **** stmt->params); } ! if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc)) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg("structure of query does not match function result type"))); while (true) { --- 2308,2316 ---- stmt->params); } ! validate_tupdesc_compat(portal->tupDesc, estate->rettupdesc, ! "structure of query does not match function " ! "result type"); while (true) { *************** *** 5138,5160 **** } /* ! * Check two tupledescs have matching number and types of attributes */ ! static bool ! compatible_tupdesc(TupleDesc td1, TupleDesc td2) { ! int i; if (td1->natts != td2->natts) ! return false; for (i = 0; i < td1->natts; i++) - { if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid) ! return false; ! } ! ! return true; } /* ---------- --- 5134,5170 ---- } /* ! * Validates compatibility of supplied TupleDesc couple by checking # and type ! * of available arguments. */ ! static void ! validate_tupdesc_compat(TupleDesc td1, TupleDesc td2, char *msg) { ! int i; ! ! if (!td1 || !td2) ! ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg(msg))); if (td1->natts != td2->natts) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg(msg), ! errdetail("Number of returned columns (%d) does not match " ! "expected column count (%d).", ! td1->natts, td2->natts))); for (i = 0; i < td1->natts; i++) if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg(msg), ! errdetail("Returned record type (%s) does not match " ! "expected record type (%s) in column %d (%s).", ! format_type_with_typemod(td1->attrs[i]->atttypid, ! td1->attrs[i]->atttypmod), ! format_type_with_typemod(td2->attrs[i]->atttypid, ! td2->attrs[i]->atttypmod), ! (1+i), NameStr(td2->attrs[i]->attname)))); } /* ----------
Volkan YAZICI wrote: > Made callers pass related error message as a string parameter, and > appended required details using errdetail(). Cool, thanks. I had a look and you had some of the expected vs. returned reversed. This patch should be OK. Amazingly, none of the regression tests need changing, which is a bit worrisome. I wasn't able to run the tests in contrib, I don't know why, and I have to go out now. I'll commit this tomorrow. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> I wasn't able to run the tests in contrib, I don't know why, and I have
> to go out now. I'll commit this tomorrow.
This is not ready to go: you've lost the ability to localize most of the
error message strings. Also, "char *msg" should be "const char *msg"
if you're going to pass literal constants to it, and this gives me
the willies even though the passed-in strings are supposedly all fixed: errmsg(msg),
Use errmsg("%s", msg),
to be safe.
Actually, the entire concept of varying the main message to suit the
context exactly, while the detail messages are *not* changing, seems
pretty bogus...
Another problem with it is it's likely going to fail completely on
dropped columns (which will have atttypid = 0).
regards, tom lane
On Thu, 4 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes: > Cool, thanks. I had a look and you had some of the expected vs. > returned reversed. I'll happy to fix the reversed ones if you can report them in more details. Regards.
On Thu, 04 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:
> This is not ready to go: you've lost the ability to localize most of
> the error message strings.
How can I make this available? What's your suggestion?
> Also, "char *msg" should be "const char *msg"
Done.
> if you're going to pass literal constants to it, and this gives me the
> willies even though the passed-in strings are supposedly all fixed:
> errmsg(msg),
> Use
> errmsg("%s", msg),
> to be safe.
Done.
> Actually, the entire concept of varying the main message to suit the
> context exactly, while the detail messages are *not* changing, seems
> pretty bogus...
I share your concerns but couldn't come up with a better approach. I'd
be happy to hear your suggestions.
> Another problem with it is it's likely going to fail completely on
> dropped columns (which will have atttypid = 0).
Is it ok if I'd replace
if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
line in validate_tupdesc_compat with
if (td1->attrs[i]->atttypid &&
td2->attrs[i]->atttypid &&
td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
expression to fix this?
Regards.
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.219
diff -c -r1.219 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 1 Sep 2008 22:30:33 -0000 1.219
--- src/pl/plpgsql/src/pl_exec.c 5 Sep 2008 06:13:18 -0000
***************
*** 188,194 ****
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);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
--- 188,195 ----
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
! static void validate_tupdesc_compat(TupleDesc td1, TupleDesc td2,
! const char *msg);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
***************
*** 384,394 ****
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! if (estate.rettupdesc == NULL ||
! !compatible_tupdesc(estate.rettupdesc, tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned record type does not match expected record type")));
break;
case TYPEFUNC_RECORD:
--- 385,393 ----
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! validate_tupdesc_compat(tupdesc, estate.rettupdesc,
! "returned record type does not "
! "match expected record type");
break;
case TYPEFUNC_RECORD:
***************
*** 705,715 ****
rettup = NULL;
else
{
! if (!compatible_tupdesc(estate.rettupdesc,
! trigdata->tg_relation->rd_att))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned tuple structure does not match table of trigger event")));
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
--- 704,713 ----
rettup = NULL;
else
{
! validate_tupdesc_compat(trigdata->tg_relation->rd_att,
! estate.rettupdesc,
! "returned tuple structure does not match "
! "table of trigger event");
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
***************
*** 2199,2209 ****
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
! if (!compatible_tupdesc(tupdesc, rec->tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("wrong record type supplied in RETURN NEXT")));
tuple = rec->tup;
}
break;
--- 2197,2207 ----
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned"
! " record is indeterminate.")));
! validate_tupdesc_compat(rec->tupdesc, tupdesc,
! "wrong record type supplied in "
! "RETURN NEXT");
tuple = rec->tup;
}
break;
***************
*** 2309,2318 ****
stmt->params);
}
! if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("structure of query does not match function result type")));
while (true)
{
--- 2307,2315 ----
stmt->params);
}
! validate_tupdesc_compat(portal->tupDesc, estate->rettupdesc,
! "structure of query does not match function "
! "result type");
while (true)
{
***************
*** 5145,5167 ****
}
/*
! * Check two tupledescs have matching number and types of attributes
*/
! static bool
! compatible_tupdesc(TupleDesc td1, TupleDesc td2)
{
! int i;
if (td1->natts != td2->natts)
! return false;
for (i = 0; i < td1->natts; i++)
- {
if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
! return false;
! }
!
! return true;
}
/* ----------
--- 5142,5178 ----
}
/*
! * Validates compatibility of supplied TupleDesc couple by checking # and type
! * of available arguments.
*/
! static void
! validate_tupdesc_compat(TupleDesc td1, TupleDesc td2, const char *msg)
{
! int i;
!
! if (!td1 || !td2)
! ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg(msg)));
if (td1->natts != td2->natts)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", msg),
! errdetail("Number of returned columns (%d) does not match "
! "expected column count (%d).",
! td1->natts, td2->natts)));
for (i = 0; i < td1->natts; i++)
if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", msg),
! errdetail("Returned record type (%s) does not match "
! "expected record type (%s) in column %d (%s).",
! format_type_with_typemod(td1->attrs[i]->atttypid,
! td1->attrs[i]->atttypmod),
! format_type_with_typemod(td2->attrs[i]->atttypid,
! td2->attrs[i]->atttypmod),
! (1+i), NameStr(td2->attrs[i]->attname))));
}
/* ----------
Volkan YAZICI wrote: > On Thu, 4 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes: > > Cool, thanks. I had a look and you had some of the expected vs. > > returned reversed. > > I'll happy to fix the reversed ones if you can report them in more > details. Please use the patch I posted yesterday, as it had all the issues I found fixed. There were other changes in that patch too. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Volkan YAZICI <yazicivo@ttmail.com> writes:
> On Thu, 04 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:
>> This is not ready to go: you've lost the ability to localize most of
>> the error message strings.
> How can I make this available? What's your suggestion?
I think the best way is to use
subroutine(..., gettext_noop("special error message here"))
at the call sites, and then
errmsg("%s", _(msg))
when throwing the error. gettext_noop() is needed to have the string
be put into the message catalog, but it doesn't represent any run-time
effort. The _() macro is what actually makes the translation lookup
happen. We don't want to incur that cost in the normal no-error case,
which is why you shouldn't just do _() at the call site and pass an
already-translated string to the subroutine.
>> Another problem with it is it's likely going to fail completely on
>> dropped columns (which will have atttypid = 0).
> Is it ok if I'd replace
> if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
> line in validate_tupdesc_compat with
> if (td1->attrs[i]->atttypid &&
> td2->attrs[i]->atttypid &&
> td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
> expression to fix this?
No, that's weakening the compatibility check. (There's a separate issue
here of teaching plpgsql to actually cope nicely with rowtypes
containing dropped columns, but that's well beyond the scope of this
patch.)
What I'm on about is protecting format_type_be() from being passed
a zero and then failing. Perhaps it would be good enough to do
something like
OidIsValid(td1->attrs[i]->atttypid) ? format_type_with_typemod(td1->attrs[i]->atttypid,
td1->attrs[i]->atttypmod): "dropped column"
while throwing the error.
BTW, since what's actually being looked at is just the type OID and not
the typmod, it seems inappropriate to me to show the typmod in the
error. I'd go with just format_type_be(td1->attrs[i]->atttypid) here
I think.
regards, tom lane
On Fri, 05 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:
> I think the best way is to use
>
> subroutine(..., gettext_noop("special error message here"))
>
> at the call sites, and then
>
> errmsg("%s", _(msg))
>
> when throwing the error. gettext_noop() is needed to have the string
> be put into the message catalog, but it doesn't represent any run-time
> effort. The _() macro is what actually makes the translation lookup
> happen. We don't want to incur that cost in the normal no-error case,
> which is why you shouldn't just do _() at the call site and pass an
> already-translated string to the subroutine.
Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?
>> if (td1->attrs[i]->atttypid &&
>> td2->attrs[i]->atttypid &&
>> td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
>
>> expression to fix this?
>
> No, that's weakening the compatibility check. (There's a separate issue
> here of teaching plpgsql to actually cope nicely with rowtypes
> containing dropped columns, but that's well beyond the scope of this
> patch.)
>
> What I'm on about is protecting format_type_be() from being passed
> a zero and then failing. Perhaps it would be good enough to do
> something like
>
> OidIsValid(td1->attrs[i]->atttypid) ?
> format_type_with_typemod(td1->attrs[i]->atttypid,
> td1->attrs[i]->atttypmod) :
> "dropped column"
>
> while throwing the error.
>
> BTW, since what's actually being looked at is just the type OID and not
> the typmod, it seems inappropriate to me to show the typmod in the
> error. I'd go with just format_type_be(td1->attrs[i]->atttypid) here
> I think.
Done with format_type_be() usage.
BTW, Alvaro fixed my string concatenations which yielded in lines
exceeding 80 characters width, but I'd want to ask twice if you're sure
with this. Because, IMHO, PostgreSQL is also famous with the quality and
readability of its source code -- that I'm quite proud of as a user,
kudos to developers -- and I think it'd be better to stick with 80
characters width convention as much as one can.
Regards.
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.219
diff -c -r1.219 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 1 Sep 2008 22:30:33 -0000 1.219
--- src/pl/plpgsql/src/pl_exec.c 5 Sep 2008 18:19:50 -0000
***************
*** 188,194 ****
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);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
--- 188,195 ----
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
! static void validate_tupdesc_compat(TupleDesc expected, TupleDesc returned,
! const char *msg);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
***************
*** 384,394 ****
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! if (estate.rettupdesc == NULL ||
! !compatible_tupdesc(estate.rettupdesc, tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned record type does not match expected record type")));
break;
case TYPEFUNC_RECORD:
--- 385,392 ----
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! validate_tupdesc_compat(tupdesc, estate.rettupdesc,
! gettext_noop("returned record type does not match expected record
type"));
break;
case TYPEFUNC_RECORD:
***************
*** 705,715 ****
rettup = NULL;
else
{
! if (!compatible_tupdesc(estate.rettupdesc,
! trigdata->tg_relation->rd_att))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned tuple structure does not match table of trigger event")));
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
--- 703,711 ----
rettup = NULL;
else
{
! validate_tupdesc_compat(trigdata->tg_relation->rd_att,
! estate.rettupdesc,
! gettext_noop("returned tuple structure does not match table of trigger event"));
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
***************
*** 2199,2209 ****
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
! if (!compatible_tupdesc(tupdesc, rec->tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("wrong record type supplied in RETURN NEXT")));
tuple = rec->tup;
}
break;
--- 2195,2204 ----
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned"
! " record is indeterminate.")));
! validate_tupdesc_compat(tupdesc, rec->tupdesc,
! gettext_noop("wrong record type supplied in RETURN NEXT"));
tuple = rec->tup;
}
break;
***************
*** 2309,2318 ****
stmt->params);
}
! if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("structure of query does not match function result type")));
while (true)
{
--- 2304,2311 ----
stmt->params);
}
! validate_tupdesc_compat(estate->rettupdesc, portal->tupDesc,
! gettext_noop("structure of query does not match function result type"));
while (true)
{
***************
*** 5145,5167 ****
}
/*
! * Check two tupledescs have matching number and types of attributes
*/
! static bool
! compatible_tupdesc(TupleDesc td1, TupleDesc td2)
{
! int i;
! if (td1->natts != td2->natts)
! return false;
! for (i = 0; i < td1->natts; i++)
! {
! if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
! return false;
! }
! return true;
}
/* ----------
--- 5138,5176 ----
}
/*
! * Validates compatibility of supplied TupleDesc pair by checking number and type
! * of attributes.
*/
! static void
! validate_tupdesc_compat(TupleDesc expected, TupleDesc returned, const char *msg)
{
! int i;
! if (!expected || !returned)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg))));
! if (expected->natts != returned->natts)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg)),
! errdetail("Number of returned columns (%d) does not match expected column count (%d).",
! returned->natts, expected->natts)));
! for (i = 0; i < expected->natts; i++)
! if (expected->attrs[i]->atttypid != returned->attrs[i]->atttypid)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg)),
! errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
! OidIsValid(returned->attrs[i]->atttypid)
! ? format_type_be(returned->attrs[i]->atttypid)
! : "dropped column",
! OidIsValid(expected->attrs[i]->atttypid)
! ? format_type_be(expected->attrs[i]->atttypid)
! : "dropped column",
! NameStr(expected->attrs[i]->attname))));
}
/* ----------
Volkan YAZICI wrote: > BTW, Alvaro fixed my string concatenations which yielded in lines > exceeding 80 characters width, but I'd want to ask twice if you're sure > with this. Because, IMHO, PostgreSQL is also famous with the quality and > readability of its source code -- that I'm quite proud of as a user, > kudos to developers -- and I think it'd be better to stick with 80 > characters width convention as much as one can. Yeah, I'm quite anal about that. What will happen is that pgindent will "push back" the strings so that they start earlier in the line to keep the 80 char limit. IMHO that's actually clearer than truncating the string. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Volkan YAZICI wrote:
> On Fri, 05 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:
> > at the call sites, and then
> >
> > errmsg("%s", _(msg))
> >
> > when throwing the error.
>
> Modified as you suggested. BTW, will there be a similar i18n scenario
> for "dropped column" you mentioned below?
Yes, you need _() around those too.
> Done with format_type_be() usage.
BTW you forgot to remove the quotes around the type names (I know I told
you to add them but Tom gave the reasons why it's not needed) :-)
Those are minor problems that are easily fixed. However there's a
larger issue that Tom mentioned earlier and I concur, which is that the
caller is forming the primary error message and passing it down. It
gets a bit silly if you consider the ways the messages end up worded:
errmsg("returned record type does not match expected record type")); errdetail("Returned type \"%s\" does not match
expectedtype \"%s\" in column \"%s\".", ---> this is the case where it's OK
errmsg("wrong record type supplied in RETURN NEXT")); errdetail("Returned type \"%s\" does not match expected type
\"%s\"in column \"%s\".", --> this is strange
errmsg("returned tuple structure does not match table of trigger event")); errdetail("Returned type \"%s\" does not
matchexpected type \"%s\" in column \"%s\".", --> this is not OK
I've been thinking how to pass down the context information without
feeding the complete string, but I don't find a way without doing
message construction. Construction is to be avoided because it's a pain
for translators.
Maybe we should just use something generic like errmsg("mismatching record type")
and have the caller pass two strings specifying what's the "returned"
tuple and what's the "expected", but I don't see how ... (BTW this is
worth fixing, because every case seems to have appeared independently
without much thought as to other callsites with the same pattern.)
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Mon, 8 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Modified as you suggested. BTW, will there be a similar i18n scenario
>> for "dropped column" you mentioned below?
>
> Yes, you need _() around those too.
For this purpose, I introduced a dropped_column_type variable in
validate_tupdesc_compat() function:
const char dropped_column_type[] = _("n/a (dropped column)");
And used this in below fashion:
OidIsValid(returned->attrs[i]->atttypid)
? format_type_be(returned->attrs[i]->atttypid)
: dropped_column_type
>> Done with format_type_be() usage.
>
> BTW you forgot to remove the quotes around the type names (I know I told
> you to add them but Tom gave the reasons why it's not needed) :-)
Done.
> Those are minor problems that are easily fixed. However there's a
> larger issue that Tom mentioned earlier and I concur, which is that the
> caller is forming the primary error message and passing it down. It
> gets a bit silly if you consider the ways the messages end up worded:
>
> errmsg("returned record type does not match expected record type"));
> errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
> ---> this is the case where it's OK
>
> errmsg("wrong record type supplied in RETURN NEXT"));
> errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
> --> this is strange
>
> errmsg("returned tuple structure does not match table of trigger event"));
> errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
> --> this is not OK
What we're trying to do is to avoid code duplication while checking the
returned tuple types against expected tuple types. For this purpose we
implemented a generic function (validate_tupdesc_compat) to handle all
possible cases. But, IMHO, it's important to remember that there is no
perfect generic function to satisfy all possible cases at best.
> I've been thinking how to pass down the context information without
> feeding the complete string, but I don't find a way without doing
> message construction. Construction is to be avoided because it's a
> pain for translators.
>
> Maybe we should just use something generic like errmsg("mismatching
> record type") and have the caller pass two strings specifying what's
> the "returned" tuple and what's the "expected", but I don't see how
> ... (BTW this is worth fixing, because every case seems to have
> appeared independently without much thought as to other callsites with
> the same pattern.)
I considered the subject with identical constraints but couldn't come up
with a more rational solution. Nevertheless, I'm open to any suggestion.
Regards.
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.219
diff -c -r1.219 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 1 Sep 2008 22:30:33 -0000 1.219
--- src/pl/plpgsql/src/pl_exec.c 9 Sep 2008 05:48:57 -0000
***************
*** 188,194 ****
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);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
--- 188,195 ----
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
! static void validate_tupdesc_compat(TupleDesc expected, TupleDesc returned,
! const char *msg);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);
***************
*** 384,394 ****
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! if (estate.rettupdesc == NULL ||
! !compatible_tupdesc(estate.rettupdesc, tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned record type does not match expected record type")));
break;
case TYPEFUNC_RECORD:
--- 385,392 ----
{
case TYPEFUNC_COMPOSITE:
/* got the expected result rowtype, now check it */
! validate_tupdesc_compat(tupdesc, estate.rettupdesc,
! gettext_noop("returned record type does not match expected record
type"));
break;
case TYPEFUNC_RECORD:
***************
*** 705,715 ****
rettup = NULL;
else
{
! if (!compatible_tupdesc(estate.rettupdesc,
! trigdata->tg_relation->rd_att))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("returned tuple structure does not match table of trigger event")));
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
--- 703,711 ----
rettup = NULL;
else
{
! validate_tupdesc_compat(trigdata->tg_relation->rd_att,
! estate.rettupdesc,
! gettext_noop("returned tuple structure does not match table of trigger event"));
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
***************
*** 2199,2209 ****
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
! if (!compatible_tupdesc(tupdesc, rec->tupdesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("wrong record type supplied in RETURN NEXT")));
tuple = rec->tup;
}
break;
--- 2195,2204 ----
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("record \"%s\" is not assigned yet",
rec->refname),
! errdetail("The tuple structure of a not-yet-assigned"
! " record is indeterminate.")));
! validate_tupdesc_compat(tupdesc, rec->tupdesc,
! gettext_noop("wrong record type supplied in RETURN NEXT"));
tuple = rec->tup;
}
break;
***************
*** 2309,2318 ****
stmt->params);
}
! if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("structure of query does not match function result type")));
while (true)
{
--- 2304,2311 ----
stmt->params);
}
! validate_tupdesc_compat(estate->rettupdesc, portal->tupDesc,
! gettext_noop("structure of query does not match function result type"));
while (true)
{
***************
*** 5145,5167 ****
}
/*
! * Check two tupledescs have matching number and types of attributes
*/
! static bool
! compatible_tupdesc(TupleDesc td1, TupleDesc td2)
{
! int i;
! if (td1->natts != td2->natts)
! return false;
! for (i = 0; i < td1->natts; i++)
! {
! if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
! return false;
! }
! return true;
}
/* ----------
--- 5138,5177 ----
}
/*
! * Validates compatibility of supplied TupleDesc pair by checking number and type
! * of attributes.
*/
! static void
! validate_tupdesc_compat(TupleDesc expected, TupleDesc returned, const char *msg)
{
! int i;
! const char dropped_column_type[] = _("n/a (dropped column)");
! if (!expected || !returned)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg))));
! if (expected->natts != returned->natts)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg)),
! errdetail("Number of returned columns (%d) does not match expected column count (%d).",
! returned->natts, expected->natts)));
! for (i = 0; i < expected->natts; i++)
! if (expected->attrs[i]->atttypid != returned->attrs[i]->atttypid)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("%s", _(msg)),
! errdetail("Returned type %s does not match expected type %s in column %s.",
! OidIsValid(returned->attrs[i]->atttypid)
! ? format_type_be(returned->attrs[i]->atttypid)
! : dropped_column_type,
! OidIsValid(expected->attrs[i]->atttypid)
! ? format_type_be(expected->attrs[i]->atttypid)
! : dropped_column_type,
! NameStr(expected->attrs[i]->attname))));
}
/* ----------
Volkan YAZICI wrote:
> On Mon, 8 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:
> >> Modified as you suggested. BTW, will there be a similar i18n scenario
> >> for "dropped column" you mentioned below?
> >
> > Yes, you need _() around those too.
>
> For this purpose, I introduced a dropped_column_type variable in
> validate_tupdesc_compat() function:
>
> const char dropped_column_type[] = _("n/a (dropped column)");
>
> And used this in below fashion:
>
> OidIsValid(returned->attrs[i]->atttypid)
> ? format_type_be(returned->attrs[i]->atttypid)
> : dropped_column_type
I changed it to gettext_noop("the text") and _(dropped_column_type) in
errdetail, and committed it.
I'd still like to have a better way to word the message, and maybe have
this error appear in a regression test somewhere at least once ...
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support