Re: notice about costly ri checks (2)

Dear patchers,

Here is my second and last try of the day.

This patch adds a "notice" at constraint creation time if the referential
integrity check is to be "costly", that is it cannot use the index due to
some incompatibility.

The patch was generated with the "difforig" script against the current cvs
head.

I put much validation which looks fine to me, but it is only me.

I'm not that satisfied with the wording and the content of the error
message. Any better suggestion would be welcome.

Have a nice day,

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Fabien COELHO wrote:
>
> Dear patchers,
>
> Here is my second and last try of the day.
>
> This patch adds a "notice" at constraint creation time if the referential
> integrity check is to be "costly", that is it cannot use the index due to
> some incompatibility.
>
> The patch was generated with the "difforig" script against the current cvs
> head.
>
> I put much validation which looks fine to me, but it is only me.
>
> I'm not that satisfied with the wording and the content of the error
> message. Any better suggestion would be welcome.

Agreed. The current text is:

NOTICE: costly cross-type foreign key because of component 1

Seems we should say something like:

NOTICE: foreign key constraint 'constrname' must use a costly cross-type conversion

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Mar 5, 2004, at 1:49 PM, Bruce Momjian wrote:

> Agreed. The current text is:
>
> NOTICE: costly cross-type foreign key because of component 1
>
> Seems we should say something like:
>
> NOTICE: foreign key constraint 'constrname' must use a costly
> cross-type conversion

It seems to me that in some ways this is similar to the situation where
indexes are created to enforce a UNIQUE constraint. Indexes also incur
additional overhead for inserts and updates, but make no mention of the
cost: the DBA is assumed to know that, or they can check the docs if
they're interested in why such a notice is being raised. I'd think
something as simple as

NOTICE: foreign key constraint 'constrname' will require a cross-type
conversion

similar to
NOTICE: CREATE TABLE / UNIQUE will create implicit index
"foox_interesting_key" for table "foox"

Michael Glaesemann
grzm myrealbox com

On Mar 5, 2004, at 1:49 PM, Bruce Momjian wrote:
> NOTICE: foreign key constraint 'constrname' must use a costly
> cross-type conversion

On Fri, 5 Mar 2004, Michael Glaesemann wrote:
> NOTICE: foreign key constraint 'constrname' will require a cross-type
> conversion

Ok. I'm going to look for the constraint name and re-submit.

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Michael Glaesemann wrote:
>
> On Mar 5, 2004, at 1:49 PM, Bruce Momjian wrote:
>
> > Agreed. The current text is:
> >
> > NOTICE: costly cross-type foreign key because of component 1
> >
> > Seems we should say something like:
> >
> > NOTICE: foreign key constraint 'constrname' must use a costly
> > cross-type conversion
>
> It seems to me that in some ways this is similar to the situation where
> indexes are created to enforce a UNIQUE constraint. Indexes also incur
> additional overhead for inserts and updates, but make no mention of the
> cost: the DBA is assumed to know that, or they can check the docs if
> they're interested in why such a notice is being raised. I'd think
> something as simple as
>
> NOTICE: foreign key constraint 'constrname' will require a cross-type
> conversion
>
> similar to
> NOTICE: CREATE TABLE / UNIQUE will create implicit index
> "foox_interesting_key" for table "foox"

The issue is that an index always has a cost (pretty constant cost),
which is known to the creator. The case he is warning about is when
primary/foreign key types don't match, and a costly comparison will be
required to do the referential integrity checking.

Also, seems this should be a WARNING, rather than a notice. NOTICE, I
think, is for normal behavior (creating a sequence for SERIAL), and
warning is for unusual behavior, which this is.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Fabien COELHO wrote:
>
>
> On Mar 5, 2004, at 1:49 PM, Bruce Momjian wrote:
> > NOTICE: foreign key constraint 'constrname' must use a costly
> > cross-type conversion
>
> On Fri, 5 Mar 2004, Michael Glaesemann wrote:
> > NOTICE: foreign key constraint 'constrname' will require a cross-type
> > conversion
>
> Ok. I'm going to look for the constraint name and re-submit.

The reason I think we have to mention the constraint name is that you
could have a multi-column primary/foreign key, so instead of mentioning
each column, we just mention the constraint name, which should be easy
to identify.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

> The reason I think we have to mention the constraint name is that you
> could have a multi-column primary/foreign key, so instead of mentioning
> each column, we just mention the constraint name, which should be easy
> to identify.

Sure. See attempt (3). However it is still a "NOTICE".
Should I make version (4) with a WARNING ?

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Fabien COELHO wrote:
>
> > The reason I think we have to mention the constraint name is that you
> > could have a multi-column primary/foreign key, so instead of mentioning
> > each column, we just mention the constraint name, which should be easy
> > to identify.
>
> Sure. See attempt (3). However it is still a "NOTICE".
> Should I make version (4) with a WARNING ?

I can easily make the change before applying the patch.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

> > Should I make version (4) with a WARNING ?
>
> I can easily make the change before applying the patch.

Sure. The patche also include regression tests results with "NOTICE".

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The reason I think we have to mention the constraint name is that you
> could have a multi-column primary/foreign key, so instead of mentioning
> each column, we just mention the constraint name, which should be easy
> to identify.

However, the complaint will be about one single column being of a
non-matching type. In the case of a multicolumn foreign key, giving
only the constraint name is unhelpful. Even for a one-column key,
it's not obvious to me why the constraint name is better than the column
name.

[ thinks... ] I guess it could be that the same column is being used in
several different FK constraints, so if we just give column names then
it would also be important to mention the referenced column.

I'd suggest something along the lines of

NOTICE: foreign key constraint "constrname" will require a cross-type conversion
DETAIL: key columns "fkcol" and "pkcol" are of different types integer and double precision

