Horton

Thread

  1. Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Vik Reykja <vikreykja@gmail.com> — 2012-01-24T23:27:12Z

    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.

  2. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T15:16:18Z

    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

  3. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-26T16:29:38Z

    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

  4. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T16:58:06Z

    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

  5. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Vik Reykja <vikreykja@gmail.com> — 2012-01-26T17:02:09Z

    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).

  6. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T17:47:55Z

    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

  7. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Vik Reykja <vikreykja@gmail.com> — 2012-01-26T18:13:17Z

    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.

  8. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T18:59:16Z

    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

  9. Re: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

    Giuseppe Sucameli <brush.tyler@gmail.com> — 2012-01-26T19:14:22Z

    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