Add check function to policies #4425

mkindahl · 2022-06-07T08:25:07Z

This commit adds a check function to the bgw_job table and also
create new validation functions for continuous aggregate, reorder,
compress, and retention policies.

The checking functions will check that the configuration is
semantically correct by reading the necessary fields for each job type
and validating that all fields are there and that they have the correct
type.

The checking function will be used to check the configuration structure
when altering the job using the alter_job function.

Fixes #2895

Disable-Check: commit-count

codecov · 2022-06-09T07:29:45Z

Codecov Report

Merging #4425 (8177008) into main (1f6d697) will increase coverage by 0.05%.
The diff coverage is 95.77%.

❗ Current head 8177008 differs from pull request most recent head ab1fb26. Consider uploading reports for the commit ab1fb26 to get more accurate results

@@            Coverage Diff             @@
##             main    #4425      +/-   ##
==========================================
+ Coverage   90.77%   90.82%   +0.05%     
==========================================
  Files         224      224              
  Lines       41915    42142     +227     
==========================================
+ Hits        38047    38275     +228     
+ Misses       3868     3867       -1

Impacted Files	Coverage Δ
src/chunk.h	`100.00% <ø> (ø)`
src/ts_catalog/catalog.h	`100.00% <ø> (ø)`
tsl/src/init.c	`95.45% <ø> (ø)`
src/cross_module_fn.c	`67.44% <25.00%> (-3.00%)`	⬇️
src/bgw/job.c	`94.58% <99.00%> (+1.43%)`	⬆️
src/chunk.c	`95.10% <100.00%> (+0.59%)`	⬆️
src/copy.c	`94.49% <100.00%> (ø)`
src/hypertable.c	`86.57% <100.00%> (+0.02%)`	⬆️
src/nodes/chunk_dispatch.c	`95.16% <100.00%> (+0.16%)`	⬆️
src/nodes/chunk_insert_state.c	`97.60% <100.00%> (-0.03%)`	⬇️
... and 16 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 7c55d0d...ab1fb26. Read the comment docs.

akuzm · 2022-08-23T11:08:53Z

src/bgw/job.h

+extern TSDLLEXPORT int32 ts_bgw_job_insert_relation(
+	Name application_name, Interval *schedule_interval, Interval *max_runtime, int32 max_retries,
+	Interval *retry_period, Name proc_schema, Name proc_name, Name check_schema, Name check_name,
+	Name owner, bool scheduled, int32 hypertable_id, Jsonb *config);
 extern TSDLLEXPORT void ts_bgw_job_permission_check(BgwJob *job);

 extern TSDLLEXPORT void ts_bgw_job_validate_job_owner(Oid owner);

 extern bool ts_bgw_job_execute(BgwJob *job);


Should we run the config check before we execute the job, as well? This would allow us to centralize all the config validity checks in the check function. Otherwise we might run into an inconsistency at some point, where the check function and the execute function have different ideas about what is the valid config.

Once registered, the config can only be changed through alter_job, or by modifying the table _timescaledb_config.bgw_job, right? The second case would lead to that kind of inconsistency but I don't think it's possible to get there if only using the APIs. But of course we can also check for config validity when executing

src/bgw/job.c

erimatnor · 2022-08-23T11:29:12Z

tsl/test/expected/bgw_custom.out

+-- test procedures
+select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_proc'::regproc);
+ERROR:  Config must have drop_after
+select add_job('test_proc_with_check', '5 secs', config => '{"one": "two"}', check_config => 'test_config_check_proc'::regproc);


Nit: given what the check procedure does, this test case doesn't test anything different from the one above, so it could be elided.

erimatnor · 2022-08-23T11:33:48Z

tsl/test/expected/bgw_custom.out

+-- test functions
+select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_func'::regproc);
+ERROR:  Config must have drop_after
+select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_func'::regproc);


That this passes is somewhat unexpected given that the check function ensures there's a drop_after. So eliding the config all-together still means there is no drop_after, so why does it work?

