Foreign key type checking patch

Hello again patchers,

Here is a proposed patch against 7.4.1 to check exact match
of foreign key types wrt the referenced keys, and to show
a warning if this is not the case.

This is an attempt to prevent stupid bugs such as :

CREATE TABLE foo(id INT4 NOT NULL PRIMARY KEY);
CREATE TABLE bla(id INT2 REFERENCES foo);

which may work at the beginning, and then fails later on.

I'm not at ease with postgresql internals, however this
implementation seems reasonnable to me, and in the spirit
of how the surrounding code works.

I could not find any simple way to tell the user about
what is being processed, as there is not real context information
and tell 'while processing this constraint'... However this
situation seems to be the normal case with any postgresql
messages, as far as I can tell from my use.

Have a nice day,

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

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> Here is a proposed patch against 7.4.1 to check exact match
> of foreign key types wrt the referenced keys, and to show
> a warning if this is not the case.

I think that this concern may be obsolete in CVS tip, at least for the
cases where we have indexable cross-type operators. The correct way
to do this would be to look at the operator found by oper() and see
whether it's indexable.

regards, tom lane

Dear Tom,

Thanks for your reply.

> > Here is a proposed patch against 7.4.1 to check exact match
> > of foreign key types wrt the referenced keys, and to show
> > a warning if this is not the case.
>
> I think that this concern may be obsolete in CVS tip,

I just get the current CVS and had a quick look at it.

> at least for the cases where we have indexable cross-type operators.
> The correct way to do this would be to look at the operator found by
> oper() and see whether it's indexable.

I must admit that I do not understand your point.

I wish I would have a WARNING if a foreign key is not declared exactly as
the key it references. I think that it is a desirable feature for stupid
users, including myself!

I cannot see why whether the "=" comparison version which is chosen is
indexable or not would lead to this information. It seems quite
reasonnable to look directly at the attribute types and compare them for
this purpose.

I noticed the compatible_oper() function which would return a no-coersion
binary operator between types. However that does not fit my purpose. For
instance, it seems to me that the IsBinaryCoercible returns true for
VARCHAR(12) and VARCHAR(16), as the type oid is the same, but I think a
warning makes sense anyway. So it is not the same issue.

So I can't see your point. Maybe some more lights would help?

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> I must admit that I do not understand your point.

> I wish I would have a WARNING if a foreign key is not declared exactly as
> the key it references.

The reason why people want such a warning is that the RI checks tend to
be slow if it's not the case. Accordingly, the warning should only
appear if the check is actually going to be slow.

You sound like you think it's a bug that Postgres supports cross-type FK
references at all. I disagree. It's a feature, albeit one whose
implementation could stand improvement. The warning ought to come out
in cases where people are going to be exposed to the implementation
weaknesses.

regards, tom lane

On Mon, 1 Mar 2004, Fabien COELHO wrote:

> > > Here is a proposed patch against 7.4.1 to check exact match
> > > of foreign key types wrt the referenced keys, and to show
> > > a warning if this is not the case.
> >
> > I think that this concern may be obsolete in CVS tip,
>
> I just get the current CVS and had a quick look at it.
>
> > at least for the cases where we have indexable cross-type operators.
> > The correct way to do this would be to look at the operator found by
> > oper() and see whether it's indexable.
>
> I must admit that I do not understand your point.
>
> I wish I would have a WARNING if a foreign key is not declared exactly as
> the key it references. I think that it is a desirable feature for stupid
> users, including myself!
>
> I cannot see why whether the "=" comparison version which is chosen is
> indexable or not would lead to this information. It seems quite
> reasonnable to look directly at the attribute types and compare them for
> this purpose.
>
> I noticed the compatible_oper() function which would return a no-coersion
> binary operator between types. However that does not fit my purpose. For
> instance, it seems to me that the IsBinaryCoercible returns true for
> VARCHAR(12) and VARCHAR(16), as the type oid is the same, but I think a
> warning makes sense anyway. So it is not the same issue.
>
> So I can't see your point. Maybe some more lights would help?

