Обсуждение: MergeAttributes type (mod) conflict error detail

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

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

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

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