Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
I took my first stab at hacking the sources to fix the bug reported here:
http://archives.postgresql.org/pgsql-bugs/2012-01/msg00152.php
It's such a simple patch but it took me several hours with Google and IRC
and I'm sure I did many things wrong. It seems to work as advertised,
though, so I'm submitting it.
I suppose since I have now submitted a patch, it is my moral obligation to
review at least one. I'm not sure how helpful I'll be, but I'll go bite
the bullet and sign myself up anyway.
Attachments:
systemcolumn.patchtext/x-patch; charset=US-ASCII; name=systemcolumn.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index cb8ac67..7555d19
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecAddColumn(List **wqueue, AlteredTa
*** 4235,4240 ****
--- 4235,4241 ----
List *children;
ListCell *child;
AclResult aclresult;
+ HeapTuple attTuple;
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
*************** ATExecAddColumn(List **wqueue, AlteredTa
*** 4314,4326 ****
* this test is deliberately not attisdropped-aware, since if one tries to
* add a column matching a dropped column name, it's gonna fail anyway.
*/
! if (SearchSysCacheExists2(ATTNAME,
! ObjectIdGetDatum(myrelid),
! PointerGetDatum(colDef->colname)))
! ereport(ERROR,
! (errcode(ERRCODE_DUPLICATE_COLUMN),
! errmsg("column \"%s\" of relation \"%s\" already exists",
! colDef->colname, RelationGetRelationName(rel))));
/* Determine the new attribute's number */
if (isOid)
--- 4315,4341 ----
* this test is deliberately not attisdropped-aware, since if one tries to
* add a column matching a dropped column name, it's gonna fail anyway.
*/
! attTuple = SearchSysCache2(ATTNAME,
! ObjectIdGetDatum(myrelid),
! PointerGetDatum(colDef->colname));
!
! if (HeapTupleIsValid(attTuple))
! {
! int attnum;
! attnum = ((Form_pg_attribute) GETSTRUCT(attTuple))->attnum;
! if (attnum <= 0)
! ereport(ERROR,
! (errcode(ERRCODE_DUPLICATE_COLUMN),
! errmsg("column \"%s\" conflicts with a system column name",
! colDef->colname)));
! else
! ereport(ERROR,
! (errcode(ERRCODE_DUPLICATE_COLUMN),
! errmsg("column \"%s\" of relation \"%s\" already exists",
! colDef->colname, RelationGetRelationName(rel))));
!
! ReleaseSysCache(attTuple);
! }
/* Determine the new attribute's number */
if (isOid)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
new file mode 100644
index e992549..8385afb
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** COMMENT ON TABLE tmp_wrong IS 'table com
*** 7,12 ****
--- 7,14 ----
ERROR: relation "tmp_wrong" does not exist
COMMENT ON TABLE tmp IS 'table comment';
COMMENT ON TABLE tmp IS NULL;
+ ALTER TABLE tmp ADD COLUMN xmin integer; -- fails
+ ERROR: column "xmin" conflicts with a system column name
ALTER TABLE tmp ADD COLUMN a int4 default 3;
ALTER TABLE tmp ADD COLUMN b name;
ALTER TABLE tmp ADD COLUMN c text;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
new file mode 100644
index d9bf08d..d4e4c49
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
*************** COMMENT ON TABLE tmp_wrong IS 'table com
*** 9,14 ****
--- 9,16 ----
COMMENT ON TABLE tmp IS 'table comment';
COMMENT ON TABLE tmp IS NULL;
+ ALTER TABLE tmp ADD COLUMN xmin integer; -- fails
+
ALTER TABLE tmp ADD COLUMN a int4 default 3;
ALTER TABLE tmp ADD COLUMN b name;
On Tue, Jan 24, 2012 at 6:27 PM, Vik Reykja <vikreykja@gmail.com> wrote:
I took my first stab at hacking the sources to fix the bug reported here:
http://archives.postgresql.org/pgsql-bugs/2012-01/msg00152.php
It's such a simple patch but it took me several hours with Google and IRC
and I'm sure I did many things wrong. It seems to work as advertised,
though, so I'm submitting it.I suppose since I have now submitted a patch, it is my moral obligation to
review at least one. I'm not sure how helpful I'll be, but I'll go bite the
bullet and sign myself up anyway.
I'm not sure that an error message that is accurate but slightly less
clear than you'd like qualifies as a bug, but I agree that it would
probably be worth improving, and I also agree with the general
approach you've taken here. However, I think we ought to handle
renaming a column symmetrically to adding one. So here's a revised
version of your patch that does that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
systemcolumn-v2.patchapplication/octet-stream; name=systemcolumn-v2.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9172d99..7e6aa94 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -304,6 +304,7 @@ static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recu
static void ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
ColumnDef *colDef, bool isOid,
bool recurse, bool recursing, LOCKMODE lockmode);
+static void check_for_column_name_collision(Relation rel, const char *colname);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
@@ -2257,15 +2258,7 @@ renameatt_internal(Oid myrelid,
oldattname)));
/* new name should not already exist */
-
- /* this test is deliberately not attisdropped-aware */
- if (SearchSysCacheExists2(ATTNAME,
- ObjectIdGetDatum(myrelid),
- PointerGetDatum(newattname)))
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" already exists",
- newattname, RelationGetRelationName(targetrelation))));
+ check_for_column_name_collision(targetrelation, newattname);
/* apply the update */
namestrcpy(&(attform->attname), newattname);
@@ -4310,17 +4303,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
elog(ERROR, "cache lookup failed for relation %u", myrelid);
relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
- /*
- * this test is deliberately not attisdropped-aware, since if one tries to
- * add a column matching a dropped column name, it's gonna fail anyway.
- */
- if (SearchSysCacheExists2(ATTNAME,
- ObjectIdGetDatum(myrelid),
- PointerGetDatum(colDef->colname)))
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" already exists",
- colDef->colname, RelationGetRelationName(rel))));
+ /* new name should not already exist */
+ check_for_column_name_collision(rel, colDef->colname);
/* Determine the new attribute's number */
if (isOid)
@@ -4563,6 +4547,46 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
}
/*
+ * If a new or renamed column will collide with the name of an existing
+ * column, error out.
+ */
+static void
+check_for_column_name_collision(Relation rel, const char *colname)
+{
+ HeapTuple attTuple;
+ int attnum;
+
+ /*
+ * this test is deliberately not attisdropped-aware, since if one tries to
+ * add a column matching a dropped column name, it's gonna fail anyway.
+ */
+ attTuple = SearchSysCache2(ATTNAME,
+ ObjectIdGetDatum(RelationGetRelid(rel)),
+ PointerGetDatum(colname));
+ if (!HeapTupleIsValid(attTuple))
+ return;
+
+ attnum = ((Form_pg_attribute) GETSTRUCT(attTuple))->attnum;
+ ReleaseSysCache(attTuple);
+
+ /*
+ * We throw a different error message for conflicts with system column
+ * names, since they are normally not shown and the user might otherwise
+ * be confused about the reason for the conflict.
+ */
+ if (attnum <= 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_COLUMN),
+ errmsg("column \"%s\" conflicts with a system column name",
+ colname)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" already exists",
+ colname, RelationGetRelationName(rel))));
+}
+
+/*
* Install a column's dependency on its datatype.
*/
static void
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e992549..8385afb 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -7,6 +7,8 @@ COMMENT ON TABLE tmp_wrong IS 'table comment';
ERROR: relation "tmp_wrong" does not exist
COMMENT ON TABLE tmp IS 'table comment';
COMMENT ON TABLE tmp IS NULL;
+ALTER TABLE tmp ADD COLUMN xmin integer; -- fails
+ERROR: column "xmin" conflicts with a system column name
ALTER TABLE tmp ADD COLUMN a int4 default 3;
ALTER TABLE tmp ADD COLUMN b name;
ALTER TABLE tmp ADD COLUMN c text;
diff --git a/src/test/regress/expected/errors.out b/src/test/regress/expected/errors.out
index 4a10c6a..0e2ceb4 100644
--- a/src/test/regress/expected/errors.out
+++ b/src/test/regress/expected/errors.out
@@ -109,7 +109,7 @@ alter table emp rename column salary to manager;
ERROR: column "manager" of relation "stud_emp" already exists
-- conflict
alter table emp rename column salary to oid;
-ERROR: column "oid" of relation "stud_emp" already exists
+ERROR: column "oid" conflicts with a system column name
--
-- TRANSACTION STUFF
-- not in a xact
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index d9bf08d..d4e4c49 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -9,6 +9,8 @@ COMMENT ON TABLE tmp_wrong IS 'table comment';
COMMENT ON TABLE tmp IS 'table comment';
COMMENT ON TABLE tmp IS NULL;
+ALTER TABLE tmp ADD COLUMN xmin integer; -- fails
+
ALTER TABLE tmp ADD COLUMN a int4 default 3;
ALTER TABLE tmp ADD COLUMN b name;
Robert Haas <robertmhaas@gmail.com> writes:
However, I think we ought to handle
renaming a column symmetrically to adding one.
Yeah, I was thinking the same.
So here's a revised version of your patch that does that.
This looks reasonable to me, except that possibly the new error message
text could do with a bit more thought. It seems randomly unlike the
normal message, and I also have a bit of logical difficulty with the
wording equating a "column" with a "column name". The wording that
is in use in the existing CREATE TABLE case is
column name \"%s\" conflicts with a system column name
We could do worse than to use that verbatim, so as to avoid introducing
a new translatable string. Another possibility is
column \"%s\" of relation \"%s\" already exists as a system column
Or we could keep the primary errmsg the same as it is for a normal
column and instead add a DETAIL explaining that this is a system column.
regards, tom lane
On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
However, I think we ought to handle
renaming a column symmetrically to adding one.Yeah, I was thinking the same.
So here's a revised version of your patch that does that.
This looks reasonable to me, except that possibly the new error message
text could do with a bit more thought. It seems randomly unlike the
normal message, and I also have a bit of logical difficulty with the
wording equating a "column" with a "column name". The wording that
is in use in the existing CREATE TABLE case iscolumn name \"%s\" conflicts with a system column name
We could do worse than to use that verbatim, so as to avoid introducing
a new translatable string. Another possibility iscolumn \"%s\" of relation \"%s\" already exists as a system column
Or we could keep the primary errmsg the same as it is for a normal
column and instead add a DETAIL explaining that this is a system column.
I intended for the message to match the CREATE TABLE case. I think it
does, except I see now that Vik's patch left out the word "name" where
the original string has it. So I'll vote in favor of your first
option, above, since that's what I intended anyway.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jan 26, 2012 at 17:58, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This looks reasonable to me, except that possibly the new error message
text could do with a bit more thought. It seems randomly unlike the
normal message, and I also have a bit of logical difficulty with the
wording equating a "column" with a "column name". The wording that
is in use in the existing CREATE TABLE case iscolumn name \"%s\" conflicts with a system column name
We could do worse than to use that verbatim, so as to avoid introducing
a new translatable string. Another possibility iscolumn \"%s\" of relation \"%s\" already exists as a system column
Or we could keep the primary errmsg the same as it is for a normal
column and instead add a DETAIL explaining that this is a system column.I intended for the message to match the CREATE TABLE case. I think it
does, except I see now that Vik's patch left out the word "name" where
the original string has it. So I'll vote in favor of your first
option, above, since that's what I intended anyway.
My intention was to replicate the CREATE TABLE message; I'm not sure how I
failed on that particular point. Thank you guys for noticing and fixing it
(along with all the other corrections).
On Thu, Jan 26, 2012 at 12:02 PM, Vik Reykja <vikreykja@gmail.com> wrote:
On Thu, Jan 26, 2012 at 17:58, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This looks reasonable to me, except that possibly the new error message
text could do with a bit more thought. It seems randomly unlike the
normal message, and I also have a bit of logical difficulty with the
wording equating a "column" with a "column name". The wording that
is in use in the existing CREATE TABLE case iscolumn name \"%s\" conflicts with a system column name
We could do worse than to use that verbatim, so as to avoid introducing
a new translatable string. Another possibility iscolumn \"%s\" of relation \"%s\" already exists as a system column
Or we could keep the primary errmsg the same as it is for a normal
column and instead add a DETAIL explaining that this is a system column.I intended for the message to match the CREATE TABLE case. I think it
does, except I see now that Vik's patch left out the word "name" where
the original string has it. So I'll vote in favor of your first
option, above, since that's what I intended anyway.My intention was to replicate the CREATE TABLE message; I'm not sure how I
failed on that particular point. Thank you guys for noticing and fixing it
(along with all the other corrections).
OK, committed with that further change.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jan 26, 2012 at 18:47, Robert Haas <robertmhaas@gmail.com> wrote:
OK, committed with that further change.
Thank you, Robert! My first real contribution, even if tiny :-)
Just a small nit to pick, though: Giuseppe Sucameli contributed to this
patch but was not credited in the commit log.
On Thu, Jan 26, 2012 at 1:13 PM, Vik Reykja <vikreykja@gmail.com> wrote:
On Thu, Jan 26, 2012 at 18:47, Robert Haas <robertmhaas@gmail.com> wrote:
OK, committed with that further change.
Thank you, Robert! My first real contribution, even if tiny :-)
Just a small nit to pick, though: Giuseppe Sucameli contributed to this
patch but was not credited in the commit log.
Hmm, I should have credited him as the reporter: sorry about that. I
didn't use any of his code, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi Robert,
On Thu, Jan 26, 2012 at 7:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jan 26, 2012 at 1:13 PM, Vik Reykja <vikreykja@gmail.com> wrote:
On Thu, Jan 26, 2012 at 18:47, Robert Haas <robertmhaas@gmail.com> wrote:
OK, committed with that further change.
thank you for the fast fix and Vik for the base patch ;)
Just a small nit to pick, though: Giuseppe Sucameli contributed to this
patch but was not credited in the commit log.I didn't use any of his code, though.
I agree, Robert didn't use any part of my patch, so there's no
reason to credit me in the log.
Regards.
--
Giuseppe Sucameli