Currently the cross-type indexing issues cause us problems in performance
and so warning was considered reasonable so that it would warn people
about that problem. Tom's suggestion has effectively the same effect,
warn when there might be an issue on the performance grounds.

I'm really not sure that it makes sense to warn for the fk cases where the
semantics should be correct (if they're not we need to fix it or make it
an error) but in which an error might have been made by the user because
the types are different given that it at least seems reasonable to me that
the fk type is allowable to be a subset of the referenced type. I don't
think simply different types is sufficient to be warning material.

Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
> I'm really not sure that it makes sense to warn for the fk cases where the
> semantics should be correct (if they're not we need to fix it or make it
> an error) but in which an error might have been made by the user because
> the types are different given that it at least seems reasonable to me that
> the fk type is allowable to be a subset of the referenced type. I don't
> think simply different types is sufficient to be warning material.

I can think of several cases where it might be reasonable for the types
to be different. One case in particular that needs some thought is
where the FK and referenced PK are domains on a common base type.

regards, tom lane

> > I wish I would have a WARNING if a foreign key is not declared exactly as
> > the key it references.
>
> The reason why people want such a warning is that the RI checks tend to
> be slow if it's not the case. Accordingly, the warning should only
> appear if the check is actually going to be slow.

Ok. I understand this, and this is another issue.

> You sound like you think it's a bug that Postgres supports cross-type FK
> references at all.

I'm sorry I sound like that, because this is not what I really mean.
Maybe my poor English could stand some improvement so as to be able
to convey my thinking clearly to others.

> I disagree.

I totally agree with you.

This is not my point. I do not want to "disallow" cross-type foreign keys.

My point is just to *** WARN *** the stupid user (me) about potential
would-be bugs if the type does not match. A warning is not an error. It
just means "hey man, did you notice that? maybe that is not what you
meant, maybe it is what you meant...". Well, this is my idea of a warning.

Consider this example :

CREATE TABLE foo(fid VARCHAR(2) NOT NULL PRIMARY KEY);
CREATE TABLE bla(fid VARCHAR(4) REFERENCES foo);

Although the "fid" attribute is declared VARCHAR(4) in bla, you will
never be able to put more that a VARCHAR(2) value inside as it must
reference the foo table.

Now consider this other example :

CREATE TABLE foo(fid VARCHAR(2) NOT NULL PRIMARY KEY);
CREATE TABLE bla(fid VARCHAR(1) REFERENCES foo);

Some key of foo will never be able to be referenced by bla.
Maybe it is the intent of the user, maybe not.

Anyway, IMVHO, both these cases deserve a warning, even of there is no
actual cpu cost. Hence my patch proposal.

> It's a feature, albeit one whose implementation could stand improvement.
> The warning ought to come out in cases where people are going to be
> exposed to the implementation weaknesses.

I agree with the feature, I just wish there would be a warning.

The "implementation weaknesses" is another issue that I do understand
but that I did not try to address in the submitted patch.

--
Fabien.

> I can think of several cases where it might be reasonable for the types
> to be different.

Sure. It's all about a warning, not about an error.

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

Hello again,

I turn the discussion to the dev list as it seems more appropriate.

So about the proposed patch to warn if foreign key type do not match the
target key:

> Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
> > I'm really not sure that it makes sense to warn for the fk cases where the
> > semantics should be correct (if they're not we need to fix it or make it
> > an error) but in which an error might have been made by the user because
> > the types are different given that it at least seems reasonable to me that
> > the fk type is allowable to be a subset of the referenced type. I don't
> > think simply different types is sufficient to be warning material.
>
> I can think of several cases where it might be reasonable for the types
> to be different. One case in particular that needs some thought is
> where the FK and referenced PK are domains on a common base type.

