Re: [HACKERS] Aggregate function API versus grouping sets
On 07/02/2014 10:15 PM, Andrew Gierth wrote: > (Had one request so far for a mode() variant that returns the unique > modal value if one exists, otherwise null; so the current set of > ordered-set aggs by no means exhausts the possible applications.) I resemble that remark. :) But seriously, am I the only one who doesn't want some random value when there is no dominant one? And the fact that I can't create my own without C code is a real bummer. -- Vik -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Andrew Gierth writes: > Here are two more, cumulative with the previous patch and each other: > The first removes the assumption in ordered_set_startup that the > aggcontext can be cached in fn_extra, and puts it in the transvalue > instead. OK, done. (ATM it seems like we wouldn't need gcontext in the transvalue either, but I left it there in case we want it later.) > The second exposes the OSA* structures in a header file, so that > user-defined ordered-set aggs can use ordered_set_transition[_multi] > directly rather than having to cargo-cult it into their own code. I'm not very happy about this one; we intentionally did not expose these structs, so that we could freely make fixes like, for example, your immediately preceding patch. I think it'd be worth considering whether there's a way to allow third-party ordered-set aggs to use the infrastructure in orderedsetaggs.c while still treating these structs as opaque. In any case we'd need a more carefully thought-through API than just decreeing that some private structs are now public. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
> "Tom" == Tom Lane writes: >> How's this? (applies and passes regression on 9.4 and master) Tom> Pushed with minor cosmetic adjustments. Thanks! -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> If we're going to do that, I think it needs to be in 9.4. Are > Tom> you going to work up a patch? > How's this? (applies and passes regression on 9.4 and master) Pushed with minor cosmetic adjustments. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Here are two more, cumulative with the previous patch and each other:
The first removes the assumption in ordered_set_startup that the
aggcontext can be cached in fn_extra, and puts it in the transvalue
instead.
The second exposes the OSA* structures in a header file, so that
user-defined ordered-set aggs can use ordered_set_transition[_multi]
directly rather than having to cargo-cult it into their own code.
--
Andrew (irc:RhodiumToad)
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index d116a63..92fa9f3 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -47,8 +47,6 @@ typedef struct OSAPerQueryState
Aggref *aggref;
/* Memory context containing this struct and other per-query data: */
MemoryContext qcontext;
- /* Memory context containing per-group data: */
- MemoryContext gcontext;
/* These fields are used only when accumulating tuples: */
@@ -86,6 +84,8 @@ typedef struct OSAPerGroupState
{
/* Link to the per-query state for this aggregate: */
OSAPerQueryState *qstate;
+ /* MemoryContext for per-group data */
+ MemoryContext gcontext;
/* Sort object we're accumulating data in: */
Tuplesortstate *sortstate;
/* Number of normal rows inserted into sortstate: */
@@ -104,6 +104,15 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
OSAPerGroupState *osastate;
OSAPerQueryState *qstate;
MemoryContext oldcontext;
+ MemoryContext gcontext;
+
+ /*
+ * Check we're called as aggregate (and not a window function), and
+ * get the Agg node's group-lifespan context (which might not be the
+ * same throughout the query, but will be the same for each transval)
+ */
+ if (AggCheckCallContext(fcinfo, &gcontext) != AGG_CONTEXT_AGGREGATE)
+ elog(ERROR, "ordered-set aggregate called in non-aggregate context");
/*
* We keep a link to the per-query state in fn_extra; if it's not there,
@@ -114,16 +123,9 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
{
Aggref *aggref;
MemoryContext qcontext;
- MemoryContext gcontext;
List *sortlist;
int numSortCols;
- /*
- * Check we're called as aggregate (and not a window function), and
- * get the Agg node's group-lifespan context
- */
- if (AggCheckCallContext(fcinfo, &gcontext) != AGG_CONTEXT_AGGREGATE)
- elog(ERROR, "ordered-set aggregate called in non-aggregate context");
/* Need the Aggref as well */
aggref = AggGetAggref(fcinfo);
if (!aggref)
@@ -142,7 +144,6 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
qstate = (OSAPerQueryState *) palloc0(sizeof(OSAPerQueryState));
qstate->aggref = aggref;
qstate->qcontext = qcontext;
- qstate->gcontext = gcontext;
/* Extract the sort information */
sortlist = aggref->aggorder;
@@ -259,10 +260,11 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
}
/* Now build the stuff we need in group-lifespan context */
- oldcontext = MemoryContextSwitchTo(qstate->gcontext);
+ oldcontext = MemoryContextSwitchTo(gcontext);
osastate = (OSAPerGroupState *) palloc(sizeof(OSAPerGroupState));
osastate->qstate = qstate;
+ osastate->gcontext = gcontext;
/* Initialize tuplesort object */
if (use_tuples)
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 92fa9f3..bbad0e5 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -16,6 +16,8 @@
#include
+#include "utils/orderedset.h"
+
#include "catalog/pg_aggregate.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_type.h"
@@ -30,71 +32,8 @@
#include "utils/tuplesort.h"
-/*
- * Generic support for ordered-set aggregates
- *
- * The state for an ordered-set aggregate is divided into a per-group struct
- * (which is the internal-type transition state datum returned to nodeAgg.c)
- * and a per-query struct, which contains data and sub-objects that we can
- * create just once per query because they will not change across groups.
- * The per-query struct and subsidiary data live in the executor's per-query
- * memory context, and go away implicitly at ExecutorEnd().
- */
-
-typedef struct OSAPerQueryState
-{
- /* Aggref for this aggregate: */
- Aggref *aggref;
- /* Memory context containing this struct and other per-query data: */
- MemoryContext qcontext;
-
- /* These fields are used only when accumulating tuples: */
-
- /* Tuple descriptor for tuples inserted into sortstate: */
- TupleDesc tupdesc;
- /* Tuple slot we can use for inserting/extracting tuples: */
- TupleTableSlot *tupslot;
- /* Per-sort-column sorting information */
- int numSortCols;
- AttrNumber *sortColIdx;
- Oid *sortOperators;
- Oid *eqOperators;
- Oid *sortCollations;
- bool *sortNullsFirsts;
- /* Equality operator call info, created only if needed: */
- FmgrInfo *equalfns;
-
- /* These fields are used only when accumulating datums: */
-
- /* Inf
Re: [HACKERS] Aggregate function API versus grouping sets
> "Tom" == Tom Lane writes:
Tom> If we're going to do that, I think it needs to be in 9.4. Are
Tom> you going to work up a patch?
How's this? (applies and passes regression on 9.4 and master)
--
Andrew (irc:RhodiumToad)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 09ff035..0388735 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2199,44 +2199,56 @@ AggGetAggref(FunctionCallInfo fcinfo)
}
/*
- * AggGetPerTupleEContext - fetch per-input-tuple ExprContext
+ * AggGetTempMemoryContext - fetch short-term memory context
*
- * This is useful in agg final functions; the econtext returned is the
- * same per-tuple context that the transfn was called in (which can
- * safely get reset during the final function).
+ * This is useful in agg final functions; the context returned is one that the
+ * final function can safely reset as desired. This isn't useful for transition
+ * functions, since the context returned MAY (we don't promise) be the same as
+ * the context those are called in.
*
* As above, this is currently not useful for aggs called as window functions.
*/
-ExprContext *
-AggGetPerTupleEContext(FunctionCallInfo fcinfo)
+MemoryContext
+AggGetTempMemoryContext(FunctionCallInfo fcinfo)
{
if (fcinfo->context && IsA(fcinfo->context, AggState))
{
AggState *aggstate = (AggState *) fcinfo->context;
- return aggstate->tmpcontext;
+ return aggstate->tmpcontext->ecxt_per_tuple_memory;
}
return NULL;
}
/*
- * AggGetPerAggEContext - fetch per-output-tuple ExprContext
+ * AggRegisterCallback - register a cleanup callback linked to aggcontext
*
* This is useful for aggs to register shutdown callbacks, which will ensure
- * that non-memory resources are freed.
+ * that non-memory resources are freed. These callbacks will occur just before
+ * the associated aggcontext (as returned by AggCheckCallContext) is reset,
+ * either between groups or as a result of rescanning the query. They will NOT
+ * be called on error paths. The primary intent is to allow for freeing of
+ * tuplestores or tuplesorts maintained in aggcontext, or pins held by slots
+ * created by the agg functions. (They will not be called until after the
+ * result of the finalfn is no longer needed, so it's safe for the finalfn to
+ * return data that will be freed by the callback.)
*
* As above, this is currently not useful for aggs called as window functions.
*/
-ExprContext *
-AggGetPerAggEContext(FunctionCallInfo fcinfo)
+void
+AggRegisterCallback(FunctionCallInfo fcinfo,
+ ExprContextCallbackFunction func,
+ Datum arg)
{
if (fcinfo->context && IsA(fcinfo->context, AggState))
{
AggState *aggstate = (AggState *) fcinfo->context;
- return aggstate->ss.ps.ps_ExprContext;
+ RegisterExprContextCallback(aggstate->ss.ps.ps_ExprContext, func, arg);
+
+ return;
}
- return NULL;
+ elog(ERROR,"Aggregate function cannot register a callback in this context");
}
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index efb0411..d116a63 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -49,8 +49,6 @@ typedef struct OSAPerQueryState
MemoryContext qcontext;
/* Memory context containing per-group data: */
MemoryContext gcontext;
- /* Agg plan node's output econtext: */
- ExprContext *peraggecontext;
/* These fields are used only when accumulating tuples: */
@@ -117,7 +115,6 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
Aggref *aggref;
MemoryContext qcontext;
MemoryContext gcontext;
- ExprContext *peraggecontext;
List *sortlist;
int numSortCols;
@@ -133,10 +130,6 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
elog(ERROR, "ordered-set aggregate called in non-aggregate context");
if (!AGGKIND_IS_ORDERED_SET(aggref->aggkind))
elog(ERROR, "ordered-set aggregate support function called for non-ordered-set aggregate");
- /* Also get output exprcontext so we can register shutdown callback */
- peraggecontext = AggGetPerAggEContext(fcinfo);
- if (!peraggecontext)
- elog(ERROR, "ordered-set aggregate called in non-aggregate context");
/*
* Prepare per-query structures in the fn_mcxt, which we assume is the
@@ -150,7 +143,6 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
qstate->aggref = aggref;
qstate->qcontext = qcontext;
qstate->gcontext = gcontext;
- qstate->peraggecontext = peraggecontext;
/* Extract the sort information */
sortlist = aggref->aggorder;
@@ -291,9 +283,9 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
osastate->number_of_rows = 0;
/* Now register a shutdown callback to clean things up */
- RegisterExprContextCallback(qstate->peraggecontext,
-ordered_set_shutdown,
-PointerGetDatum(osastate));
+ AggRegisterCallback(fcinfo,
+ ordered_set_s
Re: [HACKERS] Aggregate function API versus grouping sets
> "Tom" == Tom Lane writes: >> Do we want a decision on the fn_extra matter first, or shall I do >> one patch for the econtext, and a following one for fn_extra? Tom> I think they're somewhat independent, and probably best patched Tom> separately. In any case orderedsetagg.c's use of fn_extra is a Tom> local matter that we'd not really have to fix in 9.4, except to Tom> the extent that you think third-party code might copy it. Given that there's been no attempt to expose ordered_set_startup / ordered_set_transition* as some sort of API, I think it's virtually inevitable that people will cargo-cult all of that code into any new ordered set aggregate they might wish to create. (Had one request so far for a mode() variant that returns the unique modal value if one exists, otherwise null; so the current set of ordered-set aggs by no means exhausts the possible applications.) -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> If we're going to do that, I think it needs to be in 9.4. Are > Tom> you going to work up a patch? > Do we want a decision on the fn_extra matter first, or shall I do one > patch for the econtext, and a following one for fn_extra? I think they're somewhat independent, and probably best patched separately. In any case orderedsetagg.c's use of fn_extra is a local matter that we'd not really have to fix in 9.4, except to the extent that you think third-party code might copy it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
> "Tom" == Tom Lane writes: Tom> Another approach would be to remove AggGetPerAggEContext as such Tom> from the API altogether, and instead offer an interface that Tom> says "register an aggregate cleanup callback", leaving it to the Tom> agg/window core code to figure out which context to hang that Tom> on. I had thought that exposing this econtext and the Tom> per-input-tuple one would provide useful generality, but maybe Tom> we should rethink that. >> Works for me. Tom> If we're going to do that, I think it needs to be in 9.4. Are Tom> you going to work up a patch? Do we want a decision on the fn_extra matter first, or shall I do one patch for the econtext, and a following one for fn_extra? -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> Another approach would be to remove AggGetPerAggEContext as such > Tom> from the API altogether, and instead offer an interface that > Tom> says "register an aggregate cleanup callback", leaving it to the > Tom> agg/window core code to figure out which context to hang that > Tom> on. I had thought that exposing this econtext and the > Tom> per-input-tuple one would provide useful generality, but maybe > Tom> we should rethink that. > Works for me. If we're going to do that, I think it needs to be in 9.4. Are you going to work up a patch? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
> "Tom" == Tom Lane writes: >> Doing rollup via GroupAggregate by maintaining multiple transition >> values at a time (one per grouping set) means that the transfn is >> being called interleaved for transition values in different >> contexts. So the question becomes: is it wrong for the transition >> function to assume that aggcontext can be cached this way, or is >> it necessary for the executor to use a separate flinfo for each >> concurrent grouping set? Tom> Hm. We could probably move gcontext into the per-group data. Tom> I'm not sure though if there are any other dependencies there on Tom> the groups being evaluated serially. More generally, I wonder Tom> whether user-defined aggregates are likely to be making Tom> assumptions that will be broken by this. The thing is that almost everything _except_ aggcontext and AggGetPerAggEContext that a transfn might want to hang off fn_extra really is going to be constant across all groups. The big question, as you say, is whether this is going to be an issue for existing user-defined aggregates. >> Since it seems that the cleanup callback is the sole reason for >> this function to exist, is it acceptable to remove any implication >> that the context returned is the overall per-output-tuple one, and >> simply state that it is a context whose cleanup functions are >> called at the appropriate time before aggcontext is reset? Tom> Well, I think the implication is that it's the econtext in which Tom> the result of the aggregation will be used. In the rollup case, though, it does not seem reasonable to have multiple result-tuple econtexts (that would significantly complicate the projection of rows, the handling of rescans, etc.). Tom> Another approach would be to remove AggGetPerAggEContext as such Tom> from the API altogether, and instead offer an interface that Tom> says "register an aggregate cleanup callback", leaving it to the Tom> agg/window core code to figure out which context to hang that Tom> on. I had thought that exposing this econtext and the Tom> per-input-tuple one would provide useful generality, but maybe Tom> we should rethink that. Works for me. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregate function API versus grouping sets
Andrew Gierth writes: > 1. Assumptions about the relationship between aggcontext and fn_extra > Tom's version of the ordered-set aggregate code makes the assumption > that it is safe to store the aggcontext returned by AggCheckCallContext > in a structure hung off flinfo->fn_extra. This implicitly assumes that > the function will be called in only one aggcontext, or at least only one > per flinfo. > Doing rollup via GroupAggregate by maintaining multiple transition > values at a time (one per grouping set) means that the transfn is being > called interleaved for transition values in different contexts. So the > question becomes: is it wrong for the transition function to assume that > aggcontext can be cached this way, or is it necessary for the executor > to use a separate flinfo for each concurrent grouping set? Hm. We could probably move gcontext into the per-group data. I'm not sure though if there are any other dependencies there on the groups being evaluated serially. More generally, I wonder whether user-defined aggregates are likely to be making assumptions that will be broken by this. > 2. AggGetPerAggEContext > The comment documents this as returning the per-output-tuple econtext, > and the ordered-set code assumes that the rescan of this context implies > that the aggcontext is also about to be reset (so cleanup functions can > be called). > Since it seems that the cleanup callback is the sole reason for this > function to exist, is it acceptable to remove any implication that the > context returned is the overall per-output-tuple one, and simply state > that it is a context whose cleanup functions are called at the > appropriate time before aggcontext is reset? Well, I think the implication is that it's the econtext in which the result of the aggregation will be used. Another approach would be to remove AggGetPerAggEContext as such from the API altogether, and instead offer an interface that says "register an aggregate cleanup callback", leaving it to the agg/window core code to figure out which context to hang that on. I had thought that exposing this econtext and the per-input-tuple one would provide useful generality, but maybe we should rethink that. Obviously, the time grows short to reconsider this API, but it's not quite too late. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Aggregate function API versus grouping sets
I've been assisting Atri with development of an implementation of GROUPING SETS, beginning with a one-pass implementation of ROLLUP. Two issues have arisen regarding the API for calling aggregate transition and final functions that I think need answering now, since they relate to changes in 9.4. 1. Assumptions about the relationship between aggcontext and fn_extra Tom's version of the ordered-set aggregate code makes the assumption that it is safe to store the aggcontext returned by AggCheckCallContext in a structure hung off flinfo->fn_extra. This implicitly assumes that the function will be called in only one aggcontext, or at least only one per flinfo. Doing rollup via GroupAggregate by maintaining multiple transition values at a time (one per grouping set) means that the transfn is being called interleaved for transition values in different contexts. So the question becomes: is it wrong for the transition function to assume that aggcontext can be cached this way, or is it necessary for the executor to use a separate flinfo for each concurrent grouping set? 2. AggGetPerAggEContext The comment documents this as returning the per-output-tuple econtext, and the ordered-set code assumes that the rescan of this context implies that the aggcontext is also about to be reset (so cleanup functions can be called). Since it seems that the cleanup callback is the sole reason for this function to exist, is it acceptable to remove any implication that the context returned is the overall per-output-tuple one, and simply state that it is a context whose cleanup functions are called at the appropriate time before aggcontext is reset? (In fact what I'm considering is making aggcontext be the per-tuple memory context of an ExprContext created for each grouping set, and having AggGetPerAggEContext return this - but it won't be the actual context in which the result projection happens.) -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
