MergeAttributes type (mod) conflict error detail

Started by Amit Langoteabout 10 years ago4 messages

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

1 attachment(s)

I wonder if the following error detail text could say more than it does
currently for the following rather artificial example case:

CREATE TABLE p1(a char(3));
CREATE TABLE p2(a char(2));

CREATE TABLE c(d int) INHERITS (p1, p2);
NOTICE: merging multiple inherited definitions of column "a"
ERROR: inherited column "a" has a type conflict
DETAIL: character versus character

Any specific reason why it doesn't spell out typmods in the above detail
message?

I managed to get the following with the attached:

CREATE TABLE c(a int) INHERITS (p1, p2);
NOTICE: merging multiple inherited definitions of column "a"
ERROR: inherited column "a" has a type conflict
DETAIL: character(3) versus character(2)

CREATE TABLE c(a int) INHERITS (p1);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a type conflict
DETAIL: character(3) versus integer

Thoughts?

Thanks,
Amit

Attachments:

merge-attr-err-detail-improv.patchtext/x-diff; name=merge-attr-err-detail-improv.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 56fed4d..99aa759 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1613,8 +1613,9 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 						errmsg("inherited column \"%s\" has a type conflict",
 							   attributeName),
 							 errdetail("%s versus %s",
-									   TypeNameToString(def->typeName),
-									   format_type_be(attribute->atttypid))));
+									   format_type_with_typemod(defTypeId, deftypmod),
+									   format_type_with_typemod(attribute->atttypid,
+																attribute->atttypmod))));
 				defCollId = GetColumnDefCollation(NULL, def, defTypeId);
 				if (defCollId != attribute->attcollation)
 					ereport(ERROR,
@@ -1832,8 +1833,8 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 							 errmsg("column \"%s\" has a type conflict",
 									attributeName),
 							 errdetail("%s versus %s",
-									   TypeNameToString(def->typeName),
-									   TypeNameToString(newdef->typeName))));
+									   format_type_with_typemod(defTypeId, deftypmod),
+									   format_type_with_typemod(newTypeId, newtypmod))));
 				defcollid = GetColumnDefCollation(NULL, def, defTypeId);
 				newcollid = GetColumnDefCollation(NULL, newdef, newTypeId);
 				if (defcollid != newcollid)

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Amit Langote (#1)

Re: MergeAttributes type (mod) conflict error detail

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

I wonder if the following error detail text could say more than it does
currently for the following rather artificial example case:

CREATE TABLE p1(a char(3));
CREATE TABLE p2(a char(2));

CREATE TABLE c(d int) INHERITS (p1, p2);
NOTICE: merging multiple inherited definitions of column "a"
ERROR: inherited column "a" has a type conflict
DETAIL: character versus character

Any specific reason why it doesn't spell out typmods in the above detail
message?

I agree this could stand to be improved. There are probably a couple of
reasons why this code is like it is:

* We did not use to have format_type_with_typemod(), only
format_type_be(). That's easily changed now of course.

* There's a rough policy in the parser to prefer TypeNameToString
when complaining about a TypeName input, rather than reconstructing
the type name from the OID.  The reason for this is that we'd rather
complain about the type name as entered, not the canonical type name
--- for example, if the user typed "float8" it might be a bit confusing
if the parser then complains about "double precision".

I'm not entirely sure though that that argument should be applied
to this particular case, because the other type referred to will
certainly get displayed in canonical form.

So we could either apply your patch as written, or we could replace
only the format_type_be calls with format_type_with_typemod, and
then fix TypeNameToString so that it will show the typmod if any.
(We'd need to consider whether that behavior is OK for all callers.)

Even if we decide this particular case is best handled by presenting
canonical type names on both sides, maybe it would be wise to look
into whether TypeNameToString should be changed for other callers.

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

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Tom Lane (#2)

Re: MergeAttributes type (mod) conflict error detail

I wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

Any specific reason why it doesn't spell out typmods in the above detail
message?

* There's a rough policy in the parser to prefer TypeNameToString
when complaining about a TypeName input, rather than reconstructing
the type name from the OID.  The reason for this is that we'd rather
complain about the type name as entered, not the canonical type name
--- for example, if the user typed "float8" it might be a bit confusing
if the parser then complains about "double precision".
I'm not entirely sure though that that argument should be applied
to this particular case, because the other type referred to will
certainly get displayed in canonical form.

On reflection, I think trying to spell both types according to the same
rules will be the least confusing behavior here.

So we could either apply your patch as written, or we could replace
only the format_type_be calls with format_type_with_typemod, and
then fix TypeNameToString so that it will show the typmod if any.
(We'd need to consider whether that behavior is OK for all callers.)

Even if we decide this particular case is best handled by presenting
canonical type names on both sides, maybe it would be wise to look
into whether TypeNameToString should be changed for other callers.

I looked through the other call sites for TypeNameToString and
TypeNameListToString. In none of them does it seem useful to include any
typmod info in the printout, and in many of them it would be positively
misleading (e.g., functions do not care about typmod decorations on their
argument types). So we should not change the behavior of those functions.
Perhaps down the road there'll be a use for "TypeNameAndTypmodToString",
but I don't feel a need for it today.

So I am thinking your patch is good as proposed, ie, let's use
format_type_with_typemod here.

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

In reply to: Tom Lane (#3)

Re: MergeAttributes type (mod) conflict error detail

On 2015/12/27 3:11, Tom Lane wrote:

I wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

Any specific reason why it doesn't spell out typmods in the above detail
message?
* There's a rough policy in the parser to prefer TypeNameToString
when complaining about a TypeName input, rather than reconstructing
the type name from the OID.  The reason for this is that we'd rather
complain about the type name as entered, not the canonical type name
--- for example, if the user typed "float8" it might be a bit confusing
if the parser then complains about "double precision".
I'm not entirely sure though that that argument should be applied
to this particular case, because the other type referred to will
certainly get displayed in canonical form.
On reflection, I think trying to spell both types according to the same
rules will be the least confusing behavior here.

So we could either apply your patch as written, or we could replace
only the format_type_be calls with format_type_with_typemod, and
then fix TypeNameToString so that it will show the typmod if any.
(We'd need to consider whether that behavior is OK for all callers.)

Even if we decide this particular case is best handled by presenting
canonical type names on both sides, maybe it would be wise to look
into whether TypeNameToString should be changed for other callers.

I looked through the other call sites for TypeNameToString and
TypeNameListToString. In none of them does it seem useful to include any
typmod info in the printout, and in many of them it would be positively
misleading (e.g., functions do not care about typmod decorations on their
argument types). So we should not change the behavior of those functions.
Perhaps down the road there'll be a use for "TypeNameAndTypmodToString",
but I don't feel a need for it today.

So I am thinking your patch is good as proposed, ie, let's use
format_type_with_typemod here.

I agree. Thanks for adding the tests.

Regards,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers