Thread: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Vik Reykja
Date:
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.
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.
Attachment
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Robert Haas
Date:
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
Attachment
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Tom Lane
Date:
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
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Robert Haas
Date:
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 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
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Vik Reykja
Date:
On Thu, Jan 26, 2012 at 17:58, Robert Haas <span dir="ltr"><<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>></span>wrote:<br /><div class="gmail_quote"><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><divclass="HOEnZb"><div class="h5">On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br /> > This looks reasonable to me, except that possiblythe new error message<br /> > text could do with a bit more thought. It seems randomly unlike the<br /> >normal message, and I also have a bit of logical difficulty with the<br /> > wording equating a "column" with a "columnname". The wording that<br /> > is in use in the existing CREATE TABLE case is<br /> ><br /> > column name\"%s\" conflicts with a system column name<br /> ><br /> > We could do worse than to use that verbatim, so as toavoid introducing<br /> > a new translatable string. Another possibility is<br /> ><br /> > column \"%s\" ofrelation \"%s\" already exists as a system column<br /> ><br /> > Or we could keep the primary errmsg the same asit is for a normal<br /> > column and instead add a DETAIL explaining that this is a system column.<br /><br /></div></div>Iintended for the message to match the CREATE TABLE case. I think it<br /> does, except I see now that Vik'spatch left out the word "name" where<br /> the original string has it. So I'll vote in favor of your first<br /> option,above, since that's what I intended anyway.<br /></blockquote></div><br />My intention was to replicate the CREATETABLE message; I'm not sure how I failed on that particular point. Thank you guys for noticing and fixing it (alongwith all the other corrections).<br /><br />
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Robert Haas
Date:
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 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
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Vik Reykja
Date:
On Thu, Jan 26, 2012 at 18:47, Robert Haas <span dir="ltr"><<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>></span>wrote:<br /><div class="gmail_quote"><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">OK, committed with that further change.</blockquote></div><br />Thank you, Robert! My first realcontribution, even if tiny :-)<br /><br />Just a small nit to pick, though: Giuseppe Sucameli contributed to this patchbut was not credited in the commit log.<br />
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Robert Haas
Date:
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
Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"
From
Giuseppe Sucameli
Date:
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