Check configuration in alter_job and add_job #2689

mkindahl · 2020-11-26T17:46:04Z

If a bad value is given to alter_job or add_job for a configuration
parameter, no error will be given but the job will fail to execute.

This commit adds checks of the configuration parameters to the
functions so that an error is given immediately when calling it. The
commit factors out the extraction of parameters from the configuration
from the execution functions into a separate functions and calls them
from alter_job and add_job as well as when executing the job. Only
non-custom job checks are done.

The commit also moves a few functions that were only used in TSL code
from the src/ directory to the tsl/src/ directory and also removes
a redundant permission check.

Closes #2607

codecov · 2020-11-27T09:25:57Z

Codecov Report

Merging #2689 (c2bd78c) into master (9ac67fe) will increase coverage by 0.16%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master    #2689      +/-   ##
==========================================
+ Coverage   90.03%   90.19%   +0.16%     
==========================================
  Files         212      212              
  Lines       34519    34508      -11     
==========================================
+ Hits        31078    31125      +47     
+ Misses       3441     3383      -58

Impacted Files	Coverage Δ
src/bgw/job.c	`91.44% <ø> (-0.89%)`	⬇️
tsl/src/bgw_policy/job_api.c	`96.17% <96.87%> (+1.38%)`	⬆️
tsl/src/bgw_policy/job.c	`97.25% <100.00%> (+4.94%)`	⬆️
src/loader/bgw_message_queue.c	`84.51% <0.00%> (-2.59%)`	⬇️
tsl/src/bgw_policy/continuous_aggregate_api.c	`97.64% <0.00%> (+1.76%)`	⬆️
src/jsonb_utils.c	`81.70% <0.00%> (+2.43%)`	⬆️
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 3544e33...c2bd78c. Read the comment docs.

erimatnor

This looks like a definite step in the right direction.

I would suggest two things, however:

Since this is already a refactor, I suggest defining PolicyConfig objects for each policy instead of using "ad hoc" parameters when parsing. This makes it much cleaner to parse jsonb->PolicyConfig and opens up for more structured parsing/validation as suggested by example inline. I'll let you decide.
I would also consider having a generic parse/validate function for custom jobs, which can at least check that an "alter" of the policy is compatible with the existing config (not changing types of existing key-values, etc.). This can be left for follow up PR.

tsl/src/bgw_policy/job.c

tsl/src/bgw_policy/job_api.c

mkindahl

I'm not entirely convinced that we need to create dynamic objects (that is, tagged structures) for it, but maybe it can simplify some things. Will test a few things.

tsl/src/bgw_policy/job.c

mkindahl · 2020-11-30T12:16:16Z

This looks like a definite step in the right direction.

I would suggest two things, however:

* Since this is already a refactor, I suggest defining PolicyConfig objects for each policy instead of using "ad hoc" parameters when parsing. This makes it much cleaner to parse jsonb->PolicyConfig and opens up for more structured parsing/validation as suggested by example inline. I'll let you decide.

* I would also consider having a generic parse/validate function for custom jobs, which can at least check that an "alter" of the policy is compatible with the existing config (not changing types of existing key-values, etc.). This can be left for follow up PR.

I created structures for each of the policy with the parameters so that they can easily be extracted. However, creating a generic structure like the one you suggest is useful when you work with the structures dynamically, which is done for instance with the nodes in PostgreSQL. However, in this case, we really only parse the configuration parameters from the JSON and this is done inside code specific to each procedure, hence there is no need to look up any form of tag. In other words, we have little need for the dynamic nature that it would result in, at least not for the time being, and would needlessly make the code more complicated. I think this is better done when we really that for some feature.

tsl/src/bgw_policy/job_api.c

tsl/src/bgw_policy/job.c

tsl/test/expected/continuous_aggs_errors.out

erimatnor · 2020-12-01T08:11:54Z

tsl/test/sql/continuous_aggs_errors.sql

