From: | Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Payal Singh <payal(at)omniti(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Add CINE for ALTER TABLE ... ADD COLUMN |
Date: | 2015-07-23 00:55:21 |
Message-ID: | CAFcNs+rj1B+G6gOgYxAv9rK2_Lw_q2aYqkp06VKp7eqHSMSyjg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jul 16, 2015 at 10:36 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)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 | Content-Type | Size |
---|---|---|
alter-table-add-column-if-not-exists_v5.patch | text/x-diff | 13.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro HORIGUCHI | 2015-07-23 01:25:24 | Re: Asynchronous execution on FDW |
Previous Message | Kouhei Kaigai | 2015-07-23 00:24:39 | Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it? |