Skip to content

Commit

Permalink
Fix CAgg on CAgg variable bucket size validation
Browse files Browse the repository at this point in the history
Previous attempt to fix it (PR #5130) was not entirely correct because
the bucket width calculation for interval width was wrong.

Fixed it by properly calculate the bucket width for intervals using the
Postgres internal function `interval_part` to extract the epoch of the
interval and execute the validations. For integer widths use the already
calculated bucket width.

Fixes #5158, #5168
  • Loading branch information
fabriziomello committed Jan 13, 2023
1 parent ca9d508 commit 8f0786a
Show file tree
Hide file tree
Showing 7 changed files with 1,705 additions and 211 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ accidentally triggering the load of a previous DB version.**
**Bugfixes**
* #4926 Fix corruption when inserting into compressed chunks
* #5114 Fix issue with deleting data node and dropping database
* #5130 Fix CAgg on CAgg variable bucket size validation
* #5133 Fix CAgg on CAgg using different column order on the original hypertable
* #5152 Fix adding column with NULL constraint to compressed hypertable
* #5170 Fix CAgg on CAgg variable bucket size validation

**Thanks**
* @ikkala for reporting error when adding column with NULL constraint to compressed hypertable
* @salquier-appvizer for reporting error on CAgg on CAgg using different column order on the original hypertable
* @ssmoss for reporting error on CAgg on CAgg variable bucket size validation
* @ssmoss, @adbnexxtlab and @ivanzamanov for reporting error on CAgg on CAgg variable bucket size validation

## 2.9.1 (2022-12-23)

Expand Down
67 changes: 51 additions & 16 deletions tsl/src/continuous_aggs/create.c
Original file line number Diff line number Diff line change
Expand Up @@ -1099,24 +1099,53 @@ cagg_query_supported(const Query *query, StringInfo hint, StringInfo detail, con
static inline int64
get_bucket_width(CAggTimebucketInfo bucket_info)
{
if (bucket_info.bucket_width == BUCKET_WIDTH_VARIABLE)
int64 width = 0;

/* calculate the width */
switch (bucket_info.bucket_width_type)
{
/* calculate the width */
return ts_interval_value_to_internal(IntervalPGetDatum(bucket_info.interval),
bucket_info.bucket_width_type);
case INT8OID:
case INT4OID:
case INT2OID:
width = bucket_info.bucket_width;
break;
case INTERVALOID:
{
Datum epoch = DirectFunctionCall2(interval_part,
PointerGetDatum(cstring_to_text("epoch")),
IntervalPGetDatum(bucket_info.interval));
/* cast float8 to int8 */
width = DatumGetInt64(DirectFunctionCall1(dtoi8, epoch));
break;
}
default:
Assert(false);
}

return bucket_info.bucket_width;
return width;
}

static inline Datum
get_bucket_width_datum(CAggTimebucketInfo bucket_info)
{
if (bucket_info.bucket_width == BUCKET_WIDTH_VARIABLE)
return IntervalPGetDatum(bucket_info.interval);
Datum width = (Datum) 0;

return ts_internal_to_interval_value(get_bucket_width(bucket_info),
bucket_info.bucket_width_type);
switch (bucket_info.bucket_width_type)
{
case INT8OID:
case INT4OID:
case INT2OID:
width = ts_internal_to_interval_value(bucket_info.bucket_width,
bucket_info.bucket_width_type);
break;
case INTERVALOID:
width = IntervalPGetDatum(bucket_info.interval);
break;
default:
Assert(false);
}

return width;
}

static CAggTimebucketInfo
Expand Down Expand Up @@ -1320,8 +1349,8 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
/* nested cagg validations */
if (is_nested)
{
int64 bucket_width, bucket_width_parent;
bool is_greater_or_equal_than_parent, is_multiple_of_parent;
int64 bucket_width = 0, bucket_width_parent = 0;
bool is_greater_or_equal_than_parent = true, is_multiple_of_parent = true;

Assert(prev_query->groupClause);
caggtimebucket_validate(&bucket_info_parent,
Expand Down Expand Up @@ -1354,7 +1383,13 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
is_greater_or_equal_than_parent = (bucket_width >= bucket_width_parent);

/* check if buckets are multiple */
is_multiple_of_parent = ((bucket_width % bucket_width_parent) == 0);
if (bucket_width_parent != 0)
{
if (bucket_width_parent > bucket_width && bucket_width != 0)
is_multiple_of_parent = ((bucket_width_parent % bucket_width) == 0);
else
is_multiple_of_parent = ((bucket_width % bucket_width_parent) == 0);
}

/* proceed with validation errors */
if (!is_greater_or_equal_than_parent || !is_multiple_of_parent)
Expand All @@ -1373,14 +1408,14 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
width_parent = get_bucket_width_datum(bucket_info_parent);
width_out_parent = DatumGetCString(OidFunctionCall1(outfuncid, width_parent));

/* new bucket should be greater than the parent */
if (!is_greater_or_equal_than_parent)
message = "greater than";

/* new bucket should be multiple of the parent */
if (!is_multiple_of_parent)
message = "multiple of";

/* new bucket should be greater than the parent */
if (!is_greater_or_equal_than_parent)
message = "greater or equal than";

ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot create continuous aggregate with incompatible bucket width"),
Expand Down

0 comments on commit 8f0786a

Please sign in to comment.