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

On Fri, Jun 26, 2015 at 12:41 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
>
> On Wed, Jun 24, 2015 at 3:36 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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