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 -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))));
}
/* ----------
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
