On Thu, Oct 30, 2014 at 12:11 PM, Fujii Masao <
masao.fujii@gmail.com> wrote:
>
> On Tue, Oct 7, 2014 at 2:42 AM, Fabrízio de Royes Mello
> <
fabriziomello@gmail.com> wrote:
> > On Mon, Oct 6, 2014 at 11:13 AM, Marti Raudsepp <
marti@juffo.org> wrote:
> >>
> >> On Mon, Oct 6, 2014 at 4:27 PM, Fabrízio de Royes Mello
> >> <
fabriziomello@gmail.com> wrote:
> >> > create_index_if_not_exists_v7.patch
> >>
> >> Looks good to me. Marking ready for committer.
> >>
> >
> > Thanks.
>
> The patch looks good to me except the following minor comments.
>
> + <term><literal>IF NOT EXISTS</literal></term>
>
> It's better to place this after the paragraph of CONCURRENTLY
> for the consistency with the syntax.
>
Fixed.
> + Do not throw an error if the index already exists.
>
> I think that this should be
>
> Do not throw an error if a relation with the same name already exists.
>
Fixed.
> + Index name is required when IF NOT EXISTS is specified.
>
> IF NOT EXISTS should be enclosed with <literal> tag.
>
Fixed.
> @@ -60,7 +60,8 @@ extern Oid index_create(Relation heapRelation,
> bool allow_system_table_mods,
> bool skip_build,
> bool concurrent,
> - bool is_internal);
> + bool is_internal,
> + bool if_not_exists);
>
> You forgot to add the comment of if_not_exists argument into the top of
> index_create function.
>
Fixed.
> + bool if_not_exists; /* just do nothing if index already exists */
>
> You forgot to add the trailing "?" at the above comment. There are similar
> comments in parsenodes.h, and they have such "?".
>
Fixed.
Thanks for your review!
Regards,