From 1b2926c07626280245efddde3fe4103c79d85a2e Mon Sep 17 00:00:00 2001 From: Mats Kindahl Date: Wed, 6 Apr 2022 17:30:18 +0200 Subject: [PATCH] Do not modify aggregation state in finalize The function `tsl_finalize_agg_ffunc` modified the aggregation state by setting `trans_value` to the final result when computing the final value. Since the state can be re-used several times, there could be several calls to the finalization function, and the finalization function would be confused when passed a final value instead of a aggregation state transition value. This commit fixes this by not modifying the `trans_value` when computing the final value and instead just returns it (or the original `trans_value` if there is no finalization function). Fixes #3248 --- tsl/src/partialize_finalize.c | 20 ++++++++++----- tsl/test/expected/continuous_aggs.out | 35 +++++++++++++++++++++++++++ tsl/test/sql/continuous_aggs.sql | 29 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/tsl/src/partialize_finalize.c b/tsl/src/partialize_finalize.c index 5b1fd22aa..e88ca92a6 100644 --- a/tsl/src/partialize_finalize.c +++ b/tsl/src/partialize_finalize.c @@ -518,14 +518,21 @@ tsl_finalize_agg_sfunc(PG_FUNCTION_ARGS) PG_RETURN_POINTER(tstate); } -/* tsl_finalize_agg_ffunc: - * apply the finalize function on the state we have accumulated +/* + * Apply the finalize function on the state we have accumulated + * + * The state passed down to this function can be used by other aggregate + * functions so it is important to not change the state when computing the + * final result. */ Datum tsl_finalize_agg_ffunc(PG_FUNCTION_ARGS) { FATransitionState *tstate = PG_ARGISNULL(0) ? NULL : (FATransitionState *) PG_GETARG_POINTER(0); MemoryContext fa_context, old_context; + Datum result = tstate->per_group_state->trans_value; + bool result_isnull = tstate->per_group_state->trans_value_isnull; + Assert(tstate != NULL); if (!AggCheckCallContext(fcinfo, &fa_context)) { @@ -533,6 +540,7 @@ tsl_finalize_agg_ffunc(PG_FUNCTION_ARGS) elog(ERROR, "finalize_agg_ffunc called in non-aggregate context"); } old_context = MemoryContextSwitchTo(fa_context); + if (OidIsValid(tstate->per_query_state->final_meta.finalfnoid)) { /* don't execute if strict and the trans value is NULL or there are extra args (all extra @@ -547,15 +555,15 @@ tsl_finalize_agg_ffunc(PG_FUNCTION_ARGS) FC_NULL(finalfn_fcinfo, 0) = tstate->per_group_state->trans_value_isnull; finalfn_fcinfo->isnull = false; - tstate->per_group_state->trans_value = FunctionCallInvoke(finalfn_fcinfo); - tstate->per_group_state->trans_value_isnull = finalfn_fcinfo->isnull; + result = FunctionCallInvoke(finalfn_fcinfo); + result_isnull = finalfn_fcinfo->isnull; } } MemoryContextSwitchTo(old_context); - if (tstate->per_group_state->trans_value_isnull) + if (result_isnull) PG_RETURN_NULL(); else - PG_RETURN_DATUM(tstate->per_group_state->trans_value); + PG_RETURN_DATUM(result); } /* diff --git a/tsl/test/expected/continuous_aggs.out b/tsl/test/expected/continuous_aggs.out index 2b8082d36..2f3927a20 100644 --- a/tsl/test/expected/continuous_aggs.out +++ b/tsl/test/expected/continuous_aggs.out @@ -1703,3 +1703,38 @@ where view_name = 'test_morecols_cagg'; test_morecols_cagg | t | t (1 row) +CREATE TABLE issue3248(filler_1 int, filler_2 int, filler_3 int, time timestamptz NOT NULL, device_id int, v0 int, v1 int, v2 float, v3 float); +CREATE INDEX ON issue3248(time DESC); +CREATE INDEX ON issue3248(device_id,time DESC); +SELECT create_hypertable('issue3248','time',create_default_indexes:=false); + create_hypertable +------------------------- + (46,public,issue3248,t) +(1 row) + +ALTER TABLE issue3248 DROP COLUMN filler_1; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id+1, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-01 0:00:00+0'::timestamptz,'2000-01-05 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ALTER TABLE issue3248 DROP COLUMN filler_2; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id-1, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-06 0:00:00+0'::timestamptz,'2000-01-12 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ALTER TABLE issue3248 DROP COLUMN filler_3; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-13 0:00:00+0'::timestamptz,'2000-01-19 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ANALYZE issue3248; +CREATE materialized view issue3248_cagg WITH (timescaledb.continuous) +AS SELECT time_bucket('1h',time), device_id, min(v0), max(v1), avg(v2) +FROM issue3248 GROUP BY 1,2; +NOTICE: refreshing continuous aggregate "issue3248_cagg" +SELECT + FROM issue3248 AS m, + LATERAL(SELECT m FROM issue3248_cagg WHERE avg IS NULL LIMIT 1) AS lat; +-- +(0 rows) + diff --git a/tsl/test/sql/continuous_aggs.sql b/tsl/test/sql/continuous_aggs.sql index 438d10e59..96610595a 100644 --- a/tsl/test/sql/continuous_aggs.sql +++ b/tsl/test/sql/continuous_aggs.sql @@ -1197,3 +1197,32 @@ SELECT view_name, materialized_only, compression_enabled FROM timescaledb_information.continuous_aggregates where view_name = 'test_morecols_cagg'; +CREATE TABLE issue3248(filler_1 int, filler_2 int, filler_3 int, time timestamptz NOT NULL, device_id int, v0 int, v1 int, v2 float, v3 float); +CREATE INDEX ON issue3248(time DESC); +CREATE INDEX ON issue3248(device_id,time DESC); +SELECT create_hypertable('issue3248','time',create_default_indexes:=false); + +ALTER TABLE issue3248 DROP COLUMN filler_1; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id+1, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-01 0:00:00+0'::timestamptz,'2000-01-05 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ALTER TABLE issue3248 DROP COLUMN filler_2; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id-1, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-06 0:00:00+0'::timestamptz,'2000-01-12 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ALTER TABLE issue3248 DROP COLUMN filler_3; +INSERT INTO issue3248(time,device_id,v0,v1,v2,v3) +SELECT time, device_id, device_id, device_id + 2, device_id + 0.5, NULL +FROM generate_series('2000-01-13 0:00:00+0'::timestamptz,'2000-01-19 23:55:00+0','8h') gtime(time), + generate_series(1,5,1) gdevice(device_id); +ANALYZE issue3248; + +CREATE materialized view issue3248_cagg WITH (timescaledb.continuous) +AS SELECT time_bucket('1h',time), device_id, min(v0), max(v1), avg(v2) +FROM issue3248 GROUP BY 1,2; + +SELECT + FROM issue3248 AS m, + LATERAL(SELECT m FROM issue3248_cagg WHERE avg IS NULL LIMIT 1) AS lat;