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

fix(tests): Safer migrations #14452

Merged
merged 1 commit into from
Feb 28, 2023
Merged

fix(tests): Safer migrations #14452

merged 1 commit into from
Feb 28, 2023

Conversation

timgl
Copy link
Collaborator

@timgl timgl commented Feb 28, 2023

Problem

#14277 caused an outage by doing 3 things we shouldn't be doing in migrations

  • Adding a constraint
  • Adding a non-nullable field
  • Dropping tables

All of which cause locks

Changes

  • Only allow constraints when creating a table
  • Don't allow dropping a table
  • Check for non-nullable fields per statement instead of the entire migration (the test thought it was creating a model b/c that's what we were doing earlier in the migration)

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@timgl timgl enabled auto-merge (squash) February 28, 2023 15:43
@timgl timgl merged commit 76b7480 into master Feb 28, 2023
@timgl timgl deleted the safer-migrations branch February 28, 2023 16:02
fuziontech added a commit that referenced this pull request Feb 28, 2023
* master: (53 commits)
  fix: cut the undeterministic snapshots (#14461)
  feat(hogql): Events table based on hogql (#14315)
  fix(breakdown): ensure breakdown sort can sort through values of different types (#14459)
  feat(capture): gracefully catch non-string tokens (#14453)
  chore(plugin-server): add healthcheck logging for failure (#14455)
  fix(tests): Safer migrations (#14452)
  revert: "fix(person-overrides): add constraints to catch race conditions" (#14445)
  dev(codespaces): update to python3.10 (#14449)
  chore(deps): Update posthog-js to 1.50.0 (#14448)
  feat(capture): support LIGHTWEIGHT_CAPTURE_ENDPOINT_ENABLED_TOKENS=* (#14421)
  chore: upgrade d3 (#14442)
  fix(person-overrides): add constraints to catch race conditions (#14277)
  feat(cohorts): Remove postgres calculations for flags (#14272)
  feat(capture): check token shape before team resolution too (#14439)
  feat: dashboard templates (#14322)
  feat: add a 'What's New?' button to the dropdown (#14379)
  chore(recordings): don't DLQ on PostgreSQL errors (#14438)
  chore: update autocapture attribute capture (#14435)
  chore(recordings): remove hub dependency on recordings ingestion (#14418)
  chore(deps): Update posthog-js to 1.49.0 (#14436)
  ...
@EDsCODE
Copy link
Member

EDsCODE commented Feb 28, 2023

Can we limit these checks to high volume tables such as persons? This is blocking me here but I don't think the constraint I'm adding should be much of an issue

@lharries
Copy link
Contributor

lharries commented Mar 16, 2023

This is also stopping me from adding 'scope' to dashboard_templates in postgres: https://github.com/PostHog/posthog/actions/runs/4440763635/jobs/7795000871?pr=14669

Will try and find a workaround

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.

4 participants