Postgres Pro
Downloads
Home   >   mailing lists

Thread: Add CINE for ALTER TABLE ... ADD COLUMN

Add CINE for ALTER TABLE ... ADD COLUMN

From
Fabrízio de Royes Mello
Date:
Hi all,

This simple patch add CINE for ALTER TABLE ... ADD COLUMN.

So now we can:

ALTER TABLE foo
    ADD COLUMN IF NOT EXISTS c1 integer;

and/or ...

ALTER TABLE foo
    ADD COLUMN IF NOT EXISTS c1 integer,
    ADD COLUMN c2 integer;

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Attachment

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Payal Singh
Date:
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Seeing this when trying to apply the patch:

Patching file src/backend/commands/tablecmds.c using Plan A...
Hunk #1 FAILED at 328.
Hunk #2 succeeded at 2294 (offset 11 lines).
Hunk #3 FAILED at 3399.
Hunk #4 FAILED at 3500.
Hunk #5 succeeded at 4658 with fuzz 1 (offset 65 lines).
Hunk #6 succeeded at 4753 (offset 66 lines).
Hunk #7 succeeded at 4989 with fuzz 2 (offset 66 lines).
Hunk #8 succeeded at 5003 (offset 69 lines).
Hunk #9 succeeded at 5017 (offset 69 lines).
Hunk #10 succeeded at 5033 (offset 69 lines).

The new status of this patch is: Waiting on Author

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Fabrízio de Royes Mello
Date:

On Wed, Apr 22, 2015 at 3:48 PM, Payal Singh <payal@omniti.com> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
>
> Seeing this when trying to apply the patch:
>
> Patching file src/backend/commands/tablecmds.c using Plan A...
> Hunk #1 FAILED at 328.
> Hunk #2 succeeded at 2294 (offset 11 lines).
> Hunk #3 FAILED at 3399.
> Hunk #4 FAILED at 3500.
> Hunk #5 succeeded at 4658 with fuzz 1 (offset 65 lines).
> Hunk #6 succeeded at 4753 (offset 66 lines).
> Hunk #7 succeeded at 4989 with fuzz 2 (offset 66 lines).
> Hunk #8 succeeded at 5003 (offset 69 lines).
> Hunk #9 succeeded at 5017 (offset 69 lines).
> Hunk #10 succeeded at 5033 (offset 69 lines).
>
> The new status of this patch is: Waiting on Author
>

The patch needs a "rebase". Done!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Fabrízio de Royes Mello
Date:
On Thu, Apr 23, 2015 at 12:05 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Wed, Apr 22, 2015 at 3:48 PM, Payal Singh <payal@omniti.com> wrote:
> >
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, failed
> > Implements feature:       not tested
> > Spec compliant:           not tested
> > Documentation:            not tested
> >
> > Seeing this when trying to apply the patch:
> >
> > Patching file src/backend/commands/tablecmds.c using Plan A...
> > Hunk #1 FAILED at 328.
> > Hunk #2 succeeded at 2294 (offset 11 lines).
> > Hunk #3 FAILED at 3399.
> > Hunk #4 FAILED at 3500.
> > Hunk #5 succeeded at 4658 with fuzz 1 (offset 65 lines).
> > Hunk #6 succeeded at 4753 (offset 66 lines).
> > Hunk #7 succeeded at 4989 with fuzz 2 (offset 66 lines).
> > Hunk #8 succeeded at 5003 (offset 69 lines).
> > Hunk #9 succeeded at 5017 (offset 69 lines).
> > Hunk #10 succeeded at 5033 (offset 69 lines).
> >
> > The new status of this patch is: Waiting on Author
> >
>
> The patch needs a "rebase". Done!
>

Another rebased version.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Alvaro Herrera
Date:
Fabrízio de Royes Mello wrote:

> Another rebased version.

There are a number of unrelated whitespace changes in this patch; also
please update the comment on top of check_for_column_name_collision.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Fabrízio de Royes Mello
Date:


On Wed, Jun 24, 2015 at 3:36 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabrízio de Royes Mello wrote:
>
> > Another rebased version.
>
> There are a number of unrelated whitespace changes in this patch; also
> please update the comment on top of check_for_column_name_collision.
>

Sorry, bad merging after a pgident run. Comments on top of check_for_column_name collision also updated.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Michael Paquier
Date:
On Fri, Jun 26, 2015 at 12:41 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
>
> On Wed, Jun 24, 2015 at 3:36 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>>
>> Fabrízio de Royes Mello wrote:
>>
>> > Another rebased version.
>>
>> There are a number of unrelated whitespace changes in this patch; also
>> please update the comment on top of check_for_column_name_collision.
>>
>
> Sorry, bad merging after a pgident run. Comments on top of
> check_for_column_name collision also updated.

I had a look at this patch, and here are some minor comments:
1) In alter_table.sgml, you need a space here:
[ IF NOT EXISTS ]<replaceable
2)
+       check_for_column_name_collision(targetrelation, newattname, false);
(void) needs to be added in front of check_for_column_name_collision
where its return value is not checked or static code analyzers are
surely going to complain.
3) Something minor, some lines of codes exceed 80 characters, see
declaration of check_for_column_name_collision for example...
4) This comment needs more precisions?       /* new name should not already exist */
-       check_for_column_name_collision(rel, colDef->colname);
+       if (!check_for_column_name_collision(rel, colDef->colname,
if_not_exists))
The new name can actually exist if if_not_exists is true.

Except that the implementation looks sane to me.
Regards,
--
Michael

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Fabrízio de Royes Mello
Date:

On Thu, Jul 16, 2015 at 10:36 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> I had a look at this patch, and here are some minor comments:
> 1) In alter_table.sgml, you need a space here:
> [ IF NOT EXISTS ]<replaceable

Fixed.


> 2)
> +       check_for_column_name_collision(targetrelation, newattname, false);
> (void) needs to be added in front of check_for_column_name_collision
> where its return value is not checked or static code analyzers are
> surely going to complain.

Fixed.


> 3) Something minor, some lines of codes exceed 80 characters, see
> declaration of check_for_column_name_collision for example...

Fixed.


> 4) This comment needs more precisions?
>         /* new name should not already exist */
> -       check_for_column_name_collision(rel, colDef->colname);
> +       if (!check_for_column_name_collision(rel, colDef->colname,
> if_not_exists))
> The new name can actually exist if if_not_exists is true.
>

Improved the comment.


> Except that the implementation looks sane to me.
>

Thank you for the review.

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment

Re: Add CINE for ALTER TABLE ... ADD COLUMN

From
Michael Paquier
Date:
On Thu, Jul 23, 2015 at 9:55 AM, Fabrízio de Royes Mello wrote:
> Thank you for the review.

+       /* skipp if the name already exists and if_not_exists is true */
s/skipp/skip.

Except that this looks in good shape to me (see attached for a version
fixing the typo) so switched to "Ready for committer".
--
Michael
Attachment