I'm looking forward to see an example where:

1) the difference in type is actually needed by the application.

2) a simple warning about the issue would be considered harmful.

Let me describe some examples where IMVVHO a simple warning make sense,
although they are silently accepted by postgres at the moment:

1/ integers

CREATE TABLE foo(fid INT4 NOT NULL PRIMARY KEY, ...);
CREATE TABLE bla(fid INT2 REFERENCES foo, ...);

The application will be fine till you enter fid=32767, and
it inserts will fail in bla with fid=32768. Much later on.

2/ chars

CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
CREATE TABLE bla(fid VARCHAR(2) REFERENCES foo, ...);

bla will be able to reference all 2-letters keys of foo, but no more.
If you have some counter in foo, it will fail when it turns 3 letters.

3/ chars

CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
CREATE TABLE bla(fid VARCHAR(8) REFERENCES foo, ...);

declaring a larger size is not a problem here, however you will
never be able to but any reference in bla larger than 4 as it must
match its counter part in foo. So it is just a little bit stupid.

4/ time

CREATE TABLE day(quand DATE NOT NULL PRIMARY KEY, ...);
CREATE TABLE event(quand TIMESTAMP REFERENCES day, ...);

The intent could be that events should refer to some day already
registered in the base. Obviously it does work, because the = will cast to
timestamp, to only the 00:00:00 timestamp will match a day.

etc.

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

On Tue, 2 Mar 2004, Fabien COELHO wrote:

>
> Hello again,
>
> I turn the discussion to the dev list as it seems more appropriate.
>
> So about the proposed patch to warn if foreign key type do not match the
> target key:
>
> > Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
> > > I'm really not sure that it makes sense to warn for the fk cases where the
> > > semantics should be correct (if they're not we need to fix it or make it
> > > an error) but in which an error might have been made by the user because
> > > the types are different given that it at least seems reasonable to me that
> > > the fk type is allowable to be a subset of the referenced type. I don't
> > > think simply different types is sufficient to be warning material.
> >
> > I can think of several cases where it might be reasonable for the types
> > to be different. One case in particular that needs some thought is
> > where the FK and referenced PK are domains on a common base type.
>
>
> I'm looking forward to see an example where:
>
> 1) the difference in type is actually needed by the application.
>
> 2) a simple warning about the issue would be considered harmful.
>
>
> Let me describe some examples where IMVVHO a simple warning make sense,
> although they are silently accepted by postgres at the moment:
>
> 1/ integers
>
> CREATE TABLE foo(fid INT4 NOT NULL PRIMARY KEY, ...);
> CREATE TABLE bla(fid INT2 REFERENCES foo, ...);
>
> The application will be fine till you enter fid=32767, and
> it inserts will fail in bla with fid=32768. Much later on.

Which is fine if bla is meant to store a subset of the allowable foo
values. It'd be really hard to say at bla creation time that there isn't
going to be a bla2 which say takes an int4 check (fid>=32768) which might
be being used for dividing up the foo space between multiple tables.

> 2/ chars
>
> CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
> CREATE TABLE bla(fid VARCHAR(2) REFERENCES foo, ...);
>
> bla will be able to reference all 2-letters keys of foo, but no more.
> If you have some counter in foo, it will fail when it turns 3 letters.

Same as above.

> 3/ chars
>
> CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
> CREATE TABLE bla(fid VARCHAR(8) REFERENCES foo, ...);
>
> declaring a larger size is not a problem here, however you will
> never be able to but any reference in bla larger than 4 as it must
> match its counter part in foo. So it is just a little bit stupid.

This one is fairly pointless for the single column case but a multiple
column match unspecified constraint could allow the full 8 characters if
there's a second column which is null.

> 4/ time
>
> CREATE TABLE day(quand DATE NOT NULL PRIMARY KEY, ...);
> CREATE TABLE event(quand TIMESTAMP REFERENCES day, ...);
>
> The intent could be that events should refer to some day already
> registered in the base. Obviously it does work, because the = will cast to
> timestamp, to only the 00:00:00 timestamp will match a day.

