Changing syntax to use MATERIALIZED VIEW for continuous aggregates #2243

mkindahl · 2020-08-21T13:31:05Z

We change the syntax for defining continuous aggregates to use CREATE MATERIALIZED VIEW rather than CREATE VIEW. The command still creates
a view, while CREATE MATERIALIZED VIEW creates a table. Raise an
error if CREATE VIEW is used to create a continuous aggregate and
redirect to CREATE MATERIALIZED VIEW.

In a similar vein, DROP MATERIALIZED VIEW is used for continuous
aggregates and continuous aggregates cannot be dropped with DROP VIEW.

Continuous aggregates are altered using ALTER MATERIALIZED VIEW
rather than ALTER VIEW, so we ensure that it works for ALTER MATERIALIZED VIEW and gives an error if you try to use ALTER VIEW to
change a continuous aggregate.

Note that we allow ALTER VIEW ... SET SCHEMA to be used with the
partial view as well as with the direct view, so this is handled as a
special case.

Fixes #2233

erimatnor

Did a quick pass with high-level feedback. Might do a more thorough review later.

src/continuous_agg.c

src/continuous_agg.h

src/process_utility.c

test/sql/updates/setup.continuous_aggs.v2.sql

tsl/src/continuous_aggs/create.c

src/process_utility.c

gayyappan · 2020-08-21T15:26:52Z

src/process_utility.c

+	cagg = ts_continuous_agg_find_by_view_name(NameStr(view_schema), NameStr(view_name));
+	vtyp = ts_continuous_agg_view_type(&cagg->data, NameStr(view_schema), NameStr(view_name));
+
+	if (vtyp == ContinuousAggPartialView || vtyp == ContinuousAggDirectView)


The commit message has a typo: it states that ALTER VIEW is allowed with partial and direct views.

Well, it is allowed to use ALTER VIEW to change some things, such as the schema for the view. Not sure why, but there are tests for this.

I'll clarify the situation when squashing them.

Rewrote it to mention that it is ALTER VIEW ... SET SCHEMA that we handle.

erimatnor

Approving, but there are a number of nits, issues to fix. Most pressing are several places where the return type and return values do not match.

I also think there's a test case missing for CREATE MATERIALIZED VIEW IF NOT EXISTS

Please also squash a number of the nit commits (7 commits seems a bit excessive for this change).

src/process_utility.c

tsl/src/continuous_aggs/create.c

erimatnor · 2020-08-26T08:21:34Z

tsl/src/continuous_aggs/create.c

-	if (get_relname_relid(stmt->view->relname, nspid))
+	nspid = RangeVarGetCreationNamespace(stmt->into->rel);
+
+	if (get_relname_relid(stmt->into->rel->relname, nspid))


Shouldn't this honor IF NOT EXISTS? (Looks like we are always failing here in case of an existing aggregate). Do we have a test for it?

Added check for it working in a similar way to how it works for normal tables. And added test case.

tsl/src/continuous_aggs/create.c

codecov · 2020-08-26T13:56:58Z

Codecov Report

Merging #2243 into master will increase coverage by 0.13%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #2243      +/-   ##
==========================================
+ Coverage   90.11%   90.24%   +0.13%     
==========================================
  Files         212      212              
  Lines       34347    34341       -6     
==========================================
+ Hits        30952    30992      +40     
+ Misses       3395     3349      -46

Impacted Files	Coverage Δ
src/cross_module_fn.c	`57.48% <0.00%> (ø)`
src/hypertable.c	`88.61% <ø> (ø)`
src/continuous_agg.c	`93.33% <85.71%> (-0.29%)`	⬇️
tsl/src/continuous_aggs/create.c	`95.80% <93.33%> (+<0.01%)`	⬆️
src/process_utility.c	`93.67% <95.58%> (-0.01%)`	⬇️
src/import/planner.c	`70.30% <0.00%> (+11.12%)`	⬆️

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 ba93cbf...466e236. Read the comment docs.

gayyappan

There are some unexpected diffs in the isolation test output. Other changes look good.

gayyappan · 2020-08-26T19:42:37Z

tsl/test/isolation/expected/continuous_aggs_multi.out

@@ -94,7 +94,8 @@ step Setup2:
    END; $$ LANGUAGE plpgsql;

 step AlterLag1: alter view continuous_view_1 set (timescaledb.refresh_lag = 10);
-R1: LOG:  materializing continuous aggregate public.continuous_view_1: nothing to invalidate, new range up to 15
+ERROR:  cannot alter continuous aggregate using ALTER VIEW


This error needs to be fixed. It alters the output for the .out file.

gayyappan · 2020-08-26T19:43:22Z

tsl/test/isolation/expected/continuous_aggs_multi.out

@@ -119,11 +120,12 @@ step Refresh2: REFRESH MATERIALIZED VIEW continuous_view_2; <waiting ...>
 step UnlockCompleted: ROLLBACK;
 step Refresh2: <... completed>
 step UnlockMat1: ROLLBACK;
-R1: LOG:  materializing continuous aggregate public.continuous_view_1: nothing to invalidate, new range up to 30
+R1: LOG:  materializing continuous aggregate public.continuous_view_1: nothing to invalidate, new range up to 45