It seems to me we should always run the check func even if the passed in config is NULL and then let the function decide how to act.

erimatnor · 2022-08-23T11:37:25Z

tsl/src/bgw_policy/job_api.c

+
 	if (config)
-		job_config_check(&proc_schema, &proc_name, config);
+		ts_bgw_job_run_config_check(check, 0, config);


We should probably always run the config check function if it is defined, even if the config is NULL. It should be up to the check function to decide if NULL config is acceptable since providing no configuration might be a failure case. For instance, some of the tests for custom jobs check for "drop_after" and fails if not present in the config. Passing no config at all is the same as passing a JSON without "drop_after".

Agreed, will fix

Old patch was using old validation functions, but there are already validation functions that both read and validate the policy, so using those. Also removing the old `job_config_check` function since that is no longer use and instead adding a `job_config_check` that calls the checking function with the configuration.

Previously users had no way to update the check function registered with add_job. This commit adds a parameter check_config to alter_job to allow updating the check function field. Also, previously the signature expected from a check was of the form (job_id, config) and there was no validation that the check function given had the correct signature. This commit removes the job_id as it is not required and also checks that the check function has the correct signature when it is registered with add_job, preventing an error being thrown at job runtime.

@AlmiS

This release adds major new features since the 2.7.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * time_bucket now supports bucketing by month, year and timezone * Improve performance of bulk SELECT and COPY for distributed hypertables * 1 step CAgg policy management * Migrate Continuous Aggregates to the new format **Features** * timescale#4188 Use COPY protocol in row-by-row fetcher * timescale#4307 Mark partialize_agg as parallel safe * timescale#4380 Enable chunk exclusion for space dimensions in UPDATE/DELETE * timescale#4384 Add schedule_interval to policies * timescale#4390 Faster lookup of chunks by point * timescale#4393 Support intervals with day component when constifying now() * timescale#4397 Support intervals with month component when constifying now() * timescale#4405 Support ON CONFLICT ON CONSTRAINT for hypertables * timescale#4412 Add telemetry about replication * timescale#4415 Drop remote data when detaching data node * timescale#4416 Handle TRUNCATE TABLE on chunks * timescale#4425 Add parameter check_config to alter_job * timescale#4430 Create index on Continuous Aggregates * timescale#4439 Allow ORDER BY on continuous aggregates * timescale#4443 Add stateful partition mappings * timescale#4484 Use non-blocking data node connections for COPY * timescale#4495 Support add_dimension() with existing data * timescale#4502 Add chunks to baserel cache on chunk exclusion * timescale#4545 Add hypertable distributed argument and defaults * timescale#4552 Migrate Continuous Aggregates to the new format * timescale#4556 Add runtime exclusion for hypertables * timescale#4561 Change get_git_commit to return full commit hash * timescale#4563 1 step CAgg policy management * timescale#4641 Allow bucketing by month, year, century in time_bucket and time_bucket_gapfill * timescale#4642 Add timezone support to time_bucket **Bugfixes** * timescale#4359 Create composite index on segmentby columns * timescale#4374 Remove constified now() constraints from plan * timescale#4416 Handle TRUNCATE TABLE on chunks * timescale#4478 Synchronize chunk cache sizes * timescale#4486 Adding boolean column with default value doesn't work on compressed table * timescale#4512 Fix unaligned pointer access * timescale#4519 Throw better error message on incompatible row fetcher settings * timescale#4549 Fix dump_meta_data for windows * timescale#4553 Fix timescaledb_post_restore GUC handling * timescale#4573 Load TSL library on compressed_data_out call * timescale#4575 Fix use of `get_partition_hash` and `get_partition_for_key` inside an IMMUTABLE function * timescale#4577 Fix segfaults in compression code with corrupt data * timescale#4580 Handle default privileges on CAggs properly * timescale#4582 Fix assertion in GRANT .. ON ALL TABLES IN SCHEMA * timescale#4583 Fix partitioning functions * timescale#4589 Fix rename for distributed hypertable * timescale#4601 Reset compression sequence when group resets * timescale#4611 Fix a potential OOM when loading large data sets into a hypertable * timescale#4624 Fix heap buffer overflow * timescale#4627 Fix telemetry initialization * timescale#4631 Ensure TSL library is loaded on database upgrades * timescale#4646 Fix time_bucket_ng origin handling * timescale#4647 Fix the error "SubPlan found with no parent plan" that occurred if using joins in RETURNING clause. **Thanks** * @AlmiS for reporting error on `get_partition_hash` executed inside an IMMUTABLE function * @Creatation for reporting an issue with renaming hypertables * @janko for reporting an issue when adding bool column with default value to compressed hypertable * @jayadevanm for reporting error of TRUNCATE TABLE on compressed chunk * @michaelkitson for reporting permission errors using default privileges on Continuous Aggregates * @mwahlhuetter for reporting error in joins in RETURNING clause * @ninjaltd and @mrksngl for reporting a potential OOM when loading large data sets into a hypertable * @PBudmark for reporting an issue with dump_meta_data.sql on Windows * @ssmoss for reporting an issue with time_bucket_ng origin handling

