Re: CREATE OR REPLACE AGGREGATE?

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Yeah, it seems like mostly a lack-of-round-tuits problem.

 Tom> Updating the aggregate's dependencies correctly might be a bit
 Tom> tricky, but it can't be any worse than the corresponding problem
 Tom> for functions...

I was worried about that myself but looking at it, unless I overlooked
something, it's not hard to deal with. The main thing is that all the
dependencies attach to the pg_proc entry, not the pg_aggregate row
(which has no oid anyway), and ProcedureCreate when replacing that will
delete all of the old dependency entries. So all that AggregateCreate
ends up having to do is to create the same set of dependency entries
that it would have created anyway.

Here's my initial draft patch (includes docs but not tests yet) - I have
more testing to do on it, particularly to check the dependencies are
right, but so far it seems to work.

-- 
Andrew (irc:RhodiumToad)

diff --git a/doc/src/sgml/ref/create_aggregate.sgml b/doc/src/sgml/ref/create_aggregate.sgml
index b8cd2e7af9..7530b6770b 100644
--- a/doc/src/sgml/ref/create_aggregate.sgml
+++ b/doc/src/sgml/ref/create_aggregate.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ <replaceable
class="parameter">argmode</replaceable>] [ <replaceable class="parameter">argname</replaceable> ] <replaceable
class="parameter">arg_data_type</replaceable>[ , ... ] ) (
 
+CREATE [ OR REPLACE ] AGGREGATE <replaceable class="parameter">name</replaceable> ( [ <replaceable
class="parameter">argmode</replaceable>] [ <replaceable class="parameter">argname</replaceable> ] <replaceable
class="parameter">arg_data_type</replaceable>[ , ... ] ) (
 
     SFUNC = <replaceable class="parameter">sfunc</replaceable>,
     STYPE = <replaceable class="parameter">state_data_type</replaceable>
     [ , SSPACE = <replaceable class="parameter">state_data_size</replaceable> ]
@@ -44,7 +44,7 @@ CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ <replacea
     [ , PARALLEL = { SAFE | RESTRICTED | UNSAFE } ]
 )
 
-CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ [ <replaceable
class="parameter">argmode</replaceable>] [ <replaceable class="parameter">argname</replaceable> ] <replaceable
class="parameter">arg_data_type</replaceable>[ , ... ] ]
 
