At 2013-06-12 14:29:59 -0300, fabriziome...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
