Thread: BUG #18295: In PostgreSQL a unique index on targeted columns is sufficient to support a foreign key

The following bug has been logged on the website:

Bug reference:      18295
Logged by:          Gilles PARC
Email address:      gparc@online.fr
PostgreSQL version: 16.1
Operating system:   Linux
Description:

Hello,

coming from Oracle, I'm surprised to see that in PostgreSQL, a foreign key
can be linked to a unique index
on the target table and not exclusively to a primary key constraint or
UNIQUE constraint.
Even if a primary/unique constraint implies the creation of a unique index,
semantically, it's not the same.

In Oracle, in that case (only unique index on targeted columns), we get the
following error : 
ORA-02270: no matching unique or primary key for this column-list

Reading the PostgreSQL documentation, I found this snippet in Chapter 5.4
(https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-FK)
: 
"A foreign key must reference columns that either are a primary key or form
a unique constraint. This means that the referenced columns always have an
index (the one underlying the primary key or unique constraint);"
which seems to suggest that in PostgreSQL also a foreign key should refer to
a primary/unique constraint on the target table.

Is it a bug or an intended feature ? If the latter, I think the doc should
be amended to remove any ambiguity.

Regards

P.S. by the way, I don't know what the SQL standard states about that.

Hello,

any comment ?

Regards

----- Mail original -----
De: "PG Bug reporting form" <noreply@postgresql.org>
À: "pgsql-bugs" <pgsql-bugs@lists.postgresql.org>
Cc: "gparc" <gparc@online.fr>
Envoyé: Lundi 15 Janvier 2024 12:37:19
Objet: BUG #18295: In PostgreSQL a unique index on targeted columns is sufficient to support a foreign key

The following bug has been logged on the website:

Bug reference:      18295
Logged by:          Gilles PARC
Email address:      gparc@online.fr
PostgreSQL version: 16.1
Operating system:   Linux
Description:

Hello,

coming from Oracle, I'm surprised to see that in PostgreSQL, a foreign key
can be linked to a unique index
on the target table and not exclusively to a primary key constraint or
UNIQUE constraint.
Even if a primary/unique constraint implies the creation of a unique index,
semantically, it's not the same.

In Oracle, in that case (only unique index on targeted columns), we get the
following error :
ORA-02270: no matching unique or primary key for this column-list

Reading the PostgreSQL documentation, I found this snippet in Chapter 5.4
(https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-FK)
:
"A foreign key must reference columns that either are a primary key or form
a unique constraint. This means that the referenced columns always have an
index (the one underlying the primary key or unique constraint);"
which seems to suggest that in PostgreSQL also a foreign key should refer to
a primary/unique constraint on the target table.

Is it a bug or an intended feature ? If the latter, I think the doc should
be amended to remove any ambiguity.

Regards

P.S. by the way, I don't know what the SQL standard states about that.

any comment ?

On Wed, 2024-01-24 at 11:11 +0100, gparc@free.fr wrote:
> coming from Oracle, I'm surprised to see that in PostgreSQL, a foreign key
> can be linked to a unique index
> on the target table and not exclusively to a primary key constraint or
> UNIQUE constraint.
>
> Is it a bug or an intended feature ? If the latter, I think the doc should
> be amended to remove any ambiguity.

Let's say it is an extension of the standard, but I cannot say if that is
intended or not.  At any rate, it has been like that for a very long time,
and changing it might make some users unhappy.

There is some added value, in that you could reference a unique index
that has an INCLUDE clause:

  CREATE TABLE parent (id integer, payload integer, other integer);

  CREATE UNIQUE INDEX ON parent (id) INCLUDE (payload);

  CREATE TABLE child (id integer REFERENCES parent (id));

So it might well be seen as a feature.

Looking at the source, the function comment suggests that that undocumented
feature may be there by accident:

/*
 * transformFkeyCheckAttrs -
 *
 *  Make sure that the attributes of a referenced table belong to a unique
 *  (or primary key) constraint.  Return the OID of the index supporting
 *  the constraint, as well as the opclasses associated with the index
 *  columns.
 */

The comment is speaking about a constraint, not a unique index.

So perhaps the comment should be updated, along with a note in the documentation
(in ddl.html and ref/create_table.sgml).


> P.S. by the way, I don't know what the SQL standard states about that.

That is simple: since the standard doesn't know indexes, it can only talk
about referencing a constraint.

Yours,
Laurenz Albe

----- Mail original -----
De: "Laurenz Albe" <laurenz.albe@cybertec.at>
À: "gparc" <gparc@free.fr>, "pgsql-bugs" <pgsql-bugs@lists.postgresql.org>
Envoyé: Mercredi 24 Janvier 2024 16:28:45
Objet: Re: BUG #18295: In PostgreSQL a unique index on targeted columns is sufficient to support a foreign key

