-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
adding support for using background workers instead of libpq + adding support for an audit table #111
Conversation
@@ -3,6 +3,9 @@ | |||
* src/pg_cron.c | |||
* | |||
* Implementation of the pg_cron task scheduler. | |||
* Wording: | |||
* - A job is a scheduling definition of a task | |||
* - A task is what is actually executed within the database engine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit that this wasn't the greatest use of words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very polished already.
The only issue I found so far was that a WARNING results in "failed" status. We could also do a little more to restrict concurrency within the bounds of max_worker_processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a bit mean. It hits an assert failure.
SELECT cron.schedule('* * * * *', 'drop extension pg_cron');
2020-09-01 16:15:00.007 CEST [3775] LOG: cron job 2 starting: drop extension pg_cron
TRAP: FailedAssertion("!(update_result)", File: "src/pg_cron.c", Line: 1647)
…back, adapt max running tasks
|
||
appendStringInfo(&querybuf, | ||
"update %s.%s set status = '%s', return_message = 'server restarted' where status in ('%s','%s')" | ||
, CRON_SCHEMA_NAME, JOB_RUN_DETAILS_TABLE_NAME, GetCronStatus(CRON_STATUS_FAILED), GetCronStatus(CRON_STATUS_STARTING), GetCronStatus(CRON_STATUS_RUNNING)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's usually nicer to use quote_qualified_identifier to generate qualified table names.
@@ -275,6 +281,43 @@ NextJobId(void) | |||
return jobId; | |||
} | |||
|
|||
int64 | |||
NextRunId(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some style things like having comments on all functions, but I think it's easiest to merge and adjust them later.
if (edata.elevel >= ERROR) | ||
UpdateJobRunDetail(task->runId, NULL, GetCronStatus(CRON_STATUS_FAILED), display_msg.data, NULL, &end_time); | ||
else | ||
UpdateJobRunDetail(task->runId, NULL, GetCronStatus(CRON_STATUS_SUCCEEDED), display_msg.data, NULL, &end_time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of neat to see the notices appear in the table. They do get overwritten once the task is done. Is that intentional? (I don't mind this behaviour)
ereport(WARNING, | ||
(errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), | ||
errmsg("out of background worker slots"), | ||
errhint("You might need to increase max_worker_processes."))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to not put this relevant info in the errorMessage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems fine and I'm happy to merge as is and address remaining comments myself.
I do want to confirm: Is the current warning/notice behaviour as intended?
Hi Marco, Thanks for the review! Bertrand |
Found some compilation issues on PG13:
Will merge into a separate branch to fix. |
Other issue I'll try to fix is to degrade more gracefully when the schema is outdated. pg_cron starts reading from the metadata as soon as the server starts, so even if the user knows to do |
These are exciting changes, when can a new release be expected? |
This PR is mainly made of 4 commits:
Let's explain each commit
background worker support
This commit is adding support for using background workers instead of libpq sessions to execute the jobs.
This behavior is guarded behind a GUC and disabled by default so that it does not impact backward compatibility.
The background workers use a similar technique to the
pg_background
extension to run commands, and things like cron.host, nodename, and nodeport have no effect in this mode.audit table support
This commit is adding support for an audit table.
It works for either libpq or background-worker mode.
content looks like:
job and task wording
This commit is adding some wording about job and task.
renamed bgworker scheduler to launcher
This commit is renaming the current background worker from "scheduler" to "launcher" (to go with autovac nomenclature) to avoid confusion with the new workers.