Re: CREATE IF NOT EXISTS INDEX

On Thu, Oct 2, 2014 at 8:15 PM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>
> On Wed, Oct 1, 2014 at 2:42 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> > So, what's the correct/best grammar?
> > CREATE [ IF NOT EXISTS ] [ UNIQUE ] INDEX index_name
> > or
> > CREATE [ UNIQUE ] INDEX [ IF NOT EXISTS ] index_name
>
> I've elected myself as the reviewer for this patch. Here are some
> preliminary comments...
>

Thanks...

> I agree with José. The 2nd is more consistent given the other syntaxes:
> CREATE { TABLE | SCHEMA | EXTENSION | ... } IF NOT EXISTS name ...
> It's also compatible with SQLite's grammar:
> https://www.sqlite.org/lang_createindex.html
>
> Do we want to enforce an order on the keywords or allow both?
> CREATE INDEX IF NOT EXISTS CONCURRENTLY foo ...
> CREATE INDEX CONCURRENTLY IF NOT EXISTS foo ...
>
> It's probably very rare to use both keywords at the same time, so I'd
> prefer only the 2nd, unless someone else chimes in.
>

Fixed.

> Documentation: I would prefer if the explanation were consistent with
> the description for ALTER TABLE/EXTENSION; just copy it and replace
> "relation" with "index".
>

Well, I'm not native English so I tend to agree with you.

"Do not throw an error if the index already exists. A notice is issued in
this case."

Fixed in that way. Ok?

> + ereport(NOTICE,
> + (errcode(ERRCODE_DUPLICATE_TABLE),
> + errmsg("relation \"%s\" already exists, skipping",
> + indexRelationName)));
>
> 1. Clearly "relation" should be "index".
> 2. Use ERRCODE_DUPLICATE_OBJECT not TABLE
>

Sorry, but I'm not with you. I just copy the original error message and add
", skipping" at the end.

788 ereport(ERROR,
789 (errcode(ERRCODE_DUPLICATE_TABLE),
790 errmsg("relation \"%s\" already exists",
791 indexRelationName)));

> + if (n->if_not_exists && n->idxname == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("IF NOT EXISTS requires that you
> name the index."),
>
> I think ERRCODE_SYNTAX_ERROR makes more sense, it's something that we
> decided we *don't want* to support.
>

I don't think so. It's the same as CREATE SCHEMA IF NOT EXISTS that not
support to include schema elements.

Other opinions?

> - write_msg(NULL, "reading row-security enabled for table \"%s\"",
> + write_msg(NULL, "reading row-security enabled for table \"%s\"\n",
>
> ???
>

Sorry, my mistake. I merged with a wrong local branch. Fixed.

Regards,

--
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