Rectify interval calculation

[RafiaSabih]

For Hierarchical continuous aggregates with variable bucket size, the interval was wrongly manipulated in the process. It was introduced by commit 5b60343 .

Now it is corrected by creating a copy of interval structure for validation purposes and keeping the original structure untouched.

Fixes #5734

[github-actions]

@svenklemm, @shhnwz: please review this pull request.

Powered by pull-review

[codecov]

Codecov Report

Merging #5860 (605ccea) into main (88aaf23) will decrease coverage by 0.05%.
The diff coverage is 87.50%.

❗ Current head 605ccea differs from pull request most recent head 01139cb. Consider uploading reports for the commit 01139cb to get more accurate results

@@            Coverage Diff             @@
##             main    #5860      +/-   ##
==========================================
- Coverage   87.86%   87.81%   -0.05%     
==========================================
  Files         239      239              
  Lines       55792    55789       -3     
  Branches    12359    12357       -2     
==========================================
- Hits        49020    48992      -28     
- Misses       4864     4908      +44     
+ Partials     1908     1889      -19     

Impacted Files	Coverage Δ
tsl/src/continuous_aggs/common.c	90.76% <87.50%> (+0.02%)	⬆️

... and 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[fabriziomello]

Suggested change

	\ir setup.repair.heirarchical_cagg.sql
	\ir setup.repair.hierarchical_cagg.sql

[fabriziomello]

Would be also good to mention in the PR description and commit message the previous PR that introduced the buggy code: #5255

[fabriziomello]

This test not FAIL without the fix... the idea of adding a regression test is to exercise the failing code path.

[RafiaSabih]

You are right! I have now updated the test which was failing without the fix.

[pmwkaa]

Also I've noticed that we probably have a bug in get_bucket_width() and get_bucket_width_datum() functions, something to investigate.

static Datum
get_bucket_width_datum(CAggTimebucketInfo bucket_info)

Seems like bucket_info is not a pointer, which feels like a bug to me since this is a big structure and probably misuse.

[pmwkaa]

nit: probably could do just Interval interval = *bucket_info.interval.

I am more curious why bucket_info is not a pointer, could be a bug

[fabriziomello]

nit: probably could do just Interval interval = *bucket_info.interval.

Your suggestion is good...

I am more curious why bucket_info is not a pointer, could be a bug

So the idea here is not change the bucket_info inside the get_bucket_width (it was the root cause of this issue). In any case probably you're right about it be a pointer but I prefer to don't mess to much with this ugly code right now because we'll do a major refactoring on it in this quarter to remove the old format, then we can address those issues.

[pmwkaa]

Ok, at least this is by design.

nit: there are potential performance impact if those functions are executed frequently, because you will have to copy arguments to each call

[fabriziomello]

In this case performance is not a big problem because this code is executed once when create a continuous aggregate on top of another (aka hierarchical) so unless a "crazy" user decide to create "gazillions" of hierarchical caggs concurrently we are fine here. ;-)

[fabriziomello]

Suggested change

	Interval interval = { .time = bucket_info.interval->time,
	.day = bucket_info.interval->day,
	.month = bucket_info.interval->month };
	Interval interval = *bucket_info.interval;

As per @pmwkaa suggested.

[fabriziomello]

I guess this new code block should be moved to after the \set VERBOSITY terse below, no?

[fabriziomello]

Suggested change

	--There should never be dulpicates in the output of the following query
	SELECT * from :CAGG_NAME_3TH_LEVEL;
	-- There should never be duplicated records in the output of the following query
	SELECT * from :CAGG_NAME_3TH_LEVEL ORDER BY 1;

[fabriziomello]

Suggested change

	SELECT count(*) FROM agg_test_monthly;
	-- There should never be duplicated records in the output of the following query
	SELECT * FROM agg_test_monthly ORDER BY 1;

[fabriziomello]

Left some minor comments but approved anyway.

[timescale-automation]

Automated backport to 2.11.x not done: cherry-pick failed.

Git status

HEAD detached at origin/2.11.x
You are currently cherry-picking commit 1bd527375.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   .unreleased/bugfix_5734
	new file:   test/sql/updates/post.repair.hierarchical_cagg.sql
	modified:   test/sql/updates/post.repair.sql
	new file:   test/sql/updates/setup.repair.hierarchical_cagg.sql
	modified:   test/sql/updates/setup.repair.sql
	modified:   tsl/test/expected/cagg_on_cagg.out
	modified:   tsl/test/expected/cagg_on_cagg_dist_ht.out
	modified:   tsl/test/expected/cagg_on_cagg_joins.out
	modified:   tsl/test/expected/cagg_on_cagg_joins_dist_ht.out
	modified:   tsl/test/sql/cagg_on_cagg.sql
	modified:   tsl/test/sql/cagg_on_cagg_dist_ht.sql
	modified:   tsl/test/sql/cagg_on_cagg_joins.sql
	modified:   tsl/test/sql/cagg_on_cagg_joins_dist_ht.sql
	modified:   tsl/test/sql/include/cagg_on_cagg_setup.sql
	modified:   tsl/test/sql/include/cagg_on_cagg_validations.sql

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   tsl/src/continuous_aggs/common.c

Job log