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

On Sat, 27 Jan 2024 at 02:53, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)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