CREATE OR REPLACE AGGREGATE?
So some PostGIS people were griping (on irc) about how the lack of CREATE OR REPLACE AGGREGATE made their life difficult for updates. It struck me that aggregates have acquired a relatively large number of new attributes in recent years, almost all of which are applicable at execution time rather than in parse analysis, so having a CREATE OR REPLACE option seems like a no-brainer. I took a bash at actually writing it and didn't see any obvious problems (I'll post the patch in a bit). Is there some reason (other than shortage of round tuits) why this might not be a good idea, or why it hasn't been done before? -- Andrew (irc:RhodiumToad)
CREATE OR REPLACE AGGREGATE?
So some PostGIS people were griping (on irc) about how the lack of CREATE OR REPLACE AGGREGATE made their life difficult for updates. It struck me that aggregates have acquired a relatively large number of new attributes in recent years, almost all of which are applicable at execution time rather than in parse analysis, so having a CREATE OR REPLACE option seems like a no-brainer. I took a bash at actually writing it and didn't see any obvious problems (I'll post the patch in a bit). Is there some reason (other than shortage of round tuits) why this might not be a good idea, or why it hasn't been done before? -- Andrew (irc:RhodiumToad)
Re: CREATE OR REPLACE AGGREGATE?
On Sun, Mar 17, 2019 at 07:35:16AM +, Andrew Gierth wrote: > I took a bash at actually writing it and didn't see any obvious problems > (I'll post the patch in a bit). Is there some reason (other than > shortage of round tuits) why this might not be a good idea, or why it > hasn't been done before? Indeed. There is not much on the matter in pgsql-hackers as far as I can see, except that but the thread is short: https://www.postgresql.org/message-id/CAGYyBgj3u_4mfTNPMnpOM2NPtWQVPU4WRsYz=rlcf59g-kg...@mail.gmail.com -- Michael signature.asc Description: PGP signature
Re: CREATE OR REPLACE AGGREGATE?
Michael Paquier writes: > On Sun, Mar 17, 2019 at 07:35:16AM +, Andrew Gierth wrote: >> I took a bash at actually writing it and didn't see any obvious problems >> (I'll post the patch in a bit). Is there some reason (other than >> shortage of round tuits) why this might not be a good idea, or why it >> hasn't been done before? > Indeed. Yeah, it seems like mostly a lack-of-round-tuits problem. Updating the aggregate's dependencies correctly might be a bit tricky, but it can't be any worse than the corresponding problem for functions... regards, tom lane
Re: CREATE OR REPLACE AGGREGATE?
>>>>> "Tom" == Tom Lane 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
-CREATE AGGREGATE name ( [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
+CREATE [ OR REPLACE ] AGGREGATE name ( [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
SFUNC = sfunc,
STYPE = state_data_type
[ , SSPACE = state_data_size ]
@@ -44,7 +44,7 @@ CREATE AGGREGATE name ( [ name ( [ [ argmode ] [ argname ] arg_data_type [ , ... ] ]
+CREATE [ OR REPLACE ] AGGREGATE name ( [ [ argmode ] [ argname ] arg_data_type [ , ... ] ]
ORDER BY [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
SFUNC = sfunc,
STYPE = state_data_type
@@ -59,7 +59,7 @@ CREATE AGGREGATE name ( [ [ or the old syntax
-CREATE AGGREGATE name (
+CREATE [ OR REPLACE ] AGGREGATE name (
BASETYPE = base_type,
SFUNC = sfunc,
STYPE = state_data_type
@@ -88,14 +88,23 @@ CREATE AGGREGATE name (
Description
- CREATE AGGREGATE defines a new aggregate
- function. Some basic and commonly-used aggregate functions are
- included with the distribution; they are documented in . If one defines new types or needs
- an aggregate function not already provided, then CREATE
- AGGREGATE can be used to provide the desired features.
+ CREATE AGGREGATE defines a new aggregate function.
+ CREATE OR REPLACE AGGREGATE 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 . If one defines new
+ types or needs an aggregate function not already provided, then
+ CREATE AGGREGATE can be used to provide the desired
+ features.
+
+ 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.
+
+
If a schema name is given (for example, CREATE AGGREGATE
myschema.myagg ...) 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;
+
+
Re: CREATE OR REPLACE AGGREGATE?
On Sun, Mar 17, 2019 at 07:35:16AM +, Andrew Gierth wrote: > I took a bash at actually writing it and didn't see any obvious problems > (I'll post the patch in a bit). Is there some reason (other than > shortage of round tuits) why this might not be a good idea, or why it > hasn't been done before? Indeed. There is not much on the matter in pgsql-hackers as far as I can see, except that but the thread is short: https://www.postgresql.org/message-id/CAGYyBgj3u_4mfTNPMnpOM2NPtWQVPU4WRsYz=rlcf59g-kg...@mail.gmail.com -- Michael signature.asc Description: PGP signature
Re: CREATE OR REPLACE AGGREGATE?
Michael Paquier writes: > On Sun, Mar 17, 2019 at 07:35:16AM +, Andrew Gierth wrote: >> I took a bash at actually writing it and didn't see any obvious problems >> (I'll post the patch in a bit). Is there some reason (other than >> shortage of round tuits) why this might not be a good idea, or why it >> hasn't been done before? > Indeed. Yeah, it seems like mostly a lack-of-round-tuits problem. Updating the aggregate's dependencies correctly might be a bit tricky, but it can't be any worse than the corresponding problem for functions... regards, tom lane
Re: CREATE OR REPLACE AGGREGATE?
>>>>> "Tom" == Tom Lane 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
-CREATE AGGREGATE name ( [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
+CREATE [ OR REPLACE ] AGGREGATE name ( [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
SFUNC = sfunc,
STYPE = state_data_type
[ , SSPACE = state_data_size ]
@@ -44,7 +44,7 @@ CREATE AGGREGATE name ( [ name ( [ [ argmode ] [ argname ] arg_data_type [ , ... ] ]
+CREATE [ OR REPLACE ] AGGREGATE name ( [ [ argmode ] [ argname ] arg_data_type [ , ... ] ]
ORDER BY [ argmode ] [ argname ] arg_data_type [ , ... ] ) (
SFUNC = sfunc,
STYPE = state_data_type
@@ -59,7 +59,7 @@ CREATE AGGREGATE name ( [ [ or the old syntax
-CREATE AGGREGATE name (
+CREATE [ OR REPLACE ] AGGREGATE name (
BASETYPE = base_type,
SFUNC = sfunc,
STYPE = state_data_type
@@ -88,14 +88,23 @@ CREATE AGGREGATE name (
Description
- CREATE AGGREGATE defines a new aggregate
- function. Some basic and commonly-used aggregate functions are
- included with the distribution; they are documented in . If one defines new types or needs
- an aggregate function not already provided, then CREATE
- AGGREGATE can be used to provide the desired features.
+ CREATE AGGREGATE defines a new aggregate function.
+ CREATE OR REPLACE AGGREGATE 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 . If one defines new
+ types or needs an aggregate function not already provided, then
+ CREATE AGGREGATE can be used to provide the desired
+ features.
+
+ 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.
+
+
If a schema name is given (for example, CREATE AGGREGATE
myschema.myagg ...) 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;
+
+