On Wed, 2024-01-24 at 11:11 +0100, gparc@free.fr wrote:
> coming from Oracle, I'm surprised to see that in PostgreSQL, a foreign key
> can be linked to a unique index
> on the target table and not exclusively to a primary key constraint or
> UNIQUE constraint.
>
> Is it a bug or an intended feature ? If the latter, I think the doc should
> be amended to remove any ambiguity.

Let's say it is an extension of the standard, but I cannot say if that is
intended or not.  At any rate, it has been like that for a very long time,
and changing it might make some users unhappy.

There is some added value, in that you could reference a unique index
that has an INCLUDE clause:

  CREATE TABLE parent (id integer, payload integer, other integer);

  CREATE UNIQUE INDEX ON parent (id) INCLUDE (payload);

  CREATE TABLE child (id integer REFERENCES parent (id));

So it might well be seen as a feature.

Looking at the source, the function comment suggests that that undocumented
feature may be there by accident:

/*
 * transformFkeyCheckAttrs -
 *
 *  Make sure that the attributes of a referenced table belong to a unique
 *  (or primary key) constraint.  Return the OID of the index supporting
 *  the constraint, as well as the opclasses associated with the index
 *  columns.
 */

The comment is speaking about a constraint, not a unique index.

So perhaps the comment should be updated, along with a note in the documentation
(in ddl.html and ref/create_table.sgml).


> P.S. by the way, I don't know what the SQL standard states about that.

That is simple: since the standard doesn't know indexes, it can only talk
about referencing a constraint.

Yours,
Laurenz Albe


Thanks Laurenz for your detailed reply.
I agree also for an update of the documentation and source code.

Concerning, the documentation, I propose to modify in
https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-FK
the following sentence :
"A foreign key must reference columns that either are a primary key or form a unique constraint.
 This means that the referenced columns always have an index (the one underlying the primary key or unique
constraint);"
by
"A foreign key must reference columns that either are a primary key or form a unique constraint or are specified in a
uniqueindex. 
 This means that the referenced columns are always backed by a UNIQUE index."

Regards
Gilles

----- Mail original -----
> De: "gparc" <gparc@free.fr>
> À: "Laurenz Albe" <laurenz.albe@cybertec.at>
> Cc: "pgsql-bugs" <pgsql-bugs@lists.postgresql.org>
> Envoyé: Mercredi 24 Janvier 2024 17:01:04
> Objet: Re: BUG #18295: In PostgreSQL a unique index on targeted columns is sufficient to support a foreign key

> ----- Mail original -----
> De: "Laurenz Albe" <laurenz.albe@cybertec.at>
> À: "gparc" <gparc@free.fr>, "pgsql-bugs" <pgsql-bugs@lists.postgresql.org>
> Envoyé: Mercredi 24 Janvier 2024 16:28:45
> Objet: Re: BUG #18295: In PostgreSQL a unique index on targeted columns is
> sufficient to support a foreign key
>
> On Wed, 2024-01-24 at 11:11 +0100, gparc@free.fr wrote:
>> coming from Oracle, I'm surprised to see that in PostgreSQL, a foreign key
>> can be linked to a unique index
>> on the target table and not exclusively to a primary key constraint or
>> UNIQUE constraint.
>>
>> Is it a bug or an intended feature ? If the latter, I think the doc should
>> be amended to remove any ambiguity.
>
> Let's say it is an extension of the standard, but I cannot say if that is
> intended or not.  At any rate, it has been like that for a very long time,
> and changing it might make some users unhappy.
>
> There is some added value, in that you could reference a unique index
> that has an INCLUDE clause:
>
>  CREATE TABLE parent (id integer, payload integer, other integer);
>
>  CREATE UNIQUE INDEX ON parent (id) INCLUDE (payload);
>
>  CREATE TABLE child (id integer REFERENCES parent (id));
>
> So it might well be seen as a feature.
>
> Looking at the source, the function comment suggests that that undocumented
> feature may be there by accident:
>
> /*
> * transformFkeyCheckAttrs -
> *
> *  Make sure that the attributes of a referenced table belong to a unique
> *  (or primary key) constraint.  Return the OID of the index supporting
> *  the constraint, as well as the opclasses associated with the index
> *  columns.
> */
>
> The comment is speaking about a constraint, not a unique index.
>
> So perhaps the comment should be updated, along with a note in the documentation
> (in ddl.html and ref/create_table.sgml).
>
>
>> P.S. by the way, I don't know what the SQL standard states about that.
>
> That is simple: since the standard doesn't know indexes, it can only talk
> about referencing a constraint.
>
> Yours,
> Laurenz Albe
>
>
> Thanks Laurenz for your detailed reply.
> I agree also for an update of the documentation and source code.
>
> Concerning, the documentation, I propose to modify in
> https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-FK
> the following sentence :
> "A foreign key must reference columns that either are a primary key or form a
> unique constraint.
> This means that the referenced columns always have an index (the one underlying
> the primary key or unique constraint);"
> by
> "A foreign key must reference columns that either are a primary key or form a
> unique constraint or are specified in a unique index.
> This means that the referenced columns are always backed by a UNIQUE index."
>
> Regards
> Gilles

