Conversation
… and error management
There was a problem hiding this comment.
Pull request overview
Updates the Alembic migration that creates/schedules the nightly pygeoapi materialized-view refresh job, aiming to work better in managed Postgres environments with restricted pg_cron privileges.
Changes:
- Replaced direct
SELECTaccess tocron.jobwithcron.unschedule(jobname)calls. - Added PL/pgSQL exception handling and NOTICE logging around pg_cron unschedule/schedule operations.
| '{REFRESH_SCHEDULE}', | ||
| $cmd$SELECT public.{REFRESH_FUNCTION_NAME}();$cmd$ | ||
| ); | ||
| EXCEPTION |
There was a problem hiding this comment.
cron.schedule(...) is only wrapped with an EXCEPTION WHEN insufficient_privilege handler. If scheduling fails for other pg_cron-related reasons that this migration is already trying to be resilient to (e.g., undefined_function when pg_cron functions aren’t available, or other server-state errors), the migration will still abort here. Consider extending the exception handling (or adding a pre-check) so the migration can safely continue when pg_cron scheduling isn’t possible.
| EXCEPTION | |
| EXCEPTION | |
| WHEN undefined_function THEN | |
| RAISE NOTICE | |
| 'Skipping pg_cron schedule for % because cron functions are not available.', | |
| '{REFRESH_JOB_NAME}'; | |
| WHEN invalid_parameter_value THEN | |
| RAISE NOTICE | |
| 'Skipping pg_cron schedule for % due to invalid parameter value.', | |
| '{REFRESH_JOB_NAME}'; | |
| WHEN internal_error THEN | |
| RAISE NOTICE | |
| 'Skipping pg_cron schedule for % due to internal pg_cron error.', | |
| '{REFRESH_JOB_NAME}'; |
| def _schedule_refresh_job() -> str: | ||
| return f""" | ||
| DO $do$ |
There was a problem hiding this comment.
The PR description is still the template (Why/How/Notes bullets aren’t filled in). Please update the description to summarize the motivation and approach so reviewers/operators can understand the intended behavior changes around pg_cron privilege/error handling.
| BEGIN | ||
| -- Avoid direct SELECT on cron.job because managed Postgres | ||
| -- environments may deny access to the cron schema table. | ||
| PERFORM cron.unschedule('{REFRESH_JOB_NAME}'); | ||
| EXCEPTION | ||
| WHEN undefined_function THEN | ||
| NULL; | ||
| WHEN invalid_parameter_value THEN | ||
| NULL; | ||
| WHEN internal_error THEN | ||
| -- Some pg_cron builds raise internal_error when the named | ||
| -- job does not exist. Treat this as already-unscheduled. | ||
| NULL; | ||
| WHEN insufficient_privilege THEN | ||
| RAISE NOTICE | ||
| 'Skipping pg_cron unschedule for % due to insufficient privileges.', | ||
| '{REFRESH_JOB_NAME}'; | ||
| RETURN; | ||
| END; | ||
|
|
||
| PERFORM cron.schedule( |
There was a problem hiding this comment.
In _schedule_refresh_job, the undefined_function branch for cron.unschedule('{REFRESH_JOB_NAME}') is swallowed and then the code proceeds to cron.schedule(...). In environments where cron.unschedule(text) is not available, this means any pre-existing job with the same name will not be removed by this migration, which can lead to inconsistent behavior when scheduling (e.g., an existing job remains unchanged). Consider either (a) falling back to the jobid-based unschedule when possible, or (b) skipping scheduling when unschedule cannot run, or (c) handling the “job already exists” case when calling cron.schedule.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43852fefb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHEN insufficient_privilege THEN | ||
| RAISE NOTICE | ||
| 'Skipping pg_cron unschedule for % due to insufficient privileges.', | ||
| '{REFRESH_JOB_NAME}'; |
There was a problem hiding this comment.
Swallowing insufficient_privilege in the downgrade unschedule block allows the migration to continue and later drop public.refresh_pygeoapi_materialized_views(), even when the cron job could not be removed. In environments where the job is owned by a different role, this leaves the job active and it will keep firing a command that now points to a dropped function, causing recurring pg_cron execution errors after downgrade; previously this path failed fast instead of committing a broken partial teardown.
Useful? React with 👍 / 👎.
… and error management
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?