+
+-- First test that add_job checks the config. These should all generate errors.
+\set ON_ERROR_STOP 0
+SELECT add_job(


Why use add_job here instead of the job-specific wrappers? Should we test both? At least having an explanation for this would make sense.

I am also wondering if we should actually raise an error for this case and point to the policy-specific APIs when people are adding policies using our "official" policy functions.

There is no need to test the job-specific wrappers since they take correctly typed arguments and construct the config. The function add_job accepts a configuration and we need to do type-checking here so making sure that it works.

It might be reasonable to not allow creating non-custom jobs using this API, but this is outside the scope of this PR and warrant a separate discussion, so we should put this in a separate PR.

Added a more elaborate description of why we test this.

I am not sure I agree. This seems like a strange way to test validation in add_job simply because custom jobs (which this function is typically used for) currently doesn't do any validation of the config.

So, why test something that is not the way we'd like people to use this? It seems odd, in particular since we currently have no validation of the config for custom jobs, where it really would matter for add_job.

I think your comment about job-specific wrappers validating the config in other ways (with typed arguments) says it all: we now have two different ways of adding the same jobs where one way provides must stronger validation through typed arguments. That concerns me.

I agree with Erik here, unless we check those cases elsewhere we should also test job wrappers to see that they act in the same or valid manner.

The job wrappers are already tested elsewhere: they build the config so there is nothing to check. The only purpose of this test is to check that users cannot inject an invalid config using this interface. I think that any changes to the current API requires a separate discussion, so it is out of scope for this PR.

erimatnor · 2020-12-01T11:43:43Z

scripts/clang_format_wrapper.sh

@@ -67,7 +67,7 @@ echo "formatting"

 cd ${TEMP_DIR}

-clang-format ${CLANG_FORMAT_FLAGS} ${FILE_NAMES}
+clang-format-8 ${CLANG_FORMAT_FLAGS} ${FILE_NAMES}


Unrelated change. This might break other people's setup.

Current system for picking clang-format is broken, and I had local changes to handle that, so this was accidentally added.

erimatnor · 2020-12-01T11:51:15Z

tsl/src/bgw_policy/job.c

-		/* if job does its own transaction handling it might not have set a snapshot */
-		if (ActiveSnapshotSet())
-			PopActiveSnapshot();
+	if (pushed_snapshot && ActiveSnapshotSet())


I would add a comment here about why we need both checks, i.e., that we cannot trust the function or procedure that we execute to leave us with an active snapshot, even if we pushed one, since it can be a multi-transaction procedure.

Added comment.

Isn't pushed_snapshot check shoud be enough here? Maybe assert in such case, to make sure snapshot is still active

No. The action above can actually start a new transaction with no active snapshots at all (the cagg refresh does this), so we have to check if there is an active snapshot even if we pushed one above.

erimatnor

Approving, but there are two things to check:

Remove change that sets new clang-format
Potentially add comment explaining condition for popping snapshot

pmwkaa

Feels like a good change. I am wondering if we could simplify job interfaces here even further in the future. For example, having unified functions for validating/parsing and executing jobs declared from job definition as function pointers, etc.

pmwkaa · 2020-12-02T08:57:54Z

tsl/src/bgw_policy/job.c

-			PopActiveSnapshot();
+	/* Both checks are needed: if the executed procedure commit the
+	 * transaction---which `continuous_agg_refresh_internal` does, for
+	 * example---it will remove the active snapshot and and start a new


pmwkaa · 2020-12-02T08:59:07Z

tsl/src/bgw_policy/job.c

-		/* if job does its own transaction handling it might not have set a snapshot */
-		if (ActiveSnapshotSet())
-			PopActiveSnapshot();
+	if (pushed_snapshot && ActiveSnapshotSet())


Isn't pushed_snapshot check shoud be enough here? Maybe assert in such case, to make sure snapshot is still active

pmwkaa · 2020-12-02T09:09:27Z

tsl/test/sql/continuous_aggs_errors.sql

+
+-- First test that add_job checks the config. These should all generate errors.
+\set ON_ERROR_STOP 0
+SELECT add_job(


I agree with Erik here, unless we check those cases elsewhere we should also test job wrappers to see that they act in the same or valid manner.

If a bad value is given to `alter_job` or `add_job` for a configuration parameter, no error will be given but the job will fail to execute. This commit adds checks of the configuration parameters to the functions so that an error is given immediately when calling it. The commit factors out the extraction of parameters from the configuration from the execution functions into a separate functions and calls them from `alter_job` and `add_job` as well as when executing the job. Only non-custom job checks are done. The commit also moves a few functions that were only used in TSL code from the `src/` directory to the `tsl/src/` directory and also removes a redundant permission check and does a minor refactoring of the `job_execute` function so that an active snapshot is always created regardless of whether a transaction is open or not. The corresponding code in the individual policy functions are removed since they are not needed. Closes timescale#2607

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It adds checks of configuration, adds support for gapfill on distributed tables, and improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * #2689 Check configuration in alter_job and add_job * #2696 Support gapfill on distributed hypertable * #2468 Show more information in get_git_commit * #2678 Include user actions into job stats view * #2664 Fix support for complex aggregate expression * #2672 Add hypertable to continuous aggregates view * #2662 Save compression metadata settings on access node * #2707 Introduce additional db for data node bootstrapping **Bugfixes** * #2688 Fix crash for concurrent drop and compress chunk * #2666 Fix timeout handling in async library * #2683 Fix crash in add_job when given NULL interval * #2698 Improve memory handling for remote COPY * #2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * #2689 Check configuration in alter_job and add_job * #2696 Support gapfill on distributed hypertable * #2468 Show more information in get_git_commit * #2678 Include user actions into job stats view * #2664 Fix support for complex aggregate expression * #2672 Add hypertable to continuous aggregates view * #2662 Save compression metadata settings on access node * #2707 Introduce additional db for data node bootstrapping **Bugfixes** * #2688 Fix crash for concurrent drop and compress chunk * #2666 Fix timeout handling in async library * #2683 Fix crash in add_job when given NULL interval * #2698 Improve memory handling for remote COPY * #2555 Set metadata for chunks compressed before 2.0

mkindahl force-pushed the check_config branch from b8bc325 to 0c290a2 Compare November 26, 2020 17:56

mkindahl changed the title ~~Add configuration checks to alter_table~~ Add configuration checks to alter_job Nov 26, 2020

mkindahl force-pushed the check_config branch 5 times, most recently from 0d7bf7f to d6d0403 Compare November 26, 2020 20:17

mkindahl changed the title ~~Add configuration checks to alter_job~~ Check configuration in alter_job and add_job Nov 26, 2020

mkindahl force-pushed the check_config branch 2 times, most recently from 9b581a4 to c09bb38 Compare November 27, 2020 09:12

mkindahl marked this pull request as ready for review November 27, 2020 09:22

mkindahl requested a review from a team as a code owner November 27, 2020 09:22

mkindahl requested review from pmwkaa, erimatnor, gayyappan and svenklemm and removed request for a team November 27, 2020 09:22

mkindahl self-assigned this Nov 27, 2020

mkindahl added ui/ux bgw The background worker subsystem, including the scheduler labels Nov 27, 2020

erimatnor reviewed Nov 27, 2020

View reviewed changes

tsl/src/bgw_policy/job.c Outdated Show resolved Hide resolved

tsl/src/bgw_policy/job.c Outdated Show resolved Hide resolved

tsl/src/bgw_policy/job_api.c Show resolved Hide resolved

mkindahl commented Nov 27, 2020

View reviewed changes

tsl/src/bgw_policy/job.c Outdated Show resolved Hide resolved

mkindahl force-pushed the check_config branch from 530dab1 to d05ed7e Compare November 30, 2020 12:11

mkindahl force-pushed the check_config branch 2 times, most recently from 2b9410f to 30dfec0 Compare November 30, 2020 13:24

mkindahl requested a review from erimatnor November 30, 2020 14:34

erimatnor reviewed Dec 1, 2020

View reviewed changes

mkindahl requested a review from erimatnor December 1, 2020 10:06

mkindahl force-pushed the check_config branch from f37ea87 to 8243056 Compare December 1, 2020 11:24

erimatnor reviewed Dec 1, 2020

View reviewed changes

erimatnor approved these changes Dec 1, 2020

View reviewed changes

mkindahl force-pushed the check_config branch from 8243056 to a7030a8 Compare December 1, 2020 12:49

erimatnor approved these changes Dec 1, 2020

View reviewed changes

mkindahl requested a review from k-rus December 1, 2020 19:29

pmwkaa approved these changes Dec 2, 2020

View reviewed changes

mkindahl force-pushed the check_config branch from a7030a8 to c2bd78c Compare December 2, 2020 09:34

mkindahl merged commit d043ff1 into timescale:master Dec 2, 2020

mkindahl deleted the check_config branch December 2, 2020 10:04

mkindahl mentioned this pull request Dec 2, 2020

Release 2.0.0-rc4 #2702

Merged

k-rus mentioned this pull request Jan 19, 2021

Fix tuple concurrently updated error when concurrently calling alter_job_schedule #2165

Closed

mkindahl mentioned this pull request Feb 3, 2021

Configuration check function for custom actions #2895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check configuration in alter_job and add_job #2689

Check configuration in alter_job and add_job #2689

mkindahl commented Nov 26, 2020 •

edited

Loading

codecov bot commented Nov 27, 2020 •

edited

Loading

erimatnor left a comment •

edited

Loading

mkindahl left a comment

mkindahl commented Nov 30, 2020

erimatnor Dec 1, 2020

mkindahl Dec 1, 2020

mkindahl Dec 1, 2020

erimatnor Dec 1, 2020

pmwkaa Dec 2, 2020

mkindahl Dec 2, 2020

erimatnor Dec 1, 2020

mkindahl Dec 1, 2020

erimatnor Dec 1, 2020

mkindahl Dec 1, 2020

pmwkaa Dec 2, 2020

mkindahl Dec 2, 2020

erimatnor left a comment

pmwkaa left a comment

pmwkaa Dec 2, 2020

mkindahl Dec 2, 2020

pmwkaa Dec 2, 2020

pmwkaa Dec 2, 2020

Check configuration in alter_job and add_job #2689

Check configuration in alter_job and add_job #2689

Conversation

mkindahl commented Nov 26, 2020 • edited Loading

codecov bot commented Nov 27, 2020 • edited Loading

Codecov Report

erimatnor left a comment • edited Loading

Choose a reason for hiding this comment

mkindahl left a comment

Choose a reason for hiding this comment

mkindahl commented Nov 30, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor left a comment

Choose a reason for hiding this comment

pmwkaa 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 Nov 26, 2020 •

edited

Loading

codecov bot commented Nov 27, 2020 •

edited

Loading

erimatnor left a comment •

edited

Loading