Skip to content
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

Update 004_pending_and_more migration to add pending value to river_job_state only if it does not exists #364

Merged
merged 1 commit into from
May 20, 2024

Conversation

umed
Copy link
Contributor

@umed umed commented May 19, 2024

Hi guys,

004_pending_and_more.down.sql migration does not remove river_job_state's pending value. It brings us to situation where we can't apply this migration after rollback.

Steps to reproduce:

  1. apply migration 004 to database
  2. rollback
  3. apply migration 004

Expected behavior:

All migrations applied without error

Actual:

failed: error applying version 004 [UP]: ERROR: enum label "pending" already exists (SQLSTATE 42710)
exit status 1

This PR is meant to fix this behavior by optionally adding pending value during migration application.

@umed
Copy link
Contributor Author

umed commented May 19, 2024

I tested it just by executing queries on locally running postgres. I would gladly follow if you point out to how properly test it.

Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts @brandur ?

@@ -12,7 +12,7 @@ UPDATE river_job SET metadata = '{}' WHERE metadata IS NULL;
ALTER TABLE river_job ALTER COLUMN metadata SET NOT NULL;

-- The 'pending' job state will be used for upcoming functionality:
ALTER TYPE river_job_state ADD VALUE 'pending' AFTER 'discarded';
ALTER TYPE river_job_state ADD VALUE IF NOT EXISTS 'pending' AFTER 'discarded';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we don’t want to use IF NOT EXISTS in these migrations because they indicate a bug or something has been manually manipulated. But, in this case I’m reminded that you can’t safely remove an enum value in Postgres once it’s been added, which is why it was skipped in the down migration.

Maybe this is this best we can do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't see any other viable options.

The migration in its current form isn't right because it makes the rollback unusable given it can't be used to roll forward again.

A possibility is to to an enum swap by creating a whole new enum without pending, then swap it into place on state, and remove the old enum. I'm sure that'd need a full table scan to check the type though.

So yep, looks like this is the path.

Copy link
Contributor

@brandur brandur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a changelog separately.

@brandur brandur merged commit 3d79ff0 into riverqueue:master May 20, 2024
10 checks passed
brandur added a commit that referenced this pull request May 20, 2024
Prepare version 0.6.1 for release, including the changes from #350 (no
premature rescue for jobs with long custom timeouts), #363 (exit with
status 1 in case of bad command/flags) in CLI, and #364 (fix migration
version 4 to be re-runnable).
@brandur brandur mentioned this pull request May 20, 2024
brandur added a commit that referenced this pull request May 21, 2024
Prepare version 0.6.1 for release, including the changes from #350 (no
premature rescue for jobs with long custom timeouts), #363 (exit with
status 1 in case of bad command/flags) in CLI, and #364 (fix migration
version 4 to be re-runnable).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants