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

Ok.

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

I could not use the "opt_if_not_exists" because bison emits an error:

I really don't know how to solve this problem. I'm just do ajustments like that:

I changed all statements to use "opt_if_not_exists" (like above) but bison do not

accept it.

Looking more carefully at gram.y when we use "IF_P EXISTS" (DROP ROLE IF 

EXISTS) all was written using variants of original statement. And exists the rule 

"opt_if_exists" too, that's used in "DROP CAST". Because of this reason I use 

"opt_if_not_exists" in "CREATE CAST". 

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

Ok.

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

Fixed.

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

Fixed.

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

Fixed.

> > @@ -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.)
>

Fixed.

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

Fixed.

> (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.)
>

You all right, and in other places we have "missing_ok".

When I finish this patch and the next one (to add IF NOT EXISTS to

remaining CREATE statements) I send a patch to normalize that.

> 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").
>

Fixed.

> I don't see any other problems with the patch. It passes "make check".
>
> I'm marking this as "Waiting on Author".
>

And now I'm marking this as "Needs Review" (again) ;-)

Regards,

--
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento