Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
Hi,
I think that commit f0d65c0
<https://github.com/postgres/postgres/commit/f0d65c0eaf05d6acd3ae05cde4a31465eb3992b2>
has an oversight.
Attnum == 0, is system column too, right?
All other places at tablecmds.c, has this test:
if (attnum <= 0)
ereport(ERROR,
regards,
Ranier Vilela
Attachments:
reject_system_column_equal_zero.patchapplication/octet-stream; name=reject_system_column_equal_zero.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7808241d3f..6e545a1d15 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11298,7 +11298,7 @@ transformColumnNameList(Oid relId, List *colList,
errmsg("column \"%s\" referenced in foreign key constraint does not exist",
attname)));
attform = (Form_pg_attribute) GETSTRUCT(atttuple);
- if (attform->attnum < 0)
+ if (attform->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("system columns cannot be used in foreign keys")));
Ranier Vilela <ranier.vf@gmail.com> writes:
I think that commit f0d65c0
<https://github.com/postgres/postgres/commit/f0d65c0eaf05d6acd3ae05cde4a31465eb3992b2>
has an oversight.
Attnum == 0, is system column too, right?
No, it's not valid in pg_attribute rows.
All other places at tablecmds.c, has this test:
if (attnum <= 0)
ereport(ERROR,
I was actually copying this code in indexcmds.c:
if (attno < 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("index creation on system columns is not supported")));
There's really no reason to prefer one over the other in this context.
regards, tom lane
Em sex., 31 de mar. de 2023 às 16:25, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
I think that commit f0d65c0
<https://github.com/postgres/postgres/commit/f0d65c0eaf05d6acd3ae05cde4a31465eb3992b2
has an oversight.
Attnum == 0, is system column too, right?No, it's not valid in pg_attribute rows.
All other places at tablecmds.c, has this test:
if (attnum <= 0)
ereport(ERROR,I was actually copying this code in indexcmds.c:
if (attno < 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("index creation on system columns is not
supported")));There's really no reason to prefer one over the other in this context.
I think the documentation is a bit confusing.
According to the current documentation:
/*
* attnum is the "attribute number" for the attribute: A value that
* uniquely identifies this attribute within its class. for user
* attributes, Attribute numbers are greater than 0 and not greater than
* the number of attributes in the class. i.e. if the Class pg_class says
* that Class XYZ has 10 attributes, then the user attribute numbers in
* Class pg_attribute must be 1-10.
*
* System attributes have attribute numbers less than 0 that are unique
* within the class, but not constrained to any particular range.
*
* Note that (attnum - 1) is often used as the index to an array.
Attributes equal to zero are in limbo.
IMO should be:
* System attributes have attribute numbers less or equal to 0 that are
* unique
* within the class, but not constrained to any particular range.
regards,
Ranier Vilela
At Sat, 1 Apr 2023 10:56:08 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
IMO should be:
* System attributes have attribute numbers less or equal to 0 that are
* unique
* within the class, but not constrained to any particular range.
Attnum == 0 is invalid and doesn't belong to either user columns or
system columns. You're actually right that it's in limbo, but I
believe the change you suggested actually makes the correct comment
incorrect. In the condition you're asking about, I don't think we
really need to worry about an impossible case. If I wanted to pay more
attenstion to it, I would use an assertion, but I don't think it's
necessary here.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center