This one does seem fairly broken.

5/ domains

CREATE DOMAIN posint AS int4 check(value>0);
CREATE TABLE foo(fid int4 primary key);
CREATE TABLE bla(fid posint references foo);

The intent here is that foo may contain negative numbers but that those
rows won't be referenced by bla. This is similar to 1 and 2.

Hello Stephan,

> > CREATE TABLE foo(fid INT4 NOT NULL PRIMARY KEY, ...);
> > CREATE TABLE bla(fid INT2 REFERENCES foo, ...);
> >
> > The application will be fine till you enter fid=32767, and
> > it inserts will fail in bla with fid=32768. Much later on.
>
> Which is fine if bla is meant to store a subset of the allowable foo
> values.
> [...]

Sure. This is NOT my point. I totally agree with you that the above
example MAY BE what the user intends, and that it must be allowed.
However it may ALSO be a bug that will pop up later on.

Although it is POSSIBLE that this is fine, it is much more PROBABLE that
it is a bug, hence I just suggest to issue a mere simple basic plain
user-friendly little warning, what is quite different from issuing an
error.

Thus, the user has the information. He may chose to go on as that is what
was meant, or maybe check the stuff and correct it.

In postgres compilation, gcc uses the -Wall option to issue warnings about
correct C constructs that may hide application bugs. This is the
philosophy I'm suggesting here for this very small feature.
"Dear user, what you ask is right, however it looks a little bit strange,
so I tell you just in case." I'm sure you're pretty happy that the gcc
developers put such features for basic programmers, and that you use
them;-) Why not allowing that kind of approach in postgres?

> > CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
> > CREATE TABLE bla(fid VARCHAR(8) REFERENCES foo, ...);
> >
> > declaring a larger size is not a problem here, however you will
> > never be able to but any reference in bla larger than 4 as it must
> > match its counter part in foo. So it is just a little bit stupid.
>
> This one is fairly pointless

Isn't it what I'm saying?

> for the single column case but a multiple column match unspecified
> constraint could allow the full 8 characters if there's a second column
> which is null.

I do not understand. I can't see how you can put 8 characters in a
reference which must match a 4 characters string.

> > CREATE TABLE day(quand DATE NOT NULL PRIMARY KEY, ...);
> > CREATE TABLE event(quand TIMESTAMP REFERENCES day, ...);
> >
> > The intent could be that events should refer to some day already
> > registered in the base. Obviously it does work, because the = will cast to
> > timestamp, to only the 00:00:00 timestamp will match a day.
>
> This one does seem fairly broken.

Yes, my comment is broken, as I wrote "it does work", meaning, "it does
not work":-(

Anyway postgres accepts the two tables and the foreign key references from
a timestamp to a date without a warning, and although I can imagine an
application that would really mean that, I'm pretty sure most users would
like this reported as a warning, as it is more likely to be a bug.

> CREATE DOMAIN posint AS int4 check(value>0);
> CREATE TABLE foo(fid int4 primary key);
> CREATE TABLE bla(fid posint references foo);
>
> The intent here is that foo may contain negative numbers but that those
> rows won't be referenced by bla. This is similar to 1 and 2.

Sure, I agree with you there are example where you may mean that.
My point is about a ***warning***, because I think that most of the time
this hides a future bug, even if some time it may be the intent.

Have a nice day,

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

On Tue, 2 Mar 2004, Fabien COELHO wrote:

