Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add check for NULL offset for caggs built on top of caggs (#7272) #7290

Merged
merged 1 commit into from
Sep 26, 2024
+99 −18
Merged

Add check for NULL offset for caggs built on top of caggs (#7272) #7290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .unreleased/pr_7290
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixes: #7290 Add check for NULL offset for caggs built on top of caggs
Thanks: @snyrkill for discovering and reporting the issue
15 changes: 11 additions & 4 deletions tsl/src/continuous_aggs/common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,13 +1044,15 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
if (bucket_info.bf->bucket_time_offset != NULL ||
bucket_info_parent.bf->bucket_time_offset != NULL)
{
bool bucket_offset_isnull = bucket_info.bf->bucket_time_offset == NULL;
bool bucket_offset_parent_isnull = bucket_info_parent.bf->bucket_time_offset == NULL;

Datum offset_datum = IntervalPGetDatum(bucket_info.bf->bucket_time_offset);
Datum offset_datum_parent =
IntervalPGetDatum(bucket_info_parent.bf->bucket_time_offset);

bool both_buckets_are_equal = false;
bool both_buckets_have_offset = (bucket_info.bf->bucket_time_offset != NULL) &&
(bucket_info_parent.bf->bucket_time_offset != NULL);
bool both_buckets_have_offset = !bucket_offset_isnull && !bucket_offset_parent_isnull;

if (both_buckets_have_offset)
{
Expand All @@ -1060,9 +1062,14 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s

if (!both_buckets_are_equal)
{
char *offset = DatumGetCString(DirectFunctionCall1(interval_out, offset_datum));
char *offset =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this probably works, I am wondering if we shouldn't check this further up around line 1044.

It seems one of the bucket_time_offsets is NULL, so I think we should have NULL check on the original C pointer before trying to convert to a Datum. We should probably not call IntervalPGetDatum() on a NULL pointer to begin with and check the bucket_time_offset_pointer instead. One alternative is to also use a NullableDatum to pass this info down.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @erimatnor!

I think using NullableDatum would make sense for datums stored by value. Interval is stored as a pointer so we can just compare it with NULL directly. And IntervalPGetDatum just converts the pointer type to datum, doesn't dereference it.

#define IntervalPGetDatum(X) PointerGetDatum(X)

So while I could implement what you are suggesting, I'm not sure it is really needed here.

!bucket_offset_isnull ?
DatumGetCString(DirectFunctionCall1(interval_out, offset_datum)) :
"NULL";
char *offset_parent =
DatumGetCString(DirectFunctionCall1(interval_out, offset_datum_parent));
!bucket_offset_parent_isnull ?
DatumGetCString(DirectFunctionCall1(interval_out, offset_datum_parent)) :
"NULL";

ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
Expand Down
39 changes: 32 additions & 7 deletions tsl/test/expected/cagg_query.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2366,26 +2366,51 @@ CREATE MATERIALIZED VIEW cagg_1_week_offset
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:889: ERROR: cannot create continuous aggregate with different bucket offset values
-- Cagg with NULL offset on top of cagg with non-NULL offset
\set VERBOSITY default
CREATE MATERIALIZED VIEW cagg_1_week_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>NULL::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:897: ERROR: cannot create continuous aggregate with different bucket offset values
DETAIL: Time origin of "public.cagg_1_week_null_offset" [NULL] and "public.cagg_1_hour_offset" [@ 30 mins] should be the same.
-- Cagg with non-NULL offset on top of cagg with NULL offset
CREATE MATERIALIZED VIEW cagg_1_hour_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 hour', time, "offset"=>NULL::interval) AS hour_bucket, max(value) AS max_value
FROM temperature
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:904: NOTICE: refreshing continuous aggregate "cagg_1_hour_null_offset"
HINT: Use WITH NO DATA if you do not want to refresh the continuous aggregate on creation.
CREATE MATERIALIZED VIEW cagg_1_week_non_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>'35m'::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_null_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:910: ERROR: cannot create continuous aggregate with different bucket offset values
DETAIL: Time origin of "public.cagg_1_week_non_null_offset" [@ 35 mins] and "public.cagg_1_hour_null_offset" [NULL] should be the same.
\set VERBOSITY terse
-- Different integer offset
CREATE MATERIALIZED VIEW cagg_int_offset_5
WITH (timescaledb.continuous, timescaledb.materialized_only=false)
AS SELECT time_bucket('10', time, "offset"=>5) AS time, SUM(data) AS value
FROM table_int
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:896: NOTICE: refreshing continuous aggregate "cagg_int_offset_5"
psql:include/cagg_query_common.sql:918: NOTICE: refreshing continuous aggregate "cagg_int_offset_5"
CREATE MATERIALIZED VIEW cagg_int_offset_10
WITH (timescaledb.continuous, timescaledb.materialized_only=false)
AS SELECT time_bucket('10', time, "offset"=>10) AS time, SUM(value) AS value
FROM cagg_int_offset_5
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:902: ERROR: cannot create continuous aggregate with different bucket offset values
psql:include/cagg_query_common.sql:924: ERROR: cannot create continuous aggregate with different bucket offset values
\set ON_ERROR_STOP 1
DROP MATERIALIZED VIEW cagg_1_hour_origin;
psql:include/cagg_query_common.sql:906: NOTICE: drop cascades to 2 other objects
psql:include/cagg_query_common.sql:928: NOTICE: drop cascades to 2 other objects
DROP MATERIALIZED VIEW cagg_1_hour_offset;
psql:include/cagg_query_common.sql:907: NOTICE: drop cascades to 2 other objects
psql:include/cagg_query_common.sql:929: NOTICE: drop cascades to 2 other objects
DROP MATERIALIZED VIEW cagg_int_offset_5;
psql:include/cagg_query_common.sql:908: NOTICE: drop cascades to 3 other objects
psql:include/cagg_query_common.sql:930: NOTICE: drop cascades to 3 other objects
---
-- CAGGs on CAGGs tests
---
Expand All @@ -2394,7 +2419,7 @@ CREATE MATERIALIZED VIEW cagg_1_hour_offset
SELECT time_bucket('1 hour', time, origin=>'2000-01-02 01:00:00 PST'::timestamptz) AS hour_bucket, max(value) AS max_value
FROM temperature
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:917: NOTICE: refreshing continuous aggregate "cagg_1_hour_offset"
psql:include/cagg_query_common.sql:939: NOTICE: refreshing continuous aggregate "cagg_1_hour_offset"
SELECT * FROM caggs_info WHERE user_view_name = 'cagg_1_hour_offset';
user_view_schema | user_view_name | bucket_func | bucket_width | bucket_origin | bucket_offset | bucket_timezone | bucket_fixed_width
------------------+--------------------+--------------------------------------------------------------------------------+--------------+------------------------------+---------------+-----------------+--------------------
Expand All @@ -2406,7 +2431,7 @@ CREATE MATERIALIZED VIEW cagg_1_week_offset
SELECT time_bucket('1 week', hour_bucket, origin=>'2000-01-02 01:00:00 PST'::timestamptz) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:924: NOTICE: refreshing continuous aggregate "cagg_1_week_offset"
psql:include/cagg_query_common.sql:946: NOTICE: refreshing continuous aggregate "cagg_1_week_offset"
SELECT * FROM caggs_info WHERE user_view_name = 'cagg_1_week_offset';
user_view_schema | user_view_name | bucket_func | bucket_width | bucket_origin | bucket_offset | bucket_timezone | bucket_fixed_width
------------------+--------------------+--------------------------------------------------------------------------------+--------------+------------------------------+---------------+-----------------+--------------------
Expand Down
39 changes: 32 additions & 7 deletions tsl/test/expected/cagg_query_using_merge.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2368,26 +2368,51 @@ CREATE MATERIALIZED VIEW cagg_1_week_offset
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:889: ERROR: cannot create continuous aggregate with different bucket offset values
-- Cagg with NULL offset on top of cagg with non-NULL offset
\set VERBOSITY default
CREATE MATERIALIZED VIEW cagg_1_week_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>NULL::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:897: ERROR: cannot create continuous aggregate with different bucket offset values
DETAIL: Time origin of "public.cagg_1_week_null_offset" [NULL] and "public.cagg_1_hour_offset" [@ 30 mins] should be the same.
-- Cagg with non-NULL offset on top of cagg with NULL offset
CREATE MATERIALIZED VIEW cagg_1_hour_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 hour', time, "offset"=>NULL::interval) AS hour_bucket, max(value) AS max_value
FROM temperature
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:904: NOTICE: refreshing continuous aggregate "cagg_1_hour_null_offset"
HINT: Use WITH NO DATA if you do not want to refresh the continuous aggregate on creation.
CREATE MATERIALIZED VIEW cagg_1_week_non_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>'35m'::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_null_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:910: ERROR: cannot create continuous aggregate with different bucket offset values
DETAIL: Time origin of "public.cagg_1_week_non_null_offset" [@ 35 mins] and "public.cagg_1_hour_null_offset" [NULL] should be the same.
\set VERBOSITY terse
-- Different integer offset
CREATE MATERIALIZED VIEW cagg_int_offset_5
WITH (timescaledb.continuous, timescaledb.materialized_only=false)
AS SELECT time_bucket('10', time, "offset"=>5) AS time, SUM(data) AS value
FROM table_int
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:896: NOTICE: refreshing continuous aggregate "cagg_int_offset_5"
psql:include/cagg_query_common.sql:918: NOTICE: refreshing continuous aggregate "cagg_int_offset_5"
CREATE MATERIALIZED VIEW cagg_int_offset_10
WITH (timescaledb.continuous, timescaledb.materialized_only=false)
AS SELECT time_bucket('10', time, "offset"=>10) AS time, SUM(value) AS value
FROM cagg_int_offset_5
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:902: ERROR: cannot create continuous aggregate with different bucket offset values
psql:include/cagg_query_common.sql:924: ERROR: cannot create continuous aggregate with different bucket offset values
\set ON_ERROR_STOP 1
DROP MATERIALIZED VIEW cagg_1_hour_origin;
psql:include/cagg_query_common.sql:906: NOTICE: drop cascades to 2 other objects
psql:include/cagg_query_common.sql:928: NOTICE: drop cascades to 2 other objects
DROP MATERIALIZED VIEW cagg_1_hour_offset;
psql:include/cagg_query_common.sql:907: NOTICE: drop cascades to 2 other objects
psql:include/cagg_query_common.sql:929: NOTICE: drop cascades to 2 other objects
DROP MATERIALIZED VIEW cagg_int_offset_5;
psql:include/cagg_query_common.sql:908: NOTICE: drop cascades to 3 other objects
psql:include/cagg_query_common.sql:930: NOTICE: drop cascades to 3 other objects
---
-- CAGGs on CAGGs tests
---
Expand All @@ -2396,7 +2421,7 @@ CREATE MATERIALIZED VIEW cagg_1_hour_offset
SELECT time_bucket('1 hour', time, origin=>'2000-01-02 01:00:00 PST'::timestamptz) AS hour_bucket, max(value) AS max_value
FROM temperature
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:917: NOTICE: refreshing continuous aggregate "cagg_1_hour_offset"
psql:include/cagg_query_common.sql:939: NOTICE: refreshing continuous aggregate "cagg_1_hour_offset"
SELECT * FROM caggs_info WHERE user_view_name = 'cagg_1_hour_offset';
user_view_schema | user_view_name | bucket_func | bucket_width | bucket_origin | bucket_offset | bucket_timezone | bucket_fixed_width
------------------+--------------------+--------------------------------------------------------------------------------+--------------+------------------------------+---------------+-----------------+--------------------
Expand All @@ -2408,7 +2433,7 @@ CREATE MATERIALIZED VIEW cagg_1_week_offset
SELECT time_bucket('1 week', hour_bucket, origin=>'2000-01-02 01:00:00 PST'::timestamptz) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;
psql:include/cagg_query_common.sql:924: NOTICE: refreshing continuous aggregate "cagg_1_week_offset"
psql:include/cagg_query_common.sql:946: NOTICE: refreshing continuous aggregate "cagg_1_week_offset"
SELECT * FROM caggs_info WHERE user_view_name = 'cagg_1_week_offset';
user_view_schema | user_view_name | bucket_func | bucket_width | bucket_origin | bucket_offset | bucket_timezone | bucket_fixed_width
------------------+--------------------+--------------------------------------------------------------------------------+--------------+------------------------------+---------------+-----------------+--------------------
Expand Down
22 changes: 22 additions & 0 deletions tsl/test/sql/include/cagg_query_common.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,28 @@ CREATE MATERIALIZED VIEW cagg_1_week_offset
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;

-- Cagg with NULL offset on top of cagg with non-NULL offset
\set VERBOSITY default
CREATE MATERIALIZED VIEW cagg_1_week_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>NULL::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_offset
GROUP BY 1 ORDER BY 1;

-- Cagg with non-NULL offset on top of cagg with NULL offset
CREATE MATERIALIZED VIEW cagg_1_hour_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 hour', time, "offset"=>NULL::interval) AS hour_bucket, max(value) AS max_value
FROM temperature
GROUP BY 1 ORDER BY 1;

CREATE MATERIALIZED VIEW cagg_1_week_non_null_offset
WITH (timescaledb.continuous) AS
SELECT time_bucket('1 week', hour_bucket, "offset"=>'35m'::interval) AS week_bucket, max(max_value) AS max_value
FROM cagg_1_hour_null_offset
GROUP BY 1 ORDER BY 1;
\set VERBOSITY terse

-- Different integer offset
CREATE MATERIALIZED VIEW cagg_int_offset_5
WITH (timescaledb.continuous, timescaledb.materialized_only=false)
Expand Down
Loading