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

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.

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