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

On Sat, Jul 13, 2013 at 7:21 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2013-06-12 14:29:59 -0300, fabriziomello(at)gmail(dot)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.
>

Abhijit, thanks for your review.

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

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:

/usr/bin/bison -d -o gram.c gram.y
gram.y: conflicts: 10 shift/reduce
gram.y: expected 0 shift/reduce conflicts
make[3]: *** [gram.c] Error 1

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

CREATE AGGREGATE opt_if_not_exists func_name aggr_args
definition
{
DefineStmt *n = makeNode(DefineStmt);
n->kind = OBJECT_AGGREGATE;
n->oldstyle = false;
n->defnames = $4;
n->args = $5;
n->definition = $6;
n->if_not_exists = $3;
$$ = (Node *)n;
}

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