Avoid having to cast time arg for cagg policy #2387

pmwkaa · 2020-09-14T12:40:39Z

This patch does a minor refactoring and adds a way to guess
interval argument type based on used cagg

codecov · 2020-09-14T12:54:41Z

Codecov Report

Merging #2387 into master will increase coverage by 0.14%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #2387      +/-   ##
==========================================
+ Coverage   89.95%   90.10%   +0.14%     
==========================================
  Files         213      213              
  Lines       34354    34326      -28     
==========================================
+ Hits        30904    30928      +24     
+ Misses       3450     3398      -52

Impacted Files	Coverage Δ
src/catalog.h	`100.00% <ø> (ø)`
src/chunk_index.h	`100.00% <ø> (ø)`
src/dimension.c	`95.41% <ø> (ø)`
src/utils.c	`80.05% <ø> (ø)`
src/utils.h	`44.44% <ø> (ø)`
src/time_utils.c	`98.12% <88.88%> (+0.04%)`	⬆️
tsl/src/bgw_policy/continuous_aggregate_api.c	`92.19% <96.42%> (+0.28%)`	⬆️
src/bgw/job.c	`95.07% <96.96%> (+0.28%)`	⬆️
src/chunk_index.c	`94.72% <100.00%> (-0.03%)`	⬇️
tsl/src/fdw/deparse.c	`63.34% <100.00%> (-0.12%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1a00eb...14448fb. Read the comment docs.

erimatnor · 2020-09-14T14:45:57Z

tsl/src/bgw_policy/continuous_aggregate_api.c

-static void
-check_valid_interval(Oid dim_type, Oid interval_type, const char *str_msg)
+static Datum
+convert_interval_arg(Oid dim_type, Datum interval, Oid *interval_type, const char *str_msg)


Isn't this conversion useful for other policies as well (e.g., retention)? I figured this would be in time_utils or policy_utils for use by all policies.

Yes, I thought maybe to do that in another PR related to the retention policy.

erimatnor · 2020-09-18T08:09:23Z

tsl/test/sql/continuous_aggs_policy.sql

 \set ON_ERROR_STOP 1
-SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 day'::interval, '1 day'::interval , '1 day'::interval) as job_id \gset
+SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 day', '1 day', '1 day'::interval) as job_id \gset


Nit: Would be nice to get rid of the cast on schedule_interval too. Perhaps also for a separate PR?

Suggested change

SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 day', '1 day', '1 day'::interval) as job_id \gset

SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 days', '1 day', '1 day'::interval) as job_id \gset

k-rus

LGTM
It might be useful to give short comment describing the function.
And I am not favour of reusing variable names, especially it is set to a different value.

k-rus · 2020-09-18T12:35:26Z

src/time_utils.c

@@ -41,6 +41,45 @@ subtract_interval_from_now(Oid timetype, const Interval *interval)
 	return res;
 }

+Datum


I think it is worth do document this function as it is part of internal API, so it is clear what to expect from it and how to use it. Currently I cannot guess from the function name what to expect from it.

k-rus · 2020-09-18T12:44:13Z

src/time_utils.c

+		{
+			case 1:
+				/* Functions that take one input argument, e.g., the Date function */
+				arg = OidFunctionCall1(infuncid, arg);


Am I correct that this is a reuse of variable arg, which first contains the original argument, and then assigned the converted argument? I suggest to use different variable and actually it will contain different object to my understanding.

k-rus · 2020-09-18T12:47:02Z

src/time_utils.c

-	}
+	/* If no explicit cast was done by the user, try to convert the argument
+	 * to the time type used by the continuous aggregate. */
+	arg = ts_time_datum_convert_arg(arg, &argtype, timetype);


Same comment here, since arg will contain different object after the call. To avoid code changes, the original variable can be called arg_in.

k-rus · 2020-09-18T12:53:38Z

src/time_utils.c

+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("invalid time argument"),
+						 errhint("Time argument requires an explicit cast.")));


Is it possible to test this case? Codecov complains that it's not covered.

I think most of those cases already covered by continuous agg refresh function tests, which are reusing the same functionality.

I guess Codecov would not complain if it is covered ;) I searched that such error is never expected in our tests.

I don't think this case is actually possible right now, it was made more like a precaution in case new time functions will arrive in future versions I believe

This patch does a minor refactoring and adds a way to guess interval argument type based on used cagg Issue: timescale#2286

pmwkaa requested a review from a team as a code owner September 14, 2020 12:40

pmwkaa requested review from k-rus, WireBaron and svenklemm and removed request for a team September 14, 2020 12:40

erimatnor reviewed Sep 14, 2020

View reviewed changes

pmwkaa marked this pull request as draft September 15, 2020 12:31

pmwkaa force-pushed the fix_time_arg_cast branch from 4cfcb60 to fb4974a Compare September 16, 2020 12:33

pmwkaa marked this pull request as ready for review September 16, 2020 12:36

pmwkaa requested a review from a team September 16, 2020 12:36

erimatnor approved these changes Sep 18, 2020

View reviewed changes

erimatnor added the bgw The background worker subsystem, including the scheduler label Sep 18, 2020

pmwkaa force-pushed the fix_time_arg_cast branch from fb4974a to 122853d Compare September 18, 2020 10:27

k-rus approved these changes Sep 18, 2020

View reviewed changes

pmwkaa force-pushed the fix_time_arg_cast branch from 122853d to b5615ca Compare September 21, 2020 11:28

Avoid having to cast time arg for cagg policy

14448fb

This patch does a minor refactoring and adds a way to guess interval argument type based on used cagg Issue: timescale#2286

pmwkaa force-pushed the fix_time_arg_cast branch from b5615ca to 14448fb Compare September 21, 2020 11:29

pmwkaa merged commit c15d8be into timescale:master Sep 21, 2020

erimatnor mentioned this pull request Sep 22, 2020

Avoid having to cast time args in policies #2286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid having to cast time arg for cagg policy #2387

Avoid having to cast time arg for cagg policy #2387

pmwkaa commented Sep 14, 2020 •

edited

codecov bot commented Sep 14, 2020 •

edited

erimatnor Sep 14, 2020

pmwkaa Sep 16, 2020

erimatnor Sep 18, 2020

k-rus left a comment

k-rus Sep 18, 2020

k-rus Sep 18, 2020

k-rus Sep 18, 2020

k-rus Sep 18, 2020

pmwkaa Sep 18, 2020 •

edited

k-rus Sep 18, 2020 •

edited

pmwkaa Sep 21, 2020

	SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 day', '1 day', '1 day'::interval) as job_id \gset
	SELECT add_continuous_aggregate_policy('max_mat_view_date', '2 days', '1 day', '1 day'::interval) as job_id \gset

Avoid having to cast time arg for cagg policy #2387

Avoid having to cast time arg for cagg policy #2387

Conversation

pmwkaa commented Sep 14, 2020 • edited

codecov bot commented Sep 14, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-rus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmwkaa Sep 18, 2020 • edited

Choose a reason for hiding this comment

k-rus Sep 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmwkaa commented Sep 14, 2020 •

edited

codecov bot commented Sep 14, 2020 •

edited

pmwkaa Sep 18, 2020 •

edited

k-rus Sep 18, 2020 •

edited