@AlmiS

This release adds major new features since the 2.7.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * time_bucket now supports bucketing by month, year and timezone * Improve performance of bulk SELECT and COPY for distributed hypertables * 1 step CAgg policy management * Migrate Continuous Aggregates to the new format **Features** * #4188 Use COPY protocol in row-by-row fetcher * #4307 Mark partialize_agg as parallel safe * #4380 Enable chunk exclusion for space dimensions in UPDATE/DELETE * #4384 Add schedule_interval to policies * #4390 Faster lookup of chunks by point * #4393 Support intervals with day component when constifying now() * #4397 Support intervals with month component when constifying now() * #4405 Support ON CONFLICT ON CONSTRAINT for hypertables * #4412 Add telemetry about replication * #4415 Drop remote data when detaching data node * #4416 Handle TRUNCATE TABLE on chunks * #4425 Add parameter check_config to alter_job * #4430 Create index on Continuous Aggregates * #4439 Allow ORDER BY on continuous aggregates * #4443 Add stateful partition mappings * #4484 Use non-blocking data node connections for COPY * #4495 Support add_dimension() with existing data * #4502 Add chunks to baserel cache on chunk exclusion * #4545 Add hypertable distributed argument and defaults * #4552 Migrate Continuous Aggregates to the new format * #4556 Add runtime exclusion for hypertables * #4561 Change get_git_commit to return full commit hash * #4563 1 step CAgg policy management * #4641 Allow bucketing by month, year, century in time_bucket and time_bucket_gapfill * #4642 Add timezone support to time_bucket **Bugfixes** * #4359 Create composite index on segmentby columns * #4374 Remove constified now() constraints from plan * #4416 Handle TRUNCATE TABLE on chunks * #4478 Synchronize chunk cache sizes * #4486 Adding boolean column with default value doesn't work on compressed table * #4512 Fix unaligned pointer access * #4519 Throw better error message on incompatible row fetcher settings * #4549 Fix dump_meta_data for windows * #4553 Fix timescaledb_post_restore GUC handling * #4573 Load TSL library on compressed_data_out call * #4575 Fix use of `get_partition_hash` and `get_partition_for_key` inside an IMMUTABLE function * #4577 Fix segfaults in compression code with corrupt data * #4580 Handle default privileges on CAggs properly * #4582 Fix assertion in GRANT .. ON ALL TABLES IN SCHEMA * #4583 Fix partitioning functions * #4589 Fix rename for distributed hypertable * #4601 Reset compression sequence when group resets * #4611 Fix a potential OOM when loading large data sets into a hypertable * #4624 Fix heap buffer overflow * #4627 Fix telemetry initialization * #4631 Ensure TSL library is loaded on database upgrades * #4646 Fix time_bucket_ng origin handling * #4647 Fix the error "SubPlan found with no parent plan" that occurred if using joins in RETURNING clause. **Thanks** * @AlmiS for reporting error on `get_partition_hash` executed inside an IMMUTABLE function * @Creatation for reporting an issue with renaming hypertables * @janko for reporting an issue when adding bool column with default value to compressed hypertable * @jayadevanm for reporting error of TRUNCATE TABLE on compressed chunk * @michaelkitson for reporting permission errors using default privileges on Continuous Aggregates * @mwahlhuetter for reporting error in joins in RETURNING clause * @ninjaltd and @mrksngl for reporting a potential OOM when loading large data sets into a hypertable * @PBudmark for reporting an issue with dump_meta_data.sql on Windows * @ssmoss for reporting an issue with time_bucket_ng origin handling

