Re: 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.

On Tue, Jan 24, 2012 at 6:27 PM, Vik Reykja <vikreykja(at)gmail(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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 <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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).

On Thu, Jan 26, 2012 at 12:02 PM, Vik Reykja <vikreykja(at)gmail(dot)com> wrote:
> On Thu, Jan 26, 2012 at 17:58, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Thu, Jan 26, 2012 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, Jan 26, 2012 at 18:47, Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, Jan 26, 2012 at 1:13 PM, Vik Reykja <vikreykja(at)gmail(dot)com> wrote:
>> On Thu, Jan 26, 2012 at 18:47, Robert Haas <robertmhaas(at)gmail(dot)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