Add CINE for ALTER TABLE ... ADD COLUMN

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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