@AlmiS

This release adds major new features since the 2.7.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * time_bucket now supports bucketing by month, year and timezone * Improve performance of bulk SELECT and COPY for distributed hypertables * 1 step CAgg policy management * Migrate Continuous Aggregates to the new format **Features** * #4188 Use COPY protocol in row-by-row fetcher * #4307 Mark partialize_agg as parallel safe * #4380 Enable chunk exclusion for space dimensions in UPDATE/DELETE * #4384 Add schedule_interval to policies * #4390 Faster lookup of chunks by point * #4393 Support intervals with day component when constifying now() * #4397 Support intervals with month component when constifying now() * #4405 Support ON CONFLICT ON CONSTRAINT for hypertables * #4412 Add telemetry about replication * #4415 Drop remote data when detaching data node * #4416 Handle TRUNCATE TABLE on chunks * #4425 Add parameter check_config to alter_job * #4430 Create index on Continuous Aggregates * #4439 Allow ORDER BY on continuous aggregates * #4443 Add stateful partition mappings * #4484 Use non-blocking data node connections for COPY * #4495 Support add_dimension() with existing data * #4502 Add chunks to baserel cache on chunk exclusion * #4545 Add hypertable distributed argument and defaults * #4552 Migrate Continuous Aggregates to the new format * #4556 Add runtime exclusion for hypertables * #4561 Change get_git_commit to return full commit hash * #4563 1 step CAgg policy management * #4641 Allow bucketing by month, year, century in time_bucket and time_bucket_gapfill * #4642 Add timezone support to time_bucket **Bugfixes** * #4359 Create composite index on segmentby columns * #4374 Remove constified now() constraints from plan * #4416 Handle TRUNCATE TABLE on chunks * #4478 Synchronize chunk cache sizes * #4486 Adding boolean column with default value doesn't work on compressed table * #4512 Fix unaligned pointer access * #4519 Throw better error message on incompatible row fetcher settings * #4549 Fix dump_meta_data for windows * #4553 Fix timescaledb_post_restore GUC handling * #4573 Load TSL library on compressed_data_out call * #4575 Fix use of `get_partition_hash` and `get_partition_for_key` inside an IMMUTABLE function * #4577 Fix segfaults in compression code with corrupt data * #4580 Handle default privileges on CAggs properly * #4582 Fix assertion in GRANT .. ON ALL TABLES IN SCHEMA * #4583 Fix partitioning functions * #4589 Fix rename for distributed hypertable * #4601 Reset compression sequence when group resets * #4611 Fix a potential OOM when loading large data sets into a hypertable * #4624 Fix heap buffer overflow * #4627 Fix telemetry initialization * #4631 Ensure TSL library is loaded on database upgrades * #4646 Fix time_bucket_ng origin handling * #4647 Fix the error "SubPlan found with no parent plan" that occurred if using joins in RETURNING clause. **Thanks** * @AlmiS for reporting error on `get_partition_hash` executed inside an IMMUTABLE function * @Creatation for reporting an issue with renaming hypertables * @janko for reporting an issue when adding bool column with default value to compressed hypertable * @jayadevanm for reporting error of TRUNCATE TABLE on compressed chunk * @michaelkitson for reporting permission errors using default privileges on Continuous Aggregates * @mwahlhuetter for reporting error in joins in RETURNING clause * @ninjaltd and @mrksngl for reporting a potential OOM when loading large data sets into a hypertable * @PBudmark for reporting an issue with dump_meta_data.sql on Windows * @ssmoss for reporting an issue with time_bucket_ng origin handling

@AlmiS

This release adds major new features since the 2.7.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * time_bucket now supports bucketing by month, year and timezone * Improve performance of bulk SELECT and COPY for distributed hypertables * 1 step CAgg policy management * Migrate Continuous Aggregates to the new format **Features** * #4188 Use COPY protocol in row-by-row fetcher * #4307 Mark partialize_agg as parallel safe * #4380 Enable chunk exclusion for space dimensions in UPDATE/DELETE * #4384 Add schedule_interval to policies * #4390 Faster lookup of chunks by point * #4393 Support intervals with day component when constifying now() * #4397 Support intervals with month component when constifying now() * #4405 Support ON CONFLICT ON CONSTRAINT for hypertables * #4412 Add telemetry about replication * #4415 Drop remote data when detaching data node * #4416 Handle TRUNCATE TABLE on chunks * #4425 Add parameter check_config to alter_job * #4430 Create index on Continuous Aggregates * #4439 Allow ORDER BY on continuous aggregates * #4443 Add stateful partition mappings * #4484 Use non-blocking data node connections for COPY * #4495 Support add_dimension() with existing data * #4502 Add chunks to baserel cache on chunk exclusion * #4545 Add hypertable distributed argument and defaults * #4552 Migrate Continuous Aggregates to the new format * #4556 Add runtime exclusion for hypertables * #4561 Change get_git_commit to return full commit hash * #4563 1 step CAgg policy management * #4641 Allow bucketing by month, year, century in time_bucket and time_bucket_gapfill * #4642 Add timezone support to time_bucket **Bugfixes** * #4359 Create composite index on segmentby columns * #4374 Remove constified now() constraints from plan * #4416 Handle TRUNCATE TABLE on chunks * #4478 Synchronize chunk cache sizes * #4486 Adding boolean column with default value doesn't work on compressed table * #4512 Fix unaligned pointer access * #4519 Throw better error message on incompatible row fetcher settings * #4549 Fix dump_meta_data for windows * #4553 Fix timescaledb_post_restore GUC handling * #4573 Load TSL library on compressed_data_out call * #4575 Fix use of `get_partition_hash` and `get_partition_for_key` inside an IMMUTABLE function * #4577 Fix segfaults in compression code with corrupt data * #4580 Handle default privileges on CAggs properly * #4582 Fix assertion in GRANT .. ON ALL TABLES IN SCHEMA * #4583 Fix partitioning functions * #4589 Fix rename for distributed hypertable * #4601 Reset compression sequence when group resets * #4611 Fix a potential OOM when loading large data sets into a hypertable * #4624 Fix heap buffer overflow * #4627 Fix telemetry initialization * #4631 Ensure TSL library is loaded on database upgrades * #4646 Fix time_bucket_ng origin handling * #4647 Fix the error "SubPlan found with no parent plan" that occurred if using joins in RETURNING clause. **Thanks** * @AlmiS for reporting error on `get_partition_hash` executed inside an IMMUTABLE function * @Creatation for reporting an issue with renaming hypertables * @janko for reporting an issue when adding bool column with default value to compressed hypertable * @jayadevanm for reporting error of TRUNCATE TABLE on compressed chunk * @michaelkitson for reporting permission errors using default privileges on Continuous Aggregates * @mwahlhuetter for reporting error in joins in RETURNING clause * @ninjaltd and @mrksngl for reporting a potential OOM when loading large data sets into a hypertable * @PBudmark for reporting an issue with dump_meta_data.sql on Windows * @ssmoss for reporting an issue with time_bucket_ng origin handling