>
> Hello Stephan,
>
> > > CREATE TABLE foo(fid INT4 NOT NULL PRIMARY KEY, ...);
> > > CREATE TABLE bla(fid INT2 REFERENCES foo, ...);
> > >
> > > The application will be fine till you enter fid=32767, and
> > > it inserts will fail in bla with fid=32768. Much later on.
> >
> > Which is fine if bla is meant to store a subset of the allowable foo
> > values.
> > [...]
>
> Sure. This is NOT my point. I totally agree with you that the above
> example MAY BE what the user intends, and that it must be allowed.
> However it may ALSO be a bug that will pop up later on.
>
> Although it is POSSIBLE that this is fine, it is much more PROBABLE that
> it is a bug, hence I just suggest to issue a mere simple basic plain
> user-friendly little warning, what is quite different from issuing an
> error.
>
> Thus, the user has the information. He may chose to go on as that is what
> was meant, or maybe check the stuff and correct it.
>
> In postgres compilation, gcc uses the -Wall option to issue warnings about
> correct C constructs that may hide application bugs. This is the
> philosophy I'm suggesting here for this very small feature.
> "Dear user, what you ask is right, however it looks a little bit strange,
> so I tell you just in case." I'm sure you're pretty happy that the gcc
> developers put such features for basic programmers, and that you use
> them;-) Why not allowing that kind of approach in postgres?

Because producing noise warnings often *lower* the amount of use you get
from real warnings. If one has to wade through useless messages to divine
the ones that are meaningful, overall many people just start ignoring all
warnings. I could be convinced that it is "notice" material, since people
who don't want to see it probably don't want to see the other notices
either, but warning seems way to strong to me.

Fundamentally, I don't see a huge difference between this and
select * from foo,bla where foo.fid=bla.fid;
where the same general constraints on meaningful values apply.

> > > CREATE TABLE foo(fid VARCHAR(4) NOT NULL PRIMARY KEY, ...);
> > > CREATE TABLE bla(fid VARCHAR(8) REFERENCES foo, ...);
> > >
> > > declaring a larger size is not a problem here, however you will
> > > never be able to but any reference in bla larger than 4 as it must
> > > match its counter part in foo. So it is just a little bit stupid.
> >
> > This one is fairly pointless
>
> Isn't it what I'm saying?
>
> > for the single column case but a multiple column match unspecified
> > constraint could allow the full 8 characters if there's a second column
> > which is null.
>
> I do not understand. I can't see how you can put 8 characters in a
> reference which must match a 4 characters string.

Because in match unspecified, any column being null means the remainder of
the columns are not checked against the other table. IE given a key of
(fid, other), ('abcdefg', null) is valid in match unspecified even if the
other key can only have 4 characters.

Dear Stephan,

> > Although it is POSSIBLE that this is fine, it is much more PROBABLE that
> > it is a bug, hence I just suggest to issue a mere simple basic plain
> > user-friendly little warning, what is quite different from issuing an
> > error. [...] Why not allowing that kind of approach in postgres?
>
> Because producing noise warnings often *lower* the amount of use you get
> from real warnings.

Ok, I understand this argument. I agree with the general idea...
However I think that this case is special enough in the sense that:

(1) it should occur rarely, and when it occurs

(2) it should be most of the time appropriate.

> If one has to wade through useless messages to divine the ones that are
> meaningful, overall many people just start ignoring all warnings. I
> could be convinced that it is "notice" material, since people who don't
> want to see it probably don't want to see the other notices either, but
> warning seems way to strong to me.

Well, I used "NOTICE" in my suggested implementation, so it is fine;-)

> Fundamentally, I don't see a huge difference between this and
> select * from foo,bla where foo.fid=bla.fid;
> where the same general constraints on meaningful values apply.

1/ The warning I'm suggesting is ONCE in the life-time of the table,
when it is declared. Your above case would be EVERY TIME a join
is issued on the tables, which is likely to be quite often!

2/ If you use a select with a join, and if your modelisation is
correct, then you must have declared a foreign key on your table,
so you already get the warning at that time. If you chose to
ignore it, you must be right!

3/ If you bother to declare foreign keys, you want them to be sound,
so if postgres can help a little bit, that should not harm.

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