Is this diff also related to the alter view fialing?

Yup. Fixed.

We change the syntax for defining continuous aggregates to use `CREATE MATERIALIZED VIEW` rather than `CREATE VIEW`. The command still creates a view, while `CREATE MATERIALIZED VIEW` creates a table. Raise an error if `CREATE VIEW` is used to create a continuous aggregate and redirect to `CREATE MATERIALIZED VIEW`. In a similar vein, `DROP MATERIALIZED VIEW` is used for continuous aggregates and continuous aggregates cannot be dropped with `DROP VIEW`. Continuous aggregates are altered using `ALTER MATERIALIZED VIEW` rather than `ALTER VIEW`, so we ensure that it works for `ALTER MATERIALIZED VIEW` and gives an error if you try to use `ALTER VIEW` to change a continuous aggregate. Note that we allow `ALTER VIEW ... SET SCHEMA` to be used with the partial view as well as with the direct view, so this is handled as a special case. Fixes timescale#2233 Co-authored-by: =?UTF-8?q?Erik=20Nordstr=C3=B6m?= <erik@timescale.com> Co-authored-by: Mats Kindahl <mats@timescale.com>

mkindahl requested a review from a team as a code owner August 21, 2020 13:31

mkindahl requested review from mfreed, svenklemm, gayyappan and erimatnor and removed request for a team and mfreed August 21, 2020 13:31

mkindahl self-assigned this Aug 21, 2020

mkindahl added the continuous_aggregate label Aug 21, 2020

erimatnor reviewed Aug 21, 2020

View reviewed changes

src/continuous_agg.c Outdated Show resolved Hide resolved

src/continuous_agg.c Outdated Show resolved Hide resolved

src/continuous_agg.h Outdated Show resolved Hide resolved

src/process_utility.c Show resolved Hide resolved

erimatnor added this to the 2.0.0 milestone Aug 21, 2020

gayyappan reviewed Aug 21, 2020

View reviewed changes

test/sql/updates/setup.continuous_aggs.v2.sql Outdated Show resolved Hide resolved

gayyappan reviewed Aug 21, 2020

View reviewed changes

tsl/src/continuous_aggs/create.c Outdated Show resolved Hide resolved

gayyappan reviewed Aug 21, 2020

View reviewed changes

src/process_utility.c Outdated Show resolved Hide resolved

gayyappan reviewed Aug 21, 2020

View reviewed changes

mkindahl force-pushed the create_mat_view branch 3 times, most recently from 67dd652 to eee6b85 Compare August 24, 2020 11:11

mkindahl requested review from erimatnor and gayyappan August 24, 2020 11:11

mkindahl force-pushed the create_mat_view branch 4 times, most recently from fe989bd to 2de3f06 Compare August 25, 2020 14:34

erimatnor approved these changes Aug 26, 2020

View reviewed changes

mkindahl force-pushed the create_mat_view branch 5 times, most recently from 1707c17 to f63fbf8 Compare August 26, 2020 13:17

mkindahl force-pushed the create_mat_view branch from f63fbf8 to 94aa738 Compare August 26, 2020 13:28

gayyappan reviewed Aug 26, 2020

View reviewed changes

mkindahl force-pushed the create_mat_view branch 2 times, most recently from fd39737 to f80b5ed Compare August 27, 2020 10:49

mkindahl requested a review from gayyappan August 27, 2020 10:50

mkindahl force-pushed the create_mat_view branch 6 times, most recently from db6946c to 9c8f941 Compare August 27, 2020 14:12

mkindahl force-pushed the create_mat_view branch from 9c8f941 to 466e236 Compare August 27, 2020 14:25

gayyappan approved these changes Aug 27, 2020

View reviewed changes

mkindahl merged commit c054b38 into timescale:master Aug 27, 2020

mkindahl deleted the create_mat_view branch August 27, 2020 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing syntax to use MATERIALIZED VIEW for continuous aggregates #2243

Changing syntax to use MATERIALIZED VIEW for continuous aggregates #2243

mkindahl commented Aug 21, 2020 •

edited

erimatnor left a comment

gayyappan Aug 21, 2020

mkindahl Aug 24, 2020

mkindahl Aug 24, 2020

mkindahl Aug 26, 2020

erimatnor left a comment •

edited

erimatnor Aug 26, 2020

mkindahl Aug 26, 2020

codecov bot commented Aug 26, 2020 •

edited

gayyappan left a comment

gayyappan Aug 26, 2020

mkindahl Aug 27, 2020

gayyappan Aug 26, 2020

mkindahl Aug 27, 2020

Changing syntax to use MATERIALIZED VIEW for continuous aggregates #2243

Changing syntax to use MATERIALIZED VIEW for continuous aggregates #2243

Conversation

mkindahl commented Aug 21, 2020 • edited

erimatnor 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

erimatnor left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2020 • edited

Codecov Report

gayyappan 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

mkindahl commented Aug 21, 2020 •

edited

erimatnor left a comment •

edited

codecov bot commented Aug 26, 2020 •

edited