Re: Patch to add support of "IF NOT EXISTS" to others "CREATE" statements

At 2013-06-12 14:29:59 -0300, fabriziomello@gmail.com wrote:
>
> The attached patch add support to "IF NOT EXISTS" to "CREATE"
> statements listed below: […]

I noticed that this patch was listed as "Needs Review" with no
reviewers, so I had a quick look.

It applies with a couple of "trailing whitespace" warnings. Compiles OK.
Documentation changes look fine other than a couple of minor whitespace
errors (literal tabs in some continuation lines).

First, I looked at the changes in src/include: adding if_not_exists to
the relevant parse nodes, and adding ifNotExists parameters to various
functions (e.g. OperatorCreate). The changes look fine. There's a bit
more whitespace quirkiness, though (removed tabs).

Next, changes in src/backend, starting with parser changes: the patch
adds "IF_P NOT EXISTS" variants for various productions. For example:

src/backend/parser/gram.y:4605:
>
> DefineStmt:
>             CREATE AGGREGATE func_name aggr_args definition
>                 {
>                     DefineStmt *n = makeNode(DefineStmt);
>                     n->kind = OBJECT_AGGREGATE;
>                     n->oldstyle = false;
>                     n->defnames = $3;
>                     n->args = $4;
>                     n->definition = $5;
>                     n->if_not_exists = false;
>                     $$ = (Node *)n;
>                 }
>             | CREATE AGGREGATE IF_P NOT EXISTS func_name aggr_args definition
>                 {
>                     DefineStmt *n = makeNode(DefineStmt);
>                     n->kind = OBJECT_AGGREGATE;
>                     n->oldstyle = false;
>                     n->defnames = $6;
>                     n->args = $7;
>                     n->definition = $8;
>                     n->if_not_exists = true;
>                     $$ = (Node *)n;
>                 }

Although there is plenty of precedent for this kind of doubling of rules
(e.g. CREATE SCHEMA, CREATE EXTENSION), it doesn't strike me as the best
idea. There's an "opt_if_not_exists", and this patch uses it for "CREATE
CAST" (and it was already used for AlterEnumStmt).

I think opt_if_not_exists should be used for the others as well.

Moving on, the patch adds the if_not_exists field to the relevant
functions in {copyfuncs,equalfuncs}.c and adds stmt->if_not_exists
to the calls to DefineAggregate() etc. in tcop/utility.c. Fine.

> diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> index 64ca312..851c314 100644
> --- a/src/backend/catalog/heap.c
> +++ b/src/backend/catalog/heap.c
>  /* --------------------------------
> @@ -1219,7 +1220,8 @@ heap_create_with_catalog(const char *relname,
>                     -1,            /* typmod */
>                     0,            /* array dimensions for typBaseType */
>                     false,        /* Type NOT NULL */
> -                   InvalidOid); /* rowtypes never have a collation */
> +                   InvalidOid,    /* rowtypes never have a collation */
> +                   false);

Parameter needs a comment.

> diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
> index e80b600..4452ba3 100644
> --- a/src/backend/catalog/pg_aggregate.c
> +++ b/src/backend/catalog/pg_aggregate.c
> @@ -228,7 +229,7 @@ AggregateCreate(const char *aggName,
>  
>      procOid = ProcedureCreate(aggName,
>                                aggNamespace,
> -                              false,    /* no replacement */
> +                              false,    /* replacement */
>                                false,    /* doesn't return a set */
>                                finaltype,        /* returnType */
>                                GetUserId(),        /* proowner */

What's up with this? We're calling ProcedureCreate with replace==false,
so "no replacement" sounds correct to me.

> diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
> index 3c4fedb..7466e76 100644
> --- a/src/backend/catalog/pg_operator.c
> +++ b/src/backend/catalog/pg_operator.c
> @@ -336,7 +336,8 @@ OperatorCreate(const char *operatorName,
>                 Oid restrictionId,
>                 Oid joinId,
>                 bool canMerge,
> -               bool canHash)
> +               bool canHash,
> +               bool if_not_exists)
>  {
>      Relation    pg_operator_desc;
>      HeapTuple    tup;

This should be "ifNotExists" (to match the header file, and all your
other changes).

> @@ -416,11 +417,18 @@ OperatorCreate(const char *operatorName,
>                                     rightTypeId,
>                                     &operatorAlreadyDefined);
>  
> -    if (operatorAlreadyDefined)
> +    if (operatorAlreadyDefined && !if_not_exists)
>          ereport(ERROR,
>                  (errcode(ERRCODE_DUPLICATE_FUNCTION),
>                   errmsg("operator %s already exists",
>                          operatorName)));
> +    if (operatorAlreadyDefined && if_not_exists) {
> +        ereport(NOTICE,
> +                (errcode(ERRCODE_DUPLICATE_FUNCTION),
> +                 errmsg("operator %s already exists, skipping",
> +                        operatorName)));
> +        return InvalidOid;
> +    }

Everywhere else, you're doing something like this:
   if (exists)   {       if (!if_not_exists)           ERROR       else           NOTICE   }

So you should do the same thing here. Failing that, at least reorder the
blocks so that you don't test both !if_not_exists and if_not_exists:
   if (operatorAlreadyDefined && if_not_exists)   {       NOTICE;       return InvalidOid;   }
   if (operatorAlreadyDefined)       ERROR

(But first see below.)

> diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
> index 23ac3dd..3d55360 100644
> --- a/src/backend/catalog/pg_type.c
> +++ b/src/backend/catalog/pg_type.c
> @@ -397,9 +398,20 @@ TypeCreate(Oid newTypeOid,
>           * shell type, however.
>           */
>          if (((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_DUPLICATE_OBJECT),
> -                     errmsg("type \"%s\" already exists", typeName)));
> +        {
> +            if (!ifNotExists)
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                         errmsg("type \"%s\" already exists", typeName)));
> +            else
> +            {
> +                ereport(NOTICE,
> +                        (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                         errmsg("type \"%s\" already exists, skipping", typeName)));
> +                heap_close(pg_type_desc, RowExclusiveLock);
> +                return InvalidOid;
> +            }
> +        }

I'd strongly prefer to see this written everywhere as follows:
   if (already_exists)   {       if (ifNotExists)       {           ereport(NOTICE, …);
heap_close(pg_type_desc,RowExclusiveLock);           return InvalidOid;       }       ereport(ERROR, …);   }
 

The error can be in an else {} or not, I have only a weak preference in
that matter. But the "if (!ifNotExists)" is pretty awkward. Ultimately,
the patch is adding special handling for a new flag, so it makes sense
to test if the flag is set and behave specially, rather than testing if
it's not set to do what was done before.

(Actually, I like the way AlterEnumStmt calls this "skipIfExists". That
reads much more nicely. But there are enough "if_not_exists" elsewhere
in the code that I won't argue to change them in this patch, at least.)

Sorry to nitpick, but I feel quite strongly about this.

> diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
> index 4a03786..9f128bd 100644
> --- a/src/backend/commands/aggregatecmds.c
> +++ b/src/backend/commands/aggregatecmds.c
> @@ -224,6 +224,7 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters)
>                             transfuncName,        /* step function name */
>                             finalfuncName,        /* final function name */
>                             sortoperatorName,    /* sort operator name */
> -                           transTypeId, /* transition data type */
> -                           initval);    /* initial condition */
> +                           transTypeId,    /* transition data type */
> +                           initval,        /* initial condition */
> +                           ifNotExists);    /* if not exists flag */

You should settle on "if not exists" or "if not exists flag" for your
comments. I suggest the former (i.e. no "flag").

I don't see any other problems with the patch. It passes "make check".

I'm marking this as "Waiting on Author".

-- Abhijit