Reject duplicate column names in foreign key referenced-columns lists.

Such cases are disallowed by the SQL spec, and even if we wanted to allow
them, the semantics seem ambiguous: how should the FK columns be matched up
with the columns of a unique index?  (The matching could be significant in
the presence of opclasses with different notions of equality, so this issue
isn't just academic.)  However, our code did not previously reject such
cases, but instead would either fail to match to any unique index, or
generate a bizarre opclass-lookup error because of sloppy thinking in the
index-matching code.

David Rowley

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6392aee3f6a09ad0721739d3068a1963b9b92e11..3f1ea0911ffc88f21c62095a30dc00a710f4992f 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6496,6 +6496,26 @@ transformFkeyCheckAttrs(Relation pkrel,
    bool        found_deferrable = false;
    List       *indexoidlist;
    ListCell   *indexoidscan;
+   int         i,
+               j;
+
+   /*
+    * Reject duplicate appearances of columns in the referenced-columns list.
+    * Such a case is forbidden by the SQL standard, and even if we thought it
+    * useful to allow it, there would be ambiguity about how to match the
+    * list to unique indexes (in particular, it'd be unclear which index
+    * opclass goes with which FK column).
+    */
+   for (i = 0; i < numattrs; i++)
+   {
+       for (j = i + 1; j < numattrs; j++)
+       {
+           if (attnums[i] == attnums[j])
+               ereport(ERROR,
+                       (errcode(ERRCODE_INVALID_FOREIGN_KEY),
+                        errmsg("foreign key referenced-columns list must not contain duplicates")));
+       }
+   }
 
    /*
     * Get the list of index OIDs for the table from the relcache, and look up
@@ -6508,8 +6528,6 @@ transformFkeyCheckAttrs(Relation pkrel,
    {
        HeapTuple   indexTuple;
        Form_pg_index indexStruct;
-       int         i,
-                   j;
 
        indexoid = lfirst_oid(indexoidscan);
        indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexoid));
@@ -6528,11 +6546,11 @@ transformFkeyCheckAttrs(Relation pkrel,
            heap_attisnull(indexTuple, Anum_pg_index_indpred) &&
            heap_attisnull(indexTuple, Anum_pg_index_indexprs))
        {
-           /* Must get indclass the hard way */
            Datum       indclassDatum;
            bool        isnull;
            oidvector  *indclass;
 
+           /* Must get indclass the hard way */
            indclassDatum = SysCacheGetAttr(INDEXRELID, indexTuple,
                                            Anum_pg_index_indclass, &isnull);
            Assert(!isnull);
@@ -6540,7 +6558,13 @@ transformFkeyCheckAttrs(Relation pkrel,
 
            /*
             * The given attnum list may match the index columns in any order.
-            * Check that each list is a subset of the other.
+            * Check for a match, and extract the appropriate opclasses while
+            * we're at it.
+            *
+            * We know that attnums[] is duplicate-free per the test at the
+            * start of this function, and we checked above that the number of
+            * index columns agrees, so if we find a match for each attnums[]
+            * entry then we must have a one-to-one match in some order.
             */
            for (i = 0; i < numattrs; i++)
            {
@@ -6549,6 +6573,7 @@ transformFkeyCheckAttrs(Relation pkrel,
                {
                    if (attnums[i] == indexStruct->indkey.values[j])
                    {
+                       opclasses[i] = indclass->values[j];
                        found = true;
                        break;
                    }
@@ -6556,24 +6581,6 @@ transformFkeyCheckAttrs(Relation pkrel,
                if (!found)
                    break;
            }
-           if (found)
-           {
-               for (i = 0; i < numattrs; i++)
-               {
-                   found = false;
-                   for (j = 0; j < numattrs; j++)
-                   {
-                       if (attnums[j] == indexStruct->indkey.values[i])
-                       {
-                           opclasses[j] = indclass->values[i];
-                           found = true;
-                           break;
-                       }
-                   }
-                   if (!found)
-                       break;
-               }
-           }
 
            /*
             * Refuse to use a deferrable unique/primary key.  This is per SQL