On 3/9/24 13:07, Tom Lane wrote:
Joe Conway <m...@joeconway.com> writes:
On 3/5/24 17:04, Tom Lane wrote:
After reading the thread at [1], I could not escape the feeling
that contrib/tablefunc's error reporting is very confusing.
The changes all look good to me and indeed more consistent with the docs.
Do you want me to push these? If so, development tip only, or backpatch?
I can push that. I was just thinking HEAD, we aren't big on changing
error reporting in back branches.
BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.
I can take a look at this. Presumably this would not be for backpatching.
I'm not sure whether that could produce results bad enough to be
called a bug or not. But I too would lean towards not back-patching,
in view of the lack of field complaints.
Something like the attached what you had in mind? (applies on top of
your two patches)
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Make tablefunc crosstab() check typmod too
tablefunc connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. Fix that by makeing
the crosstab() check look more like the connectby() check.
--- tablefunc.c.v0002 2024-03-09 14:38:29.285393890 -0500
+++ tablefunc.c 2024-03-09 14:43:47.021399855 -0500
@@ -1527,10 +1527,10 @@
compatCrosstabTupleDescs(TupleDesc ret_tupdesc, TupleDesc sql_tupdesc)
{
int i;
- Form_pg_attribute ret_attr;
Oid ret_atttypid;
- Form_pg_attribute sql_attr;
Oid sql_atttypid;
+ int32 ret_atttypmod;
+ int32 sql_atttypmod;
if (ret_tupdesc->natts < 2)
ereport(ERROR,
@@ -1539,34 +1539,39 @@
errdetail("Return row must have at least two columns.")));
Assert(sql_tupdesc->natts == 3); /* already checked by caller */
- /* check the rowid types match */
+ /* check the row_name types match */
ret_atttypid = TupleDescAttr(ret_tupdesc, 0)->atttypid;
sql_atttypid = TupleDescAttr(sql_tupdesc, 0)->atttypid;
- if (ret_atttypid != sql_atttypid)
+ ret_atttypmod = TupleDescAttr(ret_tupdesc, 0)->atttypmod;
+ sql_atttypmod = TupleDescAttr(sql_tupdesc, 0)->atttypmod;
+ if (ret_atttypid != sql_atttypid ||
+ (ret_atttypmod >= 0 && ret_atttypmod != sql_atttypmod))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("invalid crosstab return type"),
errdetail("Source row_name datatype %s does not match return row_name datatype %s.",
- format_type_be(sql_atttypid),
- format_type_be(ret_atttypid))));
+ format_type_with_typemod(sql_atttypid, sql_atttypmod),
+ format_type_with_typemod(ret_atttypid, ret_atttypmod))));
/*
- * - attribute [1] of the sql tuple is the category; no need to check it -
- * attribute [2] of the sql tuple should match attributes [1] to [natts]
+ * attribute [1] of the sql tuple is the category; no need to check it
+ * attribute [2] of the sql tuple should match attributes [1] to [natts - 1]
* of the return tuple
*/
- sql_attr = TupleDescAttr(sql_tupdesc, 2);
+ sql_atttypid = TupleDescAttr(sql_tupdesc, 2)->atttypid;
+ sql_atttypmod = TupleDescAttr(sql_tupdesc, 2)->atttypmod;
for (i = 1; i < ret_tupdesc->natts; i++)
{
- ret_attr = TupleDescAttr(ret_tupdesc, i);
-
- if (ret_attr->atttypid != sql_attr->atttypid)
+ ret_atttypid = TupleDescAttr(ret_tupdesc, i)->atttypid;
+ ret_atttypmod = TupleDescAttr(ret_tupdesc, i)->atttypmod;
+ if (ret_atttypid != sql_atttypid ||
+ (ret_atttypmod >= 0 && ret_atttypmod != sql_atttypmod))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("invalid crosstab return type"),
errdetail("Source value datatype %s does not match return value datatype %s in column %d.",
- format_type_be(sql_attr->atttypid),
- format_type_be(ret_attr->atttypid),
+ format_type_with_typemod(sql_atttypid, sql_atttypmod),
+ format_type_with_typemod(ret_atttypid, ret_atttypmod),
i + 1)));
}
