Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access - Mailing list pgsql-bugs

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Mar-29, Tom Lane wrote:
>> We should probably just disallow system columns as foreign keys.
>> There was a legitimate use-case for that with OID columns, but
>> no more.

> +1

Seems pretty easy, as attached.  The possibly harder question is whether
to back-patch this, or to try to fix the problems Alexander identified
in the back branches.

Some investigation determined that system columns in foreign keys
appear to actually work up through v11, which perhaps coincidentally
is the last version that allowed the OID system column.  v12 and up
have the slot_attnum problem that Alexander showed.  The lack of
complaints about that suggests that nobody is trying to use any other
system columns as FKs in production.  Nonetheless, it's a bit worrisome
to remove a feature-that-used-to-work in stable branches.

On balance though, I'd rather block this than promise to make it work
in the back branches.  I propose applying the attached back to v12,
and leaving v11 alone.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3147dddf28..7808241d3f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11271,6 +11271,11 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName,
  * transformColumnNameList - transform list of column names
  *
  * Lookup each name and return its attnum and, optionally, type OID
+ *
+ * Note: the name of this function suggests that it's general-purpose,
+ * but actually it's only used to look up names appearing in foreign-key
+ * clauses.  The error messages would need work to use it in other cases,
+ * and perhaps the validity checks as well.
  */
 static int
 transformColumnNameList(Oid relId, List *colList,
@@ -11284,6 +11289,7 @@ transformColumnNameList(Oid relId, List *colList,
     {
         char       *attname = strVal(lfirst(l));
         HeapTuple    atttuple;
+        Form_pg_attribute attform;

         atttuple = SearchSysCacheAttName(relId, attname);
         if (!HeapTupleIsValid(atttuple))
@@ -11291,14 +11297,19 @@ transformColumnNameList(Oid relId, List *colList,
                     (errcode(ERRCODE_UNDEFINED_COLUMN),
                      errmsg("column \"%s\" referenced in foreign key constraint does not exist",
                             attname)));
+        attform = (Form_pg_attribute) GETSTRUCT(atttuple);
+        if (attform->attnum < 0)
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("system columns cannot be used in foreign keys")));
         if (attnum >= INDEX_MAX_KEYS)
             ereport(ERROR,
                     (errcode(ERRCODE_TOO_MANY_COLUMNS),
                      errmsg("cannot have more than %d keys in a foreign key",
                             INDEX_MAX_KEYS)));
-        attnums[attnum] = ((Form_pg_attribute) GETSTRUCT(atttuple))->attnum;
+        attnums[attnum] = attform->attnum;
         if (atttypids != NULL)
-            atttypids[attnum] = ((Form_pg_attribute) GETSTRUCT(atttuple))->atttypid;
+            atttypids[attnum] = attform->atttypid;
         ReleaseSysCache(atttuple);
         attnum++;
     }
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 2f9c083539..55f7158c1a 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -795,15 +795,16 @@ SELECT * FROM FKTABLE ORDER BY id;

 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;
-CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY);
+-- Test some invalid FK definitions
+CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY, someoid oid);
 CREATE TABLE FKTABLE_FAIL1 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest2) REFERENCES PKTABLE);
 ERROR:  column "ftest2" referenced in foreign key constraint does not exist
 CREATE TABLE FKTABLE_FAIL2 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(ptest2));
 ERROR:  column "ptest2" referenced in foreign key constraint does not exist
-DROP TABLE FKTABLE_FAIL1;
-ERROR:  table "fktable_fail1" does not exist
-DROP TABLE FKTABLE_FAIL2;
-ERROR:  table "fktable_fail2" does not exist
+CREATE TABLE FKTABLE_FAIL3 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (tableoid) REFERENCES PKTABLE(someoid));
+ERROR:  system columns cannot be used in foreign keys
+CREATE TABLE FKTABLE_FAIL4 ( ftest1 oid, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(tableoid));
+ERROR:  system columns cannot be used in foreign keys
 DROP TABLE PKTABLE;
 -- Test for referencing column number smaller than referenced constraint
 CREATE TABLE PKTABLE (ptest1 int, ptest2 int, UNIQUE(ptest1, ptest2));
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 90f6db7f21..22e177f89b 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -490,12 +490,13 @@ SELECT * FROM FKTABLE ORDER BY id;
 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;

-CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY);
+-- Test some invalid FK definitions
+CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY, someoid oid);
 CREATE TABLE FKTABLE_FAIL1 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest2) REFERENCES PKTABLE);
 CREATE TABLE FKTABLE_FAIL2 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(ptest2));
+CREATE TABLE FKTABLE_FAIL3 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (tableoid) REFERENCES PKTABLE(someoid));
+CREATE TABLE FKTABLE_FAIL4 ( ftest1 oid, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(tableoid));

-DROP TABLE FKTABLE_FAIL1;
-DROP TABLE FKTABLE_FAIL2;
 DROP TABLE PKTABLE;

 -- Test for referencing column number smaller than referenced constraint