if you want to be really complete.

I've got mixed feelings about the WARNING-vs-NOTICE issue. WARNING
seems too strong, like we are trying to tell them that it won't work at
all. Particularly with text like the above, which completely fails to
explain that the problem is only one of speed and not functionality.
If you want it to be a WARNING then we gotta work on the text some more.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The reason I think we have to mention the constraint name is that you
> > could have a multi-column primary/foreign key, so instead of mentioning
> > each column, we just mention the constraint name, which should be easy
> > to identify.
>
> However, the complaint will be about one single column being of a
> non-matching type. In the case of a multicolumn foreign key, giving
> only the constraint name is unhelpful. Even for a one-column key,
> it's not obvious to me why the constraint name is better than the column
> name.
>
> [ thinks... ] I guess it could be that the same column is being used in
> several different FK constraints, so if we just give column names then
> it would also be important to mention the referenced column.
>
> I'd suggest something along the lines of
>
> NOTICE: foreign key constraint "constrname" will require a cross-type conversion
> DETAIL: key columns "fkcol" and "pkcol" are of different types integer and double precision

I suggested the constraint name because of multi-column keys, where he
would have to print an arbitrary number of columns in the message. It
didn't seem worth doing that work. I see your idea of just printing the
column, but that doesn't really point to the primary/foreign key
relationship. If the user can't figure out which columns are a mismatch
from the constraint name, they have larger problems than this. :-)

> if you want to be really complete.
>
> I've got mixed feelings about the WARNING-vs-NOTICE issue. WARNING
> seems too strong, like we are trying to tell them that it won't work at
> all. Particularly with text like the above, which completely fails to
> explain that the problem is only one of speed and not functionality.
> If you want it to be a WARNING then we gotta work on the text some more.

Yes, let's re-add 'costly' to the text:

> WARNING: foreign key constraint "constrname" will require a costly cross-type conversion

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> I'd suggest something along the lines of
>>
>> NOTICE: foreign key constraint "constrname" will require a cross-type conversion
>> DETAIL: key columns "fkcol" and "pkcol" are of different types integer and double precision

> I suggested the constraint name because of multi-column keys, where he
> would have to print an arbitrary number of columns in the message. It
> didn't seem worth doing that work. I see your idea of just printing the
> column, but that doesn't really point to the primary/foreign key
> relationship. If the user can't figure out which columns are a mismatch
> from the constraint name, they have larger problems than this. :-)

Why should we make them guess which column is the problem, when we know
it perfectly well?

>> If you want it to be a WARNING then we gotta work on the text some more.

> Yes, let's re-add 'costly' to the text:

> WARNING: foreign key constraint "constrname" will require a costly cross-type conversion

Works for me, but I still want the DETAIL ...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> I'd suggest something along the lines of
> >>
> >> NOTICE: foreign key constraint "constrname" will require a cross-type conversion
> >> DETAIL: key columns "fkcol" and "pkcol" are of different types integer and double precision
>
> > I suggested the constraint name because of multi-column keys, where he
> > would have to print an arbitrary number of columns in the message. It
> > didn't seem worth doing that work. I see your idea of just printing the
> > column, but that doesn't really point to the primary/foreign key
> > relationship. If the user can't figure out which columns are a mismatch
> > from the constraint name, they have larger problems than this. :-)
>
> Why should we make them guess which column is the problem, when we know
> it perfectly well?

OK.

> >> If you want it to be a WARNING then we gotta work on the text some more.
>
> > Yes, let's re-add 'costly' to the text:
>
> > WARNING: foreign key constraint "constrname" will require a costly cross-type conversion
>
> Works for me, but I still want the DETAIL ...

Sounds good.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, 5 Mar 2004, Tom Lane wrote:

> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> I'd suggest something along the lines of
> >>
> >> NOTICE: foreign key constraint "constrname" will require a cross-type conversion
> >> DETAIL: key columns "fkcol" and "pkcol" are of different types integer and double precision
>
> > I suggested the constraint name because of multi-column keys, where he
> > would have to print an arbitrary number of columns in the message. It
> > didn't seem worth doing that work. I see your idea of just printing the
> > column, but that doesn't really point to the primary/foreign key
> > relationship. If the user can't figure out which columns are a mismatch
> > from the constraint name, they have larger problems than this. :-)
>
> Why should we make them guess which column is the problem, when we know
> it perfectly well?

As a side question, if there are multiple cross-type conversions in one
constraint on different column pairs, what do we think the message should
be? One message with multiple column mentions in detail or multiple
notices? (I haven't looked at the patch to see if one or the other is
easier with how it's set up)

Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
>> Why should we make them guess which column is the problem, when we know
>> it perfectly well?

> As a side question, if there are multiple cross-type conversions in one
> constraint on different column pairs, what do we think the message should
> be? One message with multiple column mentions in detail or multiple
> notices? (I haven't looked at the patch to see if one or the other is
> easier with how it's set up)

I would expect it to generate one WARNING for each mismatch; doing
anything else would make life a lot more complex, both as to writing the
code and as to formatting the output readably.

regards, tom lane

> > As a side question, if there are multiple cross-type conversions in one
> > constraint on different column pairs, what do we think the message should
> > be? One message with multiple column mentions in detail or multiple
> > notices? (I haven't looked at the patch to see if one or the other is
> > easier with how it's set up)
>
> I would expect it to generate one WARNING for each mismatch; doing
> anything else would make life a lot more complex, both as to writing the
> code and as to formatting the output readably.

I agree.

patch v1 issued the warning once for each mismatch, in the check loop.

patch v2 issued the warning once for the foreign key, outside of the loop.

patch v3 is yet to come;-)

I'll re-submit a patch some time later, with a WARNING and mismatch
column names and types specified.

--
Fabien.