Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Tue, Jan 24, 2012 at 6:27 PM, Vik Reykja vikrey...@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 systemcolumn-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Thu, Jan 26, 2012 at 17:58, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane t...@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 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. 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).
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Thu, Jan 26, 2012 at 12:02 PM, Vik Reykja vikrey...@gmail.com wrote: On Thu, Jan 26, 2012 at 17:58, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane t...@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 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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Thu, Jan 26, 2012 at 18:47, Robert Haas robertmh...@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.
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
On Thu, Jan 26, 2012 at 1:13 PM, Vik Reykja vikrey...@gmail.com wrote: On Thu, Jan 26, 2012 at 18:47, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
Hi Robert, On Thu, Jan 26, 2012 at 7:59 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 26, 2012 at 1:13 PM, Vik Reykja vikrey...@gmail.com wrote: On Thu, Jan 26, 2012 at 18:47, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin
Hi hackers, the attached patch fixes the problem I explained in pgsql-bugs (forwarded). It's a trivial problem, so no initial patch design was required. Hope to see my patch applied. Thanks for your work. Regards. -- Forwarded message -- From: Giuseppe Sucameli brush.ty...@gmail.com Date: Sun, Jan 22, 2012 at 2:22 PM Subject: Different error messages executing CREATE TABLE or ALTER TABLE to create a column xmin To: pgsql-b...@postgresql.org Hi all, trying to create a table with a column xmin I get the following error message: test= create table lx (xmin int); ERROR: column name xmin conflicts with a system column name Instead I get a different (and less understandable) error message if I try to add a column named xmin to an existent table: test= create table lx (i int); CREATE TABLE test= alter table lx add xmin int; ERROR: column xmin of relation lx already exists. The same problem occurs using xmax as column name. I'm on Ubuntu 11.04. Tried on both PostgreSQL 8.4.10 and 9.1.2 Regards. -- Giuseppe Sucameli -- Giuseppe Sucameli postgresql_syscol_message.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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.
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;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
