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.
Attachments:
plpgsql_validate_tupdesc_compat.difftext/x-diffDownload
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.
Attachments:
plpgsql_validate_tupdesc_compat.difftext/x-diffDownload
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.
Attachments:
plpgsql_validate_tupdesc_compat.difftext/x-diffDownload
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
Attachments:
plpgsql_validate_tupdesc_compat-2.patchtext/x-diff; charset=us-asciiDownload
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.219
diff -c -p -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 4 Sep 2008 22:11:46 -0000
*************** static Datum exec_simple_cast_value(Datu
*** 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,
! 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);
*************** plpgsql_exec_function(PLpgSQL_function *
*** 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,
! "returned record type does not match expected record type");
break;
case TYPEFUNC_RECORD:
*************** plpgsql_exec_trigger(PLpgSQL_function *f
*** 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,
! "returned tuple structure does not match table of trigger event");
/* Copy tuple to upper executor memory */
rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
}
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 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,
! "wrong record type supplied in RETURN NEXT");
tuple = rec->tup;
}
break;
*************** exec_stmt_return_query(PLpgSQL_execstate
*** 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,
! "structure of query does not match function result type");
while (true)
{
*************** exec_simple_check_plan(PLpgSQL_expr *exp
*** 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,5174 ----
}
/*
! * Validates compatibility of supplied TupleDesc pair by checking number and type
! * of attributes.
*/
! static void
! validate_tupdesc_compat(TupleDesc expected, TupleDesc returned, char *msg)
{
! int i;
! if (!expected || !returned)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg(msg)));
! if (expected->natts != returned->natts)
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg(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(msg),
! errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
! format_type_with_typemod(returned->attrs[i]->atttypid,
! returned->attrs[i]->atttypmod),
! format_type_with_typemod(expected->attrs[i]->atttypid,
! expected->attrs[i]->atttypmod),
! NameStr(expected->attrs[i]->attname))));
}
/* ----------
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.
Attachments:
plpgsql_validate_tupdesc_compat.difftext/x-diffDownload
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 likeOidIsValid(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.
Attachments:
plpgsql_validate_tupdesc_compat_wip_4.patchtext/x-diffDownload
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 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
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 OKerrmsg("wrong record type supplied in RETURN NEXT"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
--> this is strangeerrmsg("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.
Attachments:
plpgsql_validate_tupdesc_compat_wip_5.patchtext/x-diffDownload
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