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

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