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

Remove requirement of CASCADE from DROP VIEW #2171

Merged
merged 1 commit into from Aug 3, 2020
Merged

Remove requirement of CASCADE from DROP VIEW #2171

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 15 additions & 10 deletions src/process_utility.c
View file
Open in desktop
Expand Up @@ -1079,9 +1079,8 @@ process_grant_and_revoke_role(ProcessUtilityArgs *args)
return DDL_DONE;
}

/* Force the use of CASCADE to drop continuous aggregates */
static void
block_dropping_continuous_aggregates_without_cascade(ProcessUtilityArgs *args, DropStmt *stmt)
process_drop_continuous_aggregates(ProcessUtilityArgs *args, DropStmt *stmt)
{
ListCell *lc;

Expand All @@ -1108,13 +1107,19 @@ block_dropping_continuous_aggregates_without_cascade(ProcessUtilityArgs *args, D
name = get_rel_name(relid);

cagg = ts_continuous_agg_find_by_view_name(schema, name);
if (cagg == NULL)
continue;

if (ts_continuous_agg_view_type(&cagg->data, schema, name) == ContinuousAggUserView)
ereport(ERROR,
(errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
errmsg("dropping a continuous aggregate requires using CASCADE")));
if (cagg)
{
/* Add the materialization table to the arguments so that the
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see a test for this case. It suggest adding one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed, I'll just remove the warning and update the comment to clarify the situation.

* continuous aggregate and associated materialization table is
* dropped together.
*
* If the table is missing, something is wrong, but we proceed
* with dropping the view anyway since the user cannot get rid of
* the broken view if we generate an error. */
Hypertable *ht = ts_hypertable_get_by_id(cagg->data.mat_hypertable_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

We are making an assumption here that the only dependent object on the cagg is the materialization hypertable. What happens in the case where we have a view defined on top of the cagg like: CREATE VIEW joinview as select * from cagg, labels_tab where cagg. = ... . The CASCADE requirement can be dropped only if the dependent objects are internally created objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow; this code looks up the materialized hypertable and adds it to the list of tables to be dropped. It make no assumptions on other objects and, in particular, does not allow dropping other (dependent) objects without using CASCADE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification. See that you also have a test case for it.

if (ht)
process_add_hypertable(args, ht);
}
}
}

Expand All @@ -1133,7 +1138,7 @@ process_drop_start(ProcessUtilityArgs *args)
process_drop_hypertable_index(args, stmt);
break;
case OBJECT_VIEW:
block_dropping_continuous_aggregates_without_cascade(args, stmt);
process_drop_continuous_aggregates(args, stmt);
break;
case OBJECT_FOREIGN_SERVER:
process_drop_foreign_server_start(stmt);
Expand Down
2 changes: 1 addition & 1 deletion tsl/test/expected/bgw_reorder_drop_chunks.out
View file
Open in desktop
Expand Up @@ -658,5 +658,5 @@ SELECT show_chunks('test_drop_chunks_table');
(4 rows)

--drop the view to allow drop chunks to work
DROP VIEW tdc_view CASCADE;
DROP VIEW tdc_view;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should keep the cascade notice below if we do not require CASCADE. These notices are typically displayed when there are dependent objects created by the user and CASCADE is required to remove them at the same time.

However, the semantics we want is to treat the DROP as if it was acting on a single abstract object, so it makes no sense to keep the cascade notices about subobjects that are part of the abstract object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed, the message is something that PostgreSQL generates internally and setting up the dependencies turned out to not be trivial. I will skip this issue for this PR and investigate it separately.

Note that PostgreSQL has support for handling internal dependencies and allow objects to be part of an implementation as is the case for continuous aggregates. However, this requires a separate effort.

NOTICE: drop cascades to table _timescaledb_internal._hyper_3_13_chunk
25 changes: 11 additions & 14 deletions tsl/test/expected/continuous_aggs.out
View file
Open in desktop
Expand Up @@ -91,7 +91,7 @@ order by tgname;

SET ROLE :ROLE_DEFAULT_PERM_USER;
-- TEST2 ---
drop view mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to table _timescaledb_internal._hyper_2_2_chunk
SELECT * FROM _timescaledb_config.bgw_job;
id | application_name | job_type | schedule_interval | max_runtime | max_retries | retry_period | proc_name | proc_schema | owner | scheduled | hypertable_id | config
Expand Down Expand Up @@ -241,7 +241,7 @@ order by time_bucket('1week', timec);
-- use previous data from conditions
--drop only the view.
-- apply where clause on result of mat_m1 --
drop view mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to 2 other objects
create or replace view mat_m1( timec, minl, sumth, stddevh)
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
Expand Down Expand Up @@ -295,7 +295,7 @@ order by time_bucket('1week', timec);

-- TEST5 --
---------test with having clause ----------------------
drop view mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to 2 other objects
create or replace view mat_m1( timec, minl, sumth, stddevh)
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
Expand Down Expand Up @@ -370,7 +370,6 @@ insert into conditions
select generate_series('2018-11-01 00:00'::timestamp, '2018-12-31 00:00'::timestamp, '1 day'), 'NYC', 35, 45, 50, 40;
insert into conditions
select generate_series('2018-11-01 00:00'::timestamp, '2018-12-15 00:00'::timestamp, '1 day'), 'LA', 73, 55, 71, 28;
--drop view mat_m1 cascade;
--naming with AS clauses
create or replace view mat_naming
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
Expand Down Expand Up @@ -405,7 +404,7 @@ order by attnum, attname;
8 | chunk_id
(8 rows)

DROP VIEW mat_naming CASCADE;
DROP VIEW mat_naming;
--naming with default names
create or replace view mat_naming
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
Expand Down Expand Up @@ -440,7 +439,7 @@ order by attnum, attname;
8 | chunk_id
(8 rows)

DROP VIEW mat_naming CASCADE;
DROP VIEW mat_naming;
--naming with view col names
create or replace view mat_naming (bucket, loc, sum_t_h, stdd)
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
Expand Down Expand Up @@ -475,7 +474,7 @@ order by attnum, attname;
8 | chunk_id
(8 rows)

DROP VIEW mat_naming CASCADE;
DROP VIEW mat_naming;
create or replace view mat_m1( timec, minl, sumth, stddevh)
WITH (timescaledb.continuous, timescaledb.materialized_only=true)
as
Expand Down Expand Up @@ -561,7 +560,7 @@ insert into :"MAT_SCHEMA_NAME".:"MAT_TABLE_NAME"
select * from :"PART_VIEW_SCHEMA".:"PART_VIEW_NAME";
SET ROLE :ROLE_DEFAULT_PERM_USER;
--lets drop the view and check
drop view mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to 2 other objects
drop table conditions;
CREATE TABLE conditions (
Expand Down Expand Up @@ -636,8 +635,6 @@ DROP VIEW :"PART_VIEW_SCHEMA".:"PART_VIEW_NAME";
ERROR: cannot drop the partial/direct view because it is required by a continuous aggregate
DROP VIEW :"DIR_VIEW_SCHEMA".:"DIR_VIEW_NAME";
ERROR: cannot drop the partial/direct view because it is required by a continuous aggregate
DROP VIEW mat_test;
ERROR: dropping a continuous aggregate requires using CASCADE
\set ON_ERROR_STOP 1
--catalog entry still there;
SELECT count(*)
Expand Down Expand Up @@ -673,7 +670,7 @@ select count(*) from pg_class where relname = 'mat_test';
1
(1 row)

DROP VIEW mat_test CASCADE;
DROP VIEW mat_test;
NOTICE: drop cascades to 2 other objects
--catalog entry should be gone
SELECT count(*)
Expand Down Expand Up @@ -904,7 +901,7 @@ order by indexname;
_materialized_hypertable_20_timec_idx | CREATE INDEX _materialized_hypertable_20_timec_idx ON _timescaledb_internal._materialized_hypertable_20 USING btree (timec DESC)
(4 rows)

drop view mat_with_test cascade;
DROP VIEW mat_with_test;
--no additional indexes
create or replace view mat_with_test( timec, minl, sumt , sumh)
WITH (timescaledb.continuous, timescaledb.materialized_only=true, timescaledb.refresh_lag = '5 hours', timescaledb.refresh_interval = '1h', timescaledb.create_group_indexes=false)
Expand Down Expand Up @@ -1166,7 +1163,7 @@ WARNING: type integer text
100 |
(3 rows)

DROP view mat_ffunc_test cascade;
DROP view mat_ffunc_test;
NOTICE: drop cascades to table _timescaledb_internal._hyper_27_67_chunk
create or replace view mat_ffunc_test
WITH (timescaledb.continuous, timescaledb.materialized_only=true, timescaledb.refresh_lag = '-200')
Expand All @@ -1188,7 +1185,7 @@ WARNING: type integer bigint
(3 rows)

--refresh mat view test when time_bucket is not projected --
drop view mat_ffunc_test cascade;
DROP VIEW mat_ffunc_test;
NOTICE: drop cascades to table _timescaledb_internal._hyper_28_68_chunk
create or replace view mat_refresh_test
WITH (timescaledb.continuous, timescaledb.materialized_only=true, timescaledb.refresh_lag = '-200')
Expand Down
4 changes: 2 additions & 2 deletions tsl/test/expected/continuous_aggs_bgw.out
View file
Open in desktop
Expand Up @@ -342,7 +342,7 @@ SELECT ts_bgw_params_reset_time();

(1 row)

DROP VIEW test_continuous_agg_view CASCADE;
DROP VIEW test_continuous_agg_view;
NOTICE: drop cascades to table _timescaledb_internal._hyper_2_3_chunk
CREATE VIEW test_continuous_agg_view
WITH (timescaledb.continuous,
Expand Down Expand Up @@ -486,7 +486,7 @@ job_status | Scheduled
last_run_duration |

\x off
DROP VIEW test_continuous_agg_view CASCADE;
DROP VIEW test_continuous_agg_view;
NOTICE: drop cascades to table _timescaledb_internal._hyper_3_4_chunk
--create a view with a function that it has no permission to execute
CREATE VIEW test_continuous_agg_view
Expand Down
10 changes: 3 additions & 7 deletions tsl/test/expected/continuous_aggs_dump.out
View file
Open in desktop
Expand Up @@ -82,7 +82,7 @@ SELECT
replace(:'QUERY_TEMPLATE', 'TABLE', 'conditions_before') AS "QUERY_BEFORE",
replace(:'QUERY_TEMPLATE', 'TABLE', 'conditions_after') AS "QUERY_AFTER"
\gset
DROP VIEW if exists mat_test cascade;
DROP VIEW IF EXISTS mat_test;
NOTICE: view "mat_test" does not exist, skipping
--materialize this VIEW before dump this tests
--that the partial state survives the dump intact
Expand Down Expand Up @@ -176,10 +176,6 @@ DROP table :"MAT_SCHEMA_NAME".:"MAT_TABLE_NAME";
ERROR: cannot drop table _timescaledb_internal._materialized_hypertable_3 because other objects depend on it
DROP VIEW :"PART_VIEW_SCHEMA".:"PART_VIEW_NAME";
ERROR: cannot drop the partial/direct view because it is required by a continuous aggregate
DROP VIEW mat_before;
ERROR: dropping a continuous aggregate requires using CASCADE
DROP VIEW mat_after;
ERROR: dropping a continuous aggregate requires using CASCADE
\set ON_ERROR_STOP 1
--materialize mat_after
SET timescaledb.current_timestamp_mock = '2019-02-01 00:00';
Expand Down Expand Up @@ -219,7 +215,7 @@ SELECT count(*) FROM conditions_after;
Number of rows different between view and original (expect 0) | mat_after | 0
(1 row)

DROP VIEW mat_after cascade;
DROP VIEW mat_after;
NOTICE: drop cascades to 2 other objects
DROP VIEW mat_before cascade;
DROP VIEW mat_before;
NOTICE: drop cascades to 2 other objects
2 changes: 1 addition & 1 deletion tsl/test/expected/continuous_aggs_errors.out
View file
Open in desktop
Expand Up @@ -489,7 +489,7 @@ ERROR: timescaledb.refresh_lag out of range
ALTER TABLE conditions ALTER timec type bigint;
ERROR: cannot alter type of a column used by a view or rule
\set ON_ERROR_STOP
drop view mat_with_test CASCADE;
DROP VIEW mat_with_test;
ALTER TABLE conditions ALTER timec type bigint;
create or replace view mat_with_test( timec, minl, sumt , sumh)
WITH ( timescaledb.continuous, timescaledb.refresh_lag = '2147483647', timescaledb.refresh_interval = '2h')
Expand Down
6 changes: 3 additions & 3 deletions tsl/test/expected/continuous_aggs_materialize.out
View file
Open in desktop
Expand Up @@ -955,9 +955,9 @@ SELECT hypertable_id, watermark
(1 row)

--cleanup of continuous agg views --
DROP view test_t_mat_view CASCADE;
DROP view test_t_mat_view;
NOTICE: drop cascades to table _timescaledb_internal._hyper_6_16_chunk
DROP view extreme_view CASCADE;
DROP view extreme_view;
NOTICE: drop cascades to 4 other objects
-- negative lag test
CREATE TABLE continuous_agg_negative(time BIGINT, data BIGINT);
Expand Down Expand Up @@ -1032,7 +1032,7 @@ SELECT * FROM negative_view_5 ORDER BY 1;
10 | 5
(3 rows)

DROP VIEW negative_view_5 CASCADE;
DROP VIEW negative_view_5;
NOTICE: drop cascades to table _timescaledb_internal._hyper_10_27_chunk
TRUNCATE continuous_agg_negative;
CREATE VIEW negative_view_5
Expand Down
6 changes: 3 additions & 3 deletions tsl/test/expected/continuous_aggs_multi.out
View file
Open in desktop
Expand Up @@ -215,7 +215,7 @@ select count(*) from _timescaledb_catalog.continuous_aggs_materialization_invali
(1 row)

--now drop cagg_1, should still have materialization_invalidation_log
drop view cagg_1 cascade;
DROP VIEW cagg_1;
NOTICE: drop cascades to table _timescaledb_internal._hyper_2_5_chunk
select count(*) from _timescaledb_catalog.continuous_aggs_materialization_invalidation_log;
count
Expand Down Expand Up @@ -386,9 +386,9 @@ select * from _timescaledb_catalog.continuous_aggs_materialization_invalidation_
6 | 18 | 18 | 18
(2 rows)

DROP VIEW cagg_1 cascade;
DROP VIEW cagg_1;
NOTICE: drop cascades to table _timescaledb_internal._hyper_5_11_chunk
DROP VIEW cagg_2 cascade;
DROP VIEW cagg_2;
NOTICE: drop cascades to table _timescaledb_internal._hyper_6_12_chunk
--TEST6 test the ignore_invalidation_older_than setting
CREATE TABLE continuous_agg_test_ignore_invalidation_older_than(timeval integer, col1 integer, col2 integer);
Expand Down
4 changes: 2 additions & 2 deletions tsl/test/expected/continuous_aggs_permissions.out
View file
Open in desktop
Expand Up @@ -111,7 +111,7 @@ ALTER VIEW mat_refresh_test SET(timescaledb.refresh_lag = '6 h', timescaledb.ref
ERROR: must be owner of view mat_refresh_test
ALTER VIEW mat_refresh_test SET(timescaledb.materialized_only = true);
ERROR: must be owner of view mat_refresh_test
DROP VIEW mat_refresh_test CASCADE;
DROP VIEW mat_refresh_test;
ERROR: must be owner of view mat_refresh_test
REFRESH MATERIALIZED VIEW mat_refresh_test;
ERROR: permission denied for table conditions
Expand Down Expand Up @@ -152,7 +152,7 @@ NOTICE: adding index _materialized_hypertable_7_get_constant_time_partition_col
--this should fail
REFRESH MATERIALIZED VIEW mat_perm_view_test;
ERROR: permission denied for function get_constant
DROP VIEW mat_perm_view_test CASCADE;
DROP VIEW mat_perm_view_test;
--can create a mat view on something with select and trigger grants
create or replace view mat_perm_view_test
WITH ( timescaledb.continuous, timescaledb.materialized_only=true, timescaledb.refresh_lag = '-200')
Expand Down
6 changes: 3 additions & 3 deletions tsl/test/expected/continuous_aggs_union_view-11.out
View file
Open in desktop
Expand Up @@ -177,7 +177,7 @@ SELECT time_bucket,avg FROM metrics_summary ORDER BY 1;
Fri Dec 31 16:00:00 1999 PST | 0.5
(1 row)

DROP VIEW metrics_summary CASCADE;
DROP VIEW metrics_summary;
NOTICE: drop cascades to table _timescaledb_internal._hyper_2_2_chunk
-- check default view for new continuous aggregate with materialized_only to true
CREATE VIEW metrics_summary
Expand Down Expand Up @@ -259,7 +259,7 @@ SELECT time_bucket,avg FROM metrics_summary ORDER BY 1;
-------------+-----
(0 rows)

DROP VIEW metrics_summary CASCADE;
DROP VIEW metrics_summary;
--
-- test queries on union view
--
Expand Down Expand Up @@ -548,7 +548,7 @@ ORDER BY 1;
31 | 2 | 129 | 56 | 50
(3 rows)

DROP VIEW mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to table _timescaledb_internal._hyper_8_12_chunk
--- TEST union view with multiple WHERE and HAVING clauses
CREATE VIEW mat_m1
Expand Down
6 changes: 3 additions & 3 deletions tsl/test/expected/continuous_aggs_union_view-12.out
View file
Open in desktop
Expand Up @@ -177,7 +177,7 @@ SELECT time_bucket,avg FROM metrics_summary ORDER BY 1;
Fri Dec 31 16:00:00 1999 PST | 0.5
(1 row)

DROP VIEW metrics_summary CASCADE;
DROP VIEW metrics_summary;
NOTICE: drop cascades to table _timescaledb_internal._hyper_2_2_chunk
-- check default view for new continuous aggregate with materialized_only to true
CREATE VIEW metrics_summary
Expand Down Expand Up @@ -259,7 +259,7 @@ SELECT time_bucket,avg FROM metrics_summary ORDER BY 1;
-------------+-----
(0 rows)

DROP VIEW metrics_summary CASCADE;
DROP VIEW metrics_summary;
--
-- test queries on union view
--
Expand Down Expand Up @@ -548,7 +548,7 @@ ORDER BY 1;
31 | 2 | 129 | 56 | 50
(3 rows)

DROP VIEW mat_m1 cascade;
DROP VIEW mat_m1;
NOTICE: drop cascades to table _timescaledb_internal._hyper_8_12_chunk
--- TEST union view with multiple WHERE and HAVING clauses
CREATE VIEW mat_m1
Expand Down