Обсуждение: 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.
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.
Вложения
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
Вложения
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 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
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 />
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
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 />
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"
От
Giuseppe Sucameli
Дата:
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