Re: [HACKERS] Verbosity of Function Return Type Checks
On Mon, 8 Sep 2008, Alvaro Herrera [EMAIL PROTECTED] 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 - 1.219
--- src/pl/plpgsql/src/pl_exec.c 9 Sep 2008 05:48:57 -
***
*** 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;
Re: [HACKERS] Verbosity of Function Return Type Checks
Volkan YAZICI wrote: On Mon, 8 Sep 2008, Alvaro Herrera [EMAIL PROTECTED] 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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
Volkan YAZICI wrote: On Fri, 05 Sep 2008, Tom Lane [EMAIL PROTECTED] 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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
On Thu, 4 Sep 2008, Alvaro Herrera [EMAIL PROTECTED] 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
On Thu, 04 Sep 2008, Tom Lane [EMAIL PROTECTED] 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 - 1.219
--- src/pl/plpgsql/src/pl_exec.c 5 Sep 2008 06:13:18 -
***
*** 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
Re: [HACKERS] Verbosity of Function Return Type Checks
Volkan YAZICI wrote: On Thu, 4 Sep 2008, Alvaro Herrera [EMAIL PROTECTED] 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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
On Fri, 5 Sep 2008, Alvaro Herrera [EMAIL PROTECTED] writes:
Please use the patch I posted yesterday, as it had all the issues I
found fixed. There were other changes in that patch too.
My bad. Patch is modified with respect to suggestions[1][2] from
Tom. (All 115 tests passed in cvs tip.)
Regards.
[1] char *msg is replaced with const char *msg.
[2] errmsg(msg) is replaced with 'errmsg(%s, msg)'.
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 - 1.219
--- src/pl/plpgsql/src/pl_exec.c 5 Sep 2008 13:47:07 -
***
*** 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,
! 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,
! 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,
! 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,
! 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,5174
}
/*
! * Validates compatibility of supplied TupleDesc pair by
Re: [HACKERS] Verbosity of Function Return Type Checks
Volkan YAZICI [EMAIL PROTECTED] writes: On Thu, 04 Sep 2008, Tom Lane [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
On Fri, 05 Sep 2008, Tom Lane [EMAIL PROTECTED] 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 - 1.219
--- src/pl/plpgsql/src/pl_exec.c 5 Sep 2008 18:19:50 -
***
*** 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),
!
Re: [HACKERS] Verbosity of Function Return Type Checks
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
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 Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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 - 1.219
--- src/pl/plpgsql/src/pl_exec.c 4 Sep 2008 22:11:46 -
*** 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
Re: [HACKERS] Verbosity of Function Return Type Checks
Alvaro Herrera [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
On Fri, 8 Aug 2008, Alvaro Herrera [EMAIL PROTECTED] 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;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
[Please ignore the previous reply.]
On Fri, 8 Aug 2008, Alvaro Herrera [EMAIL PROTECTED] 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 - 1.216
--- src/pl/plpgsql/src/pl_exec.c 9 Aug 2008 10:10:32 -
***
*** 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
!
[HACKERS] 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 - 1.216
+++ src/pl/plpgsql/src/pl_exec.c 8 Aug 2008 11:52:02 -
@@ -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;
}
/* --
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Verbosity of Function Return Type Checks
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
