BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
The following bug has been logged on the website:
Bug reference: 17877
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 22.04
Description:
The following query:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
produces a valgrind-detected error:
==00:00:00:04.173 3911860== Invalid read of size 1
==00:00:00:04.173 3911860== at 0x3B032C: ATAddForeignKeyConstraint
(tablecmds.c:9242)
==00:00:00:04.173 3911860== by 0x3B2132: ATExecAddConstraint
(tablecmds.c:8875)
==00:00:00:04.173 3911860== by 0x3B8D73: ATExecCmd (tablecmds.c:5031)
==00:00:00:04.173 3911860== by 0x3B93F5: ATRewriteCatalogs
(tablecmds.c:4874)
==00:00:00:04.173 3911860== by 0x3B9613: ATController
(tablecmds.c:4451)
==00:00:00:04.173 3911860== by 0x3B969D: AlterTable (tablecmds.c:4097)
==00:00:00:04.173 3911860== by 0x596AD6: ProcessUtilitySlow
(utility.c:1325)
==00:00:00:04.173 3911860== by 0x596584: standard_ProcessUtility
(utility.c:1074)
==00:00:00:04.173 3911860== by 0x59666D: ProcessUtility (utility.c:530)
==00:00:00:04.173 3911860== by 0x59696B: ProcessUtilitySlow
(utility.c:1252)
==00:00:00:04.173 3911860== by 0x596584: standard_ProcessUtility
(utility.c:1074)
==00:00:00:04.173 3911860== by 0x59666D: ProcessUtility (utility.c:530)
==00:00:00:04.173 3911860== Address 0x4bf512dc is 113,516 bytes inside a
recently re-allocated block of size 524,288 alloc'd
...
2023-03-29 16:00:29.068 MSK|||64243669.3bb06e|LOG: server process (PID
3911860) exited with exit code 1
2023-03-29 16:00:29.068 MSK|||64243669.3bb06e|DETAIL: Failed process was
running: CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES
pt);
Here in ATAddForeignKeyConstraint():
/*
* Check some things for generated columns.
*/
for (i = 0; i < numfks; i++)
{
char attgenerated = TupleDescAttr(RelationGetDescr(rel), fkattnum[i] -
1)->attgenerated;
if (attgenerated)
...
we have fkattnum[i] = -6 for the attribute "tableoid", but TupleDescAttr()
can't handle system attributes.
I've looked at fc22b6623 and found no other TupleDescAttr() calls where
attnum can be negative.
Though maybe it is a nonsense to use tableoid in a such way.
But if not, someone can try to move on without valgrind:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
INSERT INTO pt SELECT oid, 1 FROM pg_class WHERE relname = 'ft';
INSERT INTO ft VALUES(1);
and get:
...
#5 0x0000557485037238 in ExceptionalCondition (conditionName=0x557485245f18
"attnum > 0",
errorType=0x557485245f0c "BadArgument",
fileName=0x557485245ee0 "../../../../src/include/executor/tuptable.h",
lineNumber=369) at assert.c:69
#6 0x0000557484f93846 in slot_attisnull (slot=0x557485e64420, attnum=-6)
at ../../../../src/include/executor/tuptable.h:369
#7 0x0000557484f9913c in ri_NullCheck (tupDesc=0x7fa297361aa8,
slot=0x557485e64420,
riinfo=0x557485e7ab80, rel_is_pk=false) at ri_triggers.c:2648
#8 0x0000557484f93bad in RI_FKey_check (trigdata=0x7fffc6ac8980) at
ri_triggers.c:280
#9 0x0000557484f940db in RI_FKey_check_ins (fcinfo=0x7fffc6ac87c0) at
ri_triggers.c:434
#10 0x0000557484bb9baf in ExecCallTriggerFunc (trigdata=0x7fffc6ac8980,
tgindx=0, finfo=0x557485e63878,
instr=0x0, per_tuple_context=0x557485e70f60) at trigger.c:2435
#11 0x0000557484bbd653 in AfterTriggerExecute (estate=0x557485e62ff0,
event=0x557485e6f090,
relInfo=0x557485e63480, src_relInfo=0x557485e63480,
dst_relInfo=0x557485e63480,
trigdesc=0x557485e63698, finfo=0x557485e63878, instr=0x0,
per_tuple_context=0x557485e70f60,
trig_tuple_slot1=0x0, trig_tuple_slot2=0x0) at trigger.c:4546
#12 0x0000557484bbdc68 in afterTriggerInvokeEvents (events=0x557485d8bdb0,
firing_id=1,
estate=0x557485e62ff0, delete_ok=false) at trigger.c:4785
#13 0x0000557484bbe5b5 in AfterTriggerEndQuery (estate=0x557485e62ff0) at
trigger.c:5140
#14 0x0000557484bf696b in standard_ExecutorFinish (queryDesc=0x557485e53c60)
at execMain.c:438
#15 0x0000557484bf6821 in ExecutorFinish (queryDesc=0x557485e53c60) at
execMain.c:406
#16 0x0000557484e6f649 in ProcessQuery (plan=0x557485e48798,
sourceText=0x557485d68870 "INSERT INTO ft VALUES(1);", params=0x0,
queryEnv=0x0, dest=0x557485e48888,
qc=0x7fffc6ac8e10) at pquery.c:193
PG Bug reporting form <noreply@postgresql.org> writes:
The following query:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
produces a valgrind-detected error:
We should probably just disallow system columns as foreign keys.
There was a legitimate use-case for that with OID columns, but
no more. I can't see a really good reason to use tableoid as a
foreign key, and none of the other system columns are stable
enough for this to be sane at all. So it's hard to summon
interest in trying to remove bugs of this sort.
regards, tom lane
On 2023-Mar-29, Tom Lane wrote:
PG Bug reporting form <noreply@postgresql.org> writes:
The following query:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
produces a valgrind-detected error: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
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
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