+CREATE [ OR REPLACE ] AGGREGATE <replaceable class="parameter">name</replaceable> ( [ [ <replaceable
class="parameter">argmode</replaceable>] [ <replaceable class="parameter">argname</replaceable> ] <replaceable
class="parameter">arg_data_type</replaceable>[ , ... ] ]
 
                         ORDER BY [ <replaceable class="parameter">argmode</replaceable> ] [ <replaceable
class="parameter">argname</replaceable>] <replaceable class="parameter">arg_data_type</replaceable> [ , ... ] ) (
 
     SFUNC = <replaceable class="parameter">sfunc</replaceable>,
     STYPE = <replaceable class="parameter">state_data_type</replaceable>
@@ -59,7 +59,7 @@ CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ [ <replac
 
 <phrase>or the old syntax</phrase>
 
-CREATE AGGREGATE <replaceable class="parameter">name</replaceable> (
+CREATE [ OR REPLACE ] AGGREGATE <replaceable class="parameter">name</replaceable> (
     BASETYPE = <replaceable class="parameter">base_type</replaceable>,
     SFUNC = <replaceable class="parameter">sfunc</replaceable>,
     STYPE = <replaceable class="parameter">state_data_type</replaceable>
@@ -88,14 +88,23 @@ CREATE AGGREGATE <replaceable class="parameter">name</replaceable> (
   <title>Description</title>
 
   <para>
-   <command>CREATE AGGREGATE</command> defines a new aggregate
-   function. Some basic and commonly-used aggregate functions are
-   included with the distribution; they are documented in <xref
-   linkend="functions-aggregate"/>. If one defines new types or needs
-   an aggregate function not already provided, then <command>CREATE
-   AGGREGATE</command> can be used to provide the desired features.
+   <command>CREATE AGGREGATE</command> defines a new aggregate function.
+   <command>CREATE OR REPLACE AGGREGATE</command> will either define a new
+   aggregate function or replace an existing definition. Some basic and
+   commonly-used aggregate functions are included with the distribution; they
+   are documented in <xref linkend="functions-aggregate"/>. If one defines new
+   types or needs an aggregate function not already provided, then
+   <command>CREATE AGGREGATE</command> can be used to provide the desired
+   features.
   </para>
 
+  <para>
+   When replacing an existing definition, the argument types, result type,
+   and number of direct arguments may not be changed. Also, the new definition
+   must be of the same kind (ordinary aggregate, ordered-set aggregate, or
+   hypothetical-set aggregate) as the old one.
+  </para>
+  
   <para>
    If a schema name is given (for example, <literal>CREATE AGGREGATE
    myschema.myagg ...</literal>) then the aggregate function is created in the
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 19e3171bf7..cdc8d9453d 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -45,6 +45,7 @@ static Oid lookup_agg_function(List *fnName, int nargs, Oid *input_types,
 ObjectAddress
 AggregateCreate(const char *aggName,
                 Oid aggNamespace,
+                bool replace,
                 char aggKind,
                 int numArgs,
                 int numDirectArgs,
@@ -77,8 +78,10 @@ AggregateCreate(const char *aggName,
 {
     Relation    aggdesc;
     HeapTuple    tup;
+    HeapTuple    oldtup;
     bool        nulls[Natts_pg_aggregate];
     Datum        values[Natts_pg_aggregate];
+    bool        replaces[Natts_pg_aggregate];
     Form_pg_proc proc;
     Oid            transfn;
     Oid            finalfn = InvalidOid;    /* can be omitted */
@@ -609,7 +612,7 @@ AggregateCreate(const char *aggName,
 
     myself = ProcedureCreate(aggName,
                              aggNamespace,
-                             false, /* no replacement */
+                             replace, /* maybe replacement */
                              false, /* doesn't return a set */
                              finaltype, /* returnType */
                              GetUserId(),    /* proowner */
@@ -648,6 +651,7 @@ AggregateCreate(const char *aggName,
     {
         nulls[i] = false;
         values[i] = (Datum) NULL;
+        replaces[i] = true;
     }
     values[Anum_pg_aggregate_aggfnoid - 1] = ObjectIdGetDatum(procOid);
     values[Anum_pg_aggregate_aggkind - 1] = CharGetDatum(aggKind);
@@ -678,8 +682,51 @@ AggregateCreate(const char *aggName,
     else
         nulls[Anum_pg_aggregate_aggminitval - 1] = true;
 
-    tup = heap_form_tuple(tupDesc, values, nulls);
-    CatalogTupleInsert(aggdesc, tup);
+    if (replace)
+        oldtup = SearchSysCache1(AGGFNOID, ObjectIdGetDatum(procOid));
+    else
+        oldtup = NULL;
+
+    if (HeapTupleIsValid(oldtup))
+    {
+        Form_pg_aggregate oldagg = (Form_pg_aggregate) GETSTRUCT(oldtup);
+
+        /*
+         * If we're replacing an existing entry, we need to validate that
+         * we're not changing anything that would break callers.
+         * Specifically we must not change aggkind or aggnumdirectargs,
+         * which affect how an aggregate call is treated in parse
+         * analysis.
+         */
+        if (aggKind != oldagg->aggkind)
+            ereport(ERROR,
+                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                     errmsg("cannot change routine kind"),
+                     (oldagg->aggkind == AGGKIND_NORMAL ?
+                      errdetail("\"%s\" is an ordinary aggregate function.", aggName) :
+                      oldagg->aggkind == AGGKIND_ORDERED_SET ?
+                      errdetail("\"%s\" is an ordered-set aggregate.", aggName) :
+                      oldagg->aggkind == AGGKIND_HYPOTHETICAL ?
+                      errdetail("\"%s\" is a hypothetical-set aggregate.", aggName) :
+                      0)));
+        if (numDirectArgs != oldagg->aggnumdirectargs)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+                     errmsg("cannot change number of direct args of an aggregate function")));
+
+        replaces[Anum_pg_aggregate_aggfnoid - 1] = false;
+        replaces[Anum_pg_aggregate_aggkind - 1] = false;
+        replaces[Anum_pg_aggregate_aggnumdirectargs - 1] = false;
+
+        tup = heap_modify_tuple(oldtup, tupDesc, values, nulls, replaces);
+        CatalogTupleUpdate(aggdesc, &tup->t_self, tup);
+        ReleaseSysCache(oldtup);
+    }
+    else
+    {
+        tup = heap_form_tuple(tupDesc, values, nulls);
+        CatalogTupleInsert(aggdesc, tup);
+    }
 
     table_close(aggdesc, RowExclusiveLock);
 
@@ -688,6 +735,10 @@ AggregateCreate(const char *aggName,
      * made by ProcedureCreate).  Note: we don't need an explicit dependency
      * on aggTransType since we depend on it indirectly through transfn.
      * Likewise for aggmTransType using the mtransfunc, if it exists.
+     *
+     * If we're replacing an existing definition, ProcedureCreate deleted all
+     * our existing dependencies, so we have to do the same things here either
+     * way.
      */
 
     /* Depends on transition function */
diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
index d00765fbc7..d569067dc4 100644
--- a/src/backend/commands/aggregatecmds.c
+++ b/src/backend/commands/aggregatecmds.c
@@ -54,7 +54,12 @@ static char extractModify(DefElem *defel);
  * "parameters" is a list of DefElem representing the agg's definition clauses.
  */
 ObjectAddress
-DefineAggregate(ParseState *pstate, List *name, List *args, bool oldstyle, List *parameters)
+DefineAggregate(ParseState *pstate,
+                List *name,
+                List *args,
+                bool oldstyle,
+                List *parameters,
+                bool replace)
 {
     char       *aggName;
     Oid            aggNamespace;
@@ -436,6 +441,7 @@ DefineAggregate(ParseState *pstate, List *name, List *args, bool oldstyle, List
      */
     return AggregateCreate(aggName, /* aggregate name */
                            aggNamespace,    /* namespace */
+                           replace,
                            aggKind,
                            numArgs,
                            numDirectArgs,
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index a8a735c247..6f3565ad20 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3372,6 +3372,7 @@ _copyDefineStmt(const DefineStmt *from)
     COPY_NODE_FIELD(args);
     COPY_NODE_FIELD(definition);
     COPY_SCALAR_FIELD(if_not_exists);
+    COPY_SCALAR_FIELD(replace);
 
     return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 3cab90e9f8..813606ce0e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1265,6 +1265,7 @@ _equalDefineStmt(const DefineStmt *a, const DefineStmt *b)
     COMPARE_NODE_FIELD(args);
     COMPARE_NODE_FIELD(definition);
     COMPARE_SCALAR_FIELD(if_not_exists);
+    COMPARE_SCALAR_FIELD(replace);
 
     return true;
 }
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e23e68fdb3..ff4c9cd13b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5617,25 +5617,27 @@ CreateAssertionStmt:
  *****************************************************************************/
 
 DefineStmt:
-            CREATE AGGREGATE func_name aggr_args definition
+            CREATE opt_or_replace 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->replace = $2;
+                    n->defnames = $4;
+                    n->args = $5;
+                    n->definition = $6;
                     $$ = (Node *)n;
                 }
-            | CREATE AGGREGATE func_name old_aggr_definition
+            | CREATE opt_or_replace AGGREGATE func_name old_aggr_definition
                 {
                     /* old-style (pre-8.2) syntax for CREATE AGGREGATE */
                     DefineStmt *n = makeNode(DefineStmt);
                     n->kind = OBJECT_AGGREGATE;
                     n->oldstyle = true;
-                    n->defnames = $3;
+                    n->replace = $2;
+                    n->defnames = $4;
                     n->args = NIL;
-                    n->definition = $4;
+                    n->definition = $5;
                     $$ = (Node *)n;
                 }
             | CREATE OPERATOR any_operator definition
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6ec795f1b4..8e2e13d8bb 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1237,7 +1237,8 @@ ProcessUtilitySlow(ParseState *pstate,
                             address =
                                 DefineAggregate(pstate, stmt->defnames, stmt->args,
                                                 stmt->oldstyle,
-                                                stmt->definition);
+                                                stmt->definition,
+                                                stmt->replace);
                             break;
                         case OBJECT_OPERATOR:
                             Assert(stmt->args == NIL);
diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index 832b7c2145..0b111b1283 100644
--- a/src/include/catalog/pg_aggregate.h
+++ b/src/include/catalog/pg_aggregate.h
@@ -142,6 +142,7 @@ typedef FormData_pg_aggregate *Form_pg_aggregate;
 
 extern ObjectAddress AggregateCreate(const char *aggName,
                 Oid aggNamespace,
+                bool replace,
                 char aggKind,
                 int numArgs,
                 int numDirectArgs,
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index e592a914a4..3bc2e8eb16 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -94,7 +94,7 @@ extern void UpdateStatisticsForTypeChange(Oid statsOid,
 
 /* commands/aggregatecmds.c */
 extern ObjectAddress DefineAggregate(ParseState *pstate, List *name, List *args, bool oldstyle,
-                List *parameters);
+                                     List *parameters, bool replace);
 
 /* commands/opclasscmds.c */
 extern ObjectAddress DefineOpClass(CreateOpClassStmt *stmt);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index fe35783359..e72ff4ac69 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2532,6 +2532,7 @@ typedef struct DefineStmt
     List       *args;            /* a list of TypeName (if needed) */
     List       *definition;        /* a list of DefElem */
     bool        if_not_exists;    /* just do nothing if it already exists? */
+    bool        replace;        /* replace if already exists? */
 } DefineStmt;
 
 /* ----------------------