Is this new wording OK for you ?

Regards
Gilles

On Thu, 2024-01-25 at 16:36 +0100, gparc@free.fr wrote:
> Is this new wording OK for you ?

There are a few more places that should be updated, among them
the standard conformance of CREATE TABLE.

Attached is my suggested patch.

Yours,
Laurenz Albe

On Fri, 26 Jan 2024 at 23:55, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Attached is my suggested patch.

Why did you choose to remove the mention of primary key and unique
constraints in:

+++ b/doc/src/sgml/ddl.sgml
@@ -1317,9 +1317,7 @@ CREATE TABLE posts (
    </para>

    <para>
-    A foreign key must reference columns that either are a primary key or
-    form a unique constraint.  This means that the referenced columns always
-    have an index (the one underlying the primary key or unique constraint);
+    A foreign key must reference columns on which a unique index in defined,

but choose to keep them here:

+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1166,7 +1166,7 @@ WITH ( MODULUS <replaceable
class="parameter">numeric_literal</replaceable>, REM
       class="parameter">refcolumn</replaceable> list is omitted, the
       primary key of the <replaceable class="parameter">reftable</replaceable>
       is used.  The referenced columns must be the columns of a non-deferrable
-      unique or primary key constraint in the referenced table.  The user
+      unique or primary key constraint or a unique index in the
referenced table.  The user

I'd rather we continue to mention primary keys and unique constraints
in ddl.sgml.  It just seems good practice to me to define a constraint
and it seems better if people continue to do that to increase the
likelihood that their schema is compatible with another RDBMS.

David

On Sat, 2024-01-27 at 00:33 +1300, David Rowley wrote:
> On Fri, 26 Jan 2024 at 23:55, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > Attached is my suggested patch.
>
> Why did you choose to remove the mention of primary key and unique
> constraints in:
>
> -    A foreign key must reference columns that either are a primary key or
> -    form a unique constraint.  This means that the referenced columns always
> -    have an index (the one underlying the primary key or unique constraint);
> +    A foreign key must reference columns on which a unique index in defined,
>
> but choose to keep them here:
>
> -      unique or primary key constraint in the referenced table.  The user
> +      unique or primary key constraint or a unique index in the
>
> I'd rather we continue to mention primary keys and unique constraints
> in ddl.sgml.  It just seems good practice to me to define a constraint
> and it seems better if people continue to do that to increase the
> likelihood that their schema is compatible with another RDBMS.

I removed the mention of constraints for simplicity, but I agree that the
documentation should encourage users to reference constraints.

Attached is a modified patch.

Yours,
Laurenz Albe

On Sat, 27 Jan 2024 at 01:14, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Attached is a modified patch.

I think it looks mostly fine.

I'd only adjust the following addition to be a new paragraph:

-   <title>Foreign-Key Constraint Actions</title>
+   <title>Foreign-Key Constraints</title>

    <para>
     The ability to specify column lists in the foreign-key actions
     <literal>SET DEFAULT</literal> and <literal>SET NULL</literal> is a
     <productname>PostgreSQL</productname> extension.
+    It is also a <productname>PostgreSQL</productname> extension that a
+    foreign key constraint may reference a unique index instead of a
+    primary key or unique constraint.
    </para>

and drop the "also" at the same time.

I also noticed that, generally, we're not that consistent if we spell
it "foreign-key" or "foreign key".  You're introducing "foreign key"
in a location where there are a couple of "foreign-key"s. Maybe it's
better to be consistent in at least that location?

David

On Sat, 2024-01-27 at 02:19 +1300, David Rowley wrote:
> On Sat, 27 Jan 2024 at 01:14, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> I'd only adjust the following addition to be a new paragraph:

Check.

> and drop the "also" at the same time.

Done.

> I also noticed that, generally, we're not that consistent if we spell
> it "foreign-key" or "foreign key".  You're introducing "foreign key"
> in a location where there are a couple of "foreign-key"s. Maybe it's
> better to be consistent in at least that location?

Yes, you are right.  I noticed that everywhere else on the page the
form "foreign key" is used, so that's what I did in the attached patch.

Yours,
Laurenz Albe

On Sat, 27 Jan 2024 at 02:53, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Yes, you are right.  I noticed that everywhere else on the page the
> form "foreign key" is used, so that's what I did in the attached patch.

I looked at the v3 patch with the intention of committing but on
reviewing the comment change for transformFkeyCheckAttrs() and reading
the code, I see that we require a non-partial unique index.  If we're
going to adjust the docs to mention unique indexes then we'd better
also make sure and mention those unique indexes can't be partial ones.

I struggled a bit with the following:

       is used.  The referenced columns must be the columns of a non-deferrable
-      unique or primary key constraint in the referenced table.  The user
+      unique or primary key constraint or a unique index in the
referenced table.  The user
       must have <literal>REFERENCES</literal> permission on the
referenced table

I couldn't really decide if the "or" between "unique" and "primary"
should be a comma or not.  I just found there were too many ways to
interpret the sentence. For example, does the unique index have to be
non-deferrable too? I also found the "in the referenced table" a bit
clumsy after having added the unique index part.

I ended up rewriting it to refer to <replaceable
class="parameter">refcolumn</replaceable>, so the whole thing just
becomes:

      These clauses specify a foreign key constraint, which requires
      that a group of one or more columns of the new table must only
      contain values that match values in the referenced
      column(s) of some row of the referenced table.  If the <replaceable
      class="parameter">refcolumn</replaceable> list is omitted, the
      primary key of the <replaceable class="parameter">reftable</replaceable>
      is used.  Otherwise the <replaceable
class="parameter">refcolumn</replaceable>
      list must refer to the columns of a non-deferrable unique or primary key
      constraint or be the columns of a non-partial unique index.

The part before "Otherwise" stays the same.

For the ddl.sgml changes, aka:

-    A foreign key must reference columns that either are a primary key or
-    form a unique constraint.  This means that the referenced columns always
+    A foreign key should reference columns that either are a primary key or
+    form a unique constraint (<productname>PostgreSQL</productname> only
+    requires a unique index on the columns).  This means that the
referenced columns always
     have an index (the one underlying the primary key or unique constraint);

I think it's now confusing to have "(the one underlying the primary
key or unique constraint);" since we just mentioned we can use a
unique index.

I changed this so that the first two sentences read:

    A foreign key must reference columns that either are a primary key or
    form a unique constraint, or are columns from a non-partial unique index.
    This means that the referenced columns always have an index to allow
    efficient lookups on whether a referencing row has a match.

I was happy with your changes to the header comment for
transformFkeyCheckAttrs(), I just wasn't that happy with the existing
comment in general.  Nothing was mentioned about validation for
duplicate columns and ERRORs. I was also a bit unhappy about the
'opclasses' array. It claimed to be an output parameter but the caller
needs to provide an array of the correct size as input so the array
can be populated by the function.  I know it's only a static function,
but it's a bit annoying to have to read code to figure out how you
should be calling a function.  Again none of this is your doing, I
just think if we're going to adjust it, we should fix all the
shortcomings.

This became:

 * transformFkeyCheckAttrs -
 *
 * Validate that the 'attnums' columns in the 'pkrel' relation are valid to
 * reference as part of a foreign key constraint.
 *
 * Returns the OID of the unique index supporting the constraint and
 * populates the caller-provided 'opclasses' array with the opclasses
 * associated with the index columns.
 *
 * Raises an ERROR on validation failure.

I also deleted the /* output parameter */ comment next to 'opclasses'.
I'd expect an Oid pointer documented as an "output parameter" that I
could just pass in &my_local_oid_var to have the function set it.
That's not what's happening here and I think it's misleading to call
that an output parameter.

I also ended up fiddling with the "foreign-key" vs "foreign key"
thing. I didn't expect you to change the existing ones and on changing
the new one to "foreign-key", I thought it looked weird.  Maybe we
need a master-only patch to make these consistent...

I've attached the v4 patch.

David

Thanks for your updated patch!

On Mon, 2024-01-29 at 21:42 +1300, David Rowley wrote:

> I also ended up fiddling with the "foreign-key" vs "foreign key"
> thing. I didn't expect you to change the existing ones and on changing
> the new one to "foreign-key", I thought it looked weird.  Maybe we
> need a master-only patch to make these consistent...

Hm.  With your changes, the title reads "Foreign-Key Constraints", but
the body uses "foreign key constraint".  I think they should be consistent.

I'd be happy with either variant, but since the hyphenated version
isn't used anywhere else on the page, my opinion is that we should do
away with it.

Other than that, your patch looks good to me.

Yours,
Laurenz Albe

On Mon, 29 Jan 2024 at 21:59, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Hm.  With your changes, the title reads "Foreign-Key Constraints", but
> the body uses "foreign key constraint".  I think they should be consistent.
>
> I'd be happy with either variant, but since the hyphenated version
> isn't used anywhere else on the page, my opinion is that we should do
> away with it.

OK, I've gone back to doing away with it.

> Other than that, your patch looks good to me.

Thanks. Pushed.

David

On Tue, 2024-01-30 at 10:18 +1300, David Rowley wrote:
> Thanks. Pushed.

Thanks for your effort.

Yours,
Laurenz Albe