Patch timescale#4425 introduced regression test failures, namely, a crash in function `ts_bgw_job_update_by_id`. The failures are due to the COMMIT statement in the custom check procedure. This patch removes that particular test case from bgw_custom.

Patch #4425 introduced regression test failures, namely, a crash in function `ts_bgw_job_update_by_id`. The failures are due to the COMMIT statement in the custom check procedure. This patch removes that particular test case from bgw_custom.

Patch timescale#4425 introduced regression test failures, namely, a crash in function `ts_bgw_job_update_by_id`. The failures are due to the COMMIT statement in the custom check procedure. This patch removes that particular test case from bgw_custom.

Patch #4425 introduced regression test failures, namely, a crash in function `ts_bgw_job_update_by_id`. The failures are due to the COMMIT statement in the custom check procedure. This patch removes that particular test case from bgw_custom.

mkindahl assigned konskov Jun 7, 2022

mkindahl force-pushed the job-validation-function branch from 8feec03 to 720da1b Compare June 7, 2022 08:35

mkindahl self-assigned this Jun 7, 2022

mkindahl force-pushed the job-validation-function branch 4 times, most recently from e53641a to f9a99b0 Compare June 9, 2022 07:20

mkindahl force-pushed the job-validation-function branch 19 times, most recently from d413017 to 4f25c0b Compare June 10, 2022 14:25

mkindahl mentioned this pull request Jun 29, 2022

Remove job_id from check signature and add tests #4477

Closed

konskov force-pushed the job-validation-function branch from 4f25c0b to dbc059f Compare June 29, 2022 06:49

konskov marked this pull request as ready for review June 29, 2022 07:07

akuzm reviewed Aug 23, 2022

View reviewed changes

akuzm approved these changes Aug 23, 2022

View reviewed changes

erimatnor reviewed Aug 23, 2022

View reviewed changes

konskov force-pushed the job-validation-function branch 3 times, most recently from d815056 to fef24e0 Compare August 24, 2022 10:21

konskov requested a review from erimatnor August 24, 2022 10:24

konskov force-pushed the job-validation-function branch from 69aa6d3 to 8177008 Compare August 24, 2022 11:48

fabriziomello approved these changes Aug 24, 2022

View reviewed changes

mkindahl and others added 2 commits August 25, 2022 09:23

konskov force-pushed the job-validation-function branch from 8177008 to ab1fb26 Compare August 25, 2022 06:36

konskov merged commit dc145b7 into main Aug 25, 2022

konskov deleted the job-validation-function branch August 25, 2022 07:38

svenklemm mentioned this pull request Aug 31, 2022

Release 2.8.0 #4653

Merged

konskov mentioned this pull request Sep 12, 2022

Remove test case causing bgw_custom crash #4691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check function to policies #4425

Add check function to policies #4425

mkindahl commented Jun 7, 2022 •

edited by konskov

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading

akuzm Aug 23, 2022

konskov Aug 24, 2022

erimatnor Aug 23, 2022

erimatnor Aug 23, 2022

erimatnor Aug 23, 2022

konskov Aug 24, 2022

Add check function to policies #4425

Add check function to policies #4425

Conversation

mkindahl commented Jun 7, 2022 • edited by konskov Loading

codecov bot commented Jun 9, 2022 • edited Loading

Codecov Report

akuzm Aug 23, 2022

Choose a reason for hiding this comment

konskov Aug 24, 2022

Choose a reason for hiding this comment

erimatnor Aug 23, 2022

Choose a reason for hiding this comment

erimatnor Aug 23, 2022

Choose a reason for hiding this comment

erimatnor Aug 23, 2022

Choose a reason for hiding this comment

konskov Aug 24, 2022

Choose a reason for hiding this comment

mkindahl commented Jun 7, 2022 •

edited by konskov

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading