Drop legacy submit_message & groups columns from jobs#4984
Conversation
Greptile SummaryThis PR completes the migration of
Confidence Score: 4/5Safe to merge once the deployment team has confirmed that all active jobs have a corresponding job_metadata row before running migration 039, since the new INNER JOIN in FetchJobRunLeases will silently drop any leases for jobs that are missing a metadata row. The code changes themselves are mechanically correct — the phase-conditional logic is cleanly removed, tests are updated consistently, and the NOT NULL constraint on job_metadata.submit_message is properly handled in every code path. The residual risk is operational: migration 039 backfills only WHERE submit_message IS NOT NULL, and the scheduler immediately starts using an INNER JOIN after deployment. A job that slipped through without a metadata row would lose its leases silently. That operational window is short but real. internal/scheduler/database/migrations/039_backfill_and_drop_job_metadata_columns.sql and internal/scheduler/database/job_repository.go deserve a careful second look around deployment ordering — the INNER JOIN goes live the moment the binary rolls, before migration 039 has run. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'master' into job-metadata-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| INSERT INTO job_metadata (job_id, submit_message, groups) | ||
| SELECT j.job_id, j.submit_message, j.groups | ||
| FROM jobs j | ||
| WHERE j.submit_message IS NOT NULL | ||
| AND NOT EXISTS (SELECT 1 FROM job_metadata jm WHERE jm.job_id = j.job_id) | ||
| ON CONFLICT (job_id) DO NOTHING; |
There was a problem hiding this comment.
submit_message, which become permanently invisible after the column drop
The condition WHERE j.submit_message IS NOT NULL means any jobs row where submit_message is NULL and no corresponding job_metadata row already exists will never be backfilled. After the columns are dropped, FetchJobRunLeases uses an INNER JOIN with job_metadata, so those jobs will be silently excluded from every lease-fetch result — their runs will never be dispatched to an executor.
In practice this should be safe if the deployment always ran in legacy or dualWrite mode (both of which populate submit_message in jobs), but it leaves no safety net for any data anomalies. Consider adding a pre-migration assertion that SELECT COUNT(*) FROM jobs WHERE submit_message IS NULL AND NOT EXISTS (SELECT 1 FROM job_metadata jm WHERE jm.job_id = jobs.job_id) AND terminated = false returns zero before proceeding.
| case InsertJobs: | ||
| if s.migrationPhase.WritesJobMetadata() { | ||
| specs := make([]any, 0, len(o)) | ||
| for _, v := range o { | ||
| specs = append(specs, schedulerdb.JobMetadatum{ | ||
| JobID: v.JobID, | ||
| SubmitMessage: v.SubmitMessage, | ||
| Groups: v.Groups, | ||
| }) | ||
| } | ||
| if err := database.Upsert(ctx, tx, "job_metadata", specs); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| metadata := make([]any, 0, len(o)) | ||
| records := make([]any, 0, len(o)) | ||
| for _, v := range o { | ||
| job := *v | ||
| if !s.migrationPhase.WritesJobs() { | ||
| // Cutover phase: submit_message and groups live only in job_metadata. | ||
| job.SubmitMessage = nil | ||
| job.Groups = nil | ||
| } | ||
| records = append(records, job) | ||
| metadata = append(metadata, schedulerdb.JobMetadatum{ | ||
| JobID: v.Job.JobID, | ||
| SubmitMessage: v.Metadata.SubmitMessage, | ||
| Groups: v.Metadata.Groups, | ||
| }) | ||
| records = append(records, *v.Job) | ||
| } | ||
| excluded := []string{"terminated"} | ||
| if !s.migrationPhase.WritesJobs() { | ||
| excluded = append(excluded, "submit_message", "groups") | ||
| if err := database.Upsert(ctx, tx, "job_metadata", metadata); err != nil { | ||
| return err | ||
| } | ||
| err := database.Upsert(ctx, tx, "jobs", records, database.WithExcludeColumns(excluded...)) | ||
| if err != nil { | ||
| if err := database.Upsert(ctx, tx, "jobs", records, database.WithExcludeColumns("terminated")); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
job_metadata is upserted before the jobs row exists in this transaction
The new WriteDbOp path inserts into job_metadata first, then into jobs. If job_metadata(job_id) has an immediate (non-deferred) foreign-key constraint referencing jobs(job_id), this will fail at insert time. The old dualWrite code also did this, so it was presumably not an issue then, but it's worth confirming that any FK is either absent or declared DEFERRABLE INITIALLY DEFERRED before making this the only code path.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Trey Guckian <24757349+tgucks@users.noreply.github.com>
Signed-off-by: Trey Guckian <24757349+tgucks@users.noreply.github.com>
tgucks
force-pushed
the
job-metadata-locked-migration
branch
from
b6a2413 to
05f81a7
Compare
1 participant
What type of PR is this?
Feature/Refactor
What this PR does / why we need it
This PR removes the config flags for the migration of the columns
groupsandsubmit_messagefrom thescheduler.jobstable to thejob_metadatatable.Special notes for your reviewer
Migration 039 Lock-Hold Benchmark
Migration 039 backfills
job_metadatafromjobsand drops the legacysubmit_message/groupscolumns, all under anACCESS EXCLUSIVElock. This measures how long that lock is held.Environment: Postgres 17.10 (Docker on macOS).
submit_message= 2 KB,groups= 64 B,scheduling_info= 512 B. Numbers are pessimistic vs. production (slower virtualized disk; 2 KB payloads exceed the real ~500 B compressed size and force TOAST writes).What's timed: the full migration block —
INSERT … SELECT+ 2×DROP COLUMN+COMMIT. The INSERT is >99.9% of the window; theDROP COLUMNs are catalog-only and stay ~1-6 ms at every scale.Lock-hold time (legacy phase, worst case)
Scaling is linear (~2 µs/row): 2.02 → 4.20 → 9.91 → 24.49 s.
By migration phase
Only the legacy phase copies rows (the worst case measured above). In dualWrite and cutover, the migration inserts 0 rows (already present, or
submit_messageis NULL) and completes in roughly the seq-scan time - sub-second to ~1 s even at 1M.Answering the original combos