Add functions to enable/disable policies #6116

JamesGuthrie · 2023-09-26T07:47:09Z

When a user performs a low-downtime data migration, we ask them to turn off some policies. Currently this instruction is an unwieldy SQL statement. This change adds some helper functions to simplify disabling various categories of policy.

{enable,disable}_all_policies(hypertable regclass) - enable/disable all compression, retention, and reorder policies on hypertable.
{enable,disable}_compression_policy(hypertable regclass) - enable/disable compression policies on hypertable.
{enable,disable}_reorder_policy(hypertable regclass) - enable/disable reorder policies on hypertable.
{enable,disable}_retention_policy(hypertable regclass) - enable/disable retention policies on hypertable.

codecov · 2023-09-26T08:10:29Z

Codecov Report

Merging #6116 (6116ec6) into main (6f038ec) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #6116      +/-   ##
==========================================
- Coverage   81.43%   81.42%   -0.02%     
==========================================
  Files         246      246              
  Lines       56978    56932      -46     
  Branches    12626    12607      -19     
==========================================
- Hits        46401    46357      -44     
+ Misses       8193     8167      -26     
- Partials     2384     2408      +24

see 45 files with indirect coverage changes

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

github-actions · 2023-09-27T09:42:04Z

@mahipv, @jnidzwetzki: please review this pull request.

Powered by pull-review

JamesGuthrie · 2023-09-27T09:51:10Z

I've obviously done something wrong because the update and downgrade tests are failing, but I'm not sure what

sql/updates/latest-dev.sql

mkindahl

I think you need to add similar functionality for the continuous aggregate policy, but otherwise looks fine. One minor comment on naming.

mkindahl · 2023-10-09T12:38:17Z

sql/policy_api.sql

+LANGUAGE SQL SET search_path TO pg_catalog, pg_temp;
+
+CREATE OR REPLACE FUNCTION _timescaledb_functions.set_all_policy_scheduled(hypertable REGCLASS, scheduled BOOL)


Nit: Consistency with the other names?

Suggested change

CREATE OR REPLACE FUNCTION _timescaledb_functions.set_all_policy_scheduled(hypertable REGCLASS, scheduled BOOL)

CREATE OR REPLACE FUNCTION _timescaledb_functions.set_all_policies_scheduled(hypertable REGCLASS, scheduled BOOL)

This would not work with data tiering policies. We should treat that as a timescale policy too.
Also, the external API is disable_all_policies and enable_all_policies - as a user I would expect that all policies including any user defined policy would get disabled/enabled.

@gayyappan Not sure I follow. Why would a different name of the above function not work with data tiering policies?

We probably also need to check user's permissions before the user is allowed to enable/disable policies. A Should a user without adequate permissions on the hypertable be allowed to disable a policy? IIRC, we check the role's permissions before adding a policy.

@mkindahl The proc_schema in the sql is limited to timescaledb_internal/timescaledb_functions. Not sure I follow. Why would a different name of the above function not work with data tiering policies?

@gayyappan Ah, so you were referring to the actual code for implementing the function, not the function name. It was a comment on my comment, so it wasn't entirely clear.

This would not work with data tiering policies.

Correct. Should it? Data tiering policies reside in a separate extension. Should helper functions in the TimescaleDB extension wrap functions in the OSM extension?

as a user I would expect that all policies including any user defined policy would get disabled/enabled.

The function takes a hypertable as its argument, so it only disables policies which are attached to a hypertable. Maybe the name should change to {enable,disable}_all_policies_for_hypertable. From what I can tell, user-defined policies are never associated with a hypertable, so it wouldn't make sense to enable/disable user-defined policies from this function.

We probably also need to check user's permissions before the user is allowed to enable/disable policies.

All of these functions delegate to alter_job under the hood. Does that not perform the permission check that you mention? If not, which permissions would need to be checked?

While this is true from a code perspective, from a user perspective, this is 1 database running timescale defined policies - so it should just work out of the box. Correct. Should it? Data tiering policies reside in a separate extension. Should helper functions in the TimescaleDB extension wrap functions in the OSM extension?
Either we wrap OSM extension or add hooks so that the functionality can be extended by the OSM extension(in the same way that the timescale extension extends Postgres functionality).

All of these functions delegate to alter_job under the hood. Does that not perform the permission check that you mention? If not, which permissions would need to be checked?

That could very well be true. You could modify the test to show that a user/role without permissions on the hypertable cannot alter policies.

@JamesGuthrie I'll add a follow up to get this to work with OSM extension as well. For this PR, we should verify that permissions are checked by the new apis and add a test case to confirm that.

mkindahl · 2023-10-09T12:41:51Z

sql/policy_api.sql

Missing similar functions for continuous aggregate policy.

erimatnor · 2023-10-09T13:07:31Z

sql/policy_api.sql

+RETURNS INTEGER
+AS $$
+    WITH affected_policies AS (
+        SELECT @[email protected]_job(j.id, scheduled => set_policy_scheduled.scheduled)
+        FROM _timescaledb_config.bgw_job j
+            JOIN _timescaledb_catalog.hypertable h ON h.id = j.hypertable_id
+        WHERE j.proc_schema IN ('_timescaledb_internal', '_timescaledb_functions')
+        AND j.proc_name = set_policy_scheduled.policy_type
+        AND j.id >= 1000
+        AND scheduled <> set_policy_scheduled.scheduled
+        AND format('%I.%I', h.schema_name, h.table_name) = set_policy_scheduled.hypertable::text
+    )
+SELECT count(*) FROM affected_policies;
+$$
+LANGUAGE SQL SET search_path TO pg_catalog, pg_temp;


Nit: instead of policy_type it might be better to just pass in the regproc ID and then match it with the proc_schema.proc_name cast to a regproc. Then it would be impossible to pass in a random text string.

Suggested change

CREATE OR REPLACE FUNCTION _timescaledb_functions.set_policy_scheduled(hypertable REGCLASS, policy_type TEXT, scheduled BOOL)

RETURNS INTEGER

AS $$

WITH affected_policies AS (

SELECT @[email protected]_job(j.id, scheduled => set_policy_scheduled.scheduled)

FROM _timescaledb_config.bgw_job j

JOIN _timescaledb_catalog.hypertable h ON h.id = j.hypertable_id

WHERE j.proc_schema IN ('_timescaledb_internal', '_timescaledb_functions')

AND j.proc_name = set_policy_scheduled.policy_type

AND j.id >= 1000

AND scheduled <> set_policy_scheduled.scheduled

AND format('%I.%I', h.schema_name, h.table_name) = set_policy_scheduled.hypertable::text

)

SELECT count(*) FROM affected_policies;

$$

LANGUAGE SQL SET search_path TO pg_catalog, pg_temp;

CREATE OR REPLACE FUNCTION _timescaledb_functions.set_policy_scheduled(hypertable REGCLASS, policy_proc REGPROC, scheduled BOOL)

RETURNS INTEGER

AS $$

WITH affected_policies AS (

SELECT @[email protected]_job(j.id, scheduled => set_policy_scheduled.scheduled)

FROM _timescaledb_config.bgw_job j

JOIN _timescaledb_catalog.hypertable h ON h.id = j.hypertable_id

WHERE format('%I.%I', j.proc_schema, j.proc_name)::regproc = set_policy_scheduled.policy_proc

AND j.id >= 1000

AND scheduled <> set_policy_scheduled.scheduled

AND format('%I.%I', h.schema_name, h.table_name) = set_policy_scheduled.hypertable::text

)

SELECT count(*) FROM affected_policies;

$$

LANGUAGE SQL SET search_path TO pg_catalog, pg_temp;

erimatnor · 2023-10-09T13:14:58Z

tsl/test/expected/policy_api_enable_disable.out

+ ?column? 
+----------
+ t
+(1 row)


Would be good to have some tests for error cases. For example, disabling a policy that has not been added.

github-actions bot assigned JamesGuthrie Sep 26, 2023

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch from 61258c1 to ac26f15 Compare September 26, 2023 07:58

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch 5 times, most recently from ce0c0e2 to 5625104 Compare September 26, 2023 12:59

JamesGuthrie marked this pull request as ready for review September 27, 2023 09:41

JamesGuthrie requested a review from mkindahl September 27, 2023 09:41

github-actions bot requested a review from jnidzwetzki September 27, 2023 09:42

github-actions bot requested a review from mahipv September 27, 2023 09:42

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch from 5625104 to f279223 Compare September 27, 2023 09:44

JamesGuthrie changed the title ~~WIP: add functions to enable/disable policies~~ Add functions to enable/disable policies Sep 27, 2023

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch 3 times, most recently from c910bb4 to dee3a27 Compare September 27, 2023 10:21

mkindahl linked an issue Sep 27, 2023 that may be closed by this pull request

[Enhancement]: hypertable-based enable and disable functions for policies #6115

Open

mkindahl reviewed Sep 27, 2023

View reviewed changes

sql/updates/latest-dev.sql Outdated Show resolved Hide resolved

sql/updates/latest-dev.sql Outdated Show resolved Hide resolved

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch 2 times, most recently from a689e89 to 574e0a0 Compare October 3, 2023 08:34

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch from 574e0a0 to 533990b Compare October 9, 2023 12:14

Add functions to enable/disable policies

6116ec6

JamesGuthrie force-pushed the jg/add-policy-enable-disable-functions branch from 533990b to 6116ec6 Compare October 9, 2023 12:15

mkindahl requested changes Oct 9, 2023

View reviewed changes

erimatnor reviewed Oct 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functions to enable/disable policies #6116

Add functions to enable/disable policies #6116

JamesGuthrie commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023

JamesGuthrie commented Sep 27, 2023

mkindahl left a comment

mkindahl Oct 9, 2023

gayyappan Oct 9, 2023 •

edited

Loading

mkindahl Oct 9, 2023

gayyappan Oct 9, 2023

gayyappan Oct 9, 2023

mkindahl Oct 9, 2023

JamesGuthrie Oct 9, 2023 •

edited

Loading

gayyappan Oct 9, 2023 •

edited

Loading

gayyappan Oct 9, 2023

gayyappan Oct 9, 2023

mkindahl Oct 9, 2023 •

edited

Loading

erimatnor Oct 9, 2023

erimatnor Oct 9, 2023

		LANGUAGE SQL SET search_path TO pg_catalog, pg_temp;

		CREATE OR REPLACE FUNCTION _timescaledb_functions.set_all_policy_scheduled(hypertable REGCLASS, scheduled BOOL)

Add functions to enable/disable policies #6116

Are you sure you want to change the base?

Add functions to enable/disable policies #6116

Conversation

JamesGuthrie commented Sep 26, 2023 • edited Loading

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 27, 2023

JamesGuthrie commented Sep 27, 2023

mkindahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gayyappan Oct 9, 2023 • edited Loading

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

JamesGuthrie Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

gayyappan Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkindahl Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGuthrie commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

gayyappan Oct 9, 2023 •

edited

Loading

JamesGuthrie Oct 9, 2023 •

edited

Loading

gayyappan Oct 9, 2023 •

edited

Loading

mkindahl Oct 9, 2023 •

edited

Loading