Fix: Improve pre-migration check clarity and accuracy #243
Conversation
|
| Name | Type |
|---|---|
| @pgflow/core | Minor |
| pgflow | Minor |
| @pgflow/client | Minor |
| @pgflow/edge-worker | Minor |
| @pgflow/example-flows | Minor |
| @pgflow/dsl | Minor |
| @pgflow/website | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
View your CI Pipeline Execution ↗ for commit a3222aa
☁️ Nx Cloud last updated this comment at |
…ion and pre-migration checks Implement schema changes to support map step type in SQL core, including automatic data migrations. Update pre-migration check queries to identify data requiring migration, ensuring safe deployment. Enhance documentation with detailed instructions for verifying migration success in production.
jumski
force-pushed
the
fix-map-step-migration
branch
from
7c031e0 to
a3222aa
Compare
jumski
changed the title
![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.
ℹ️ 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
| format('Found %s completed steps', COUNT(*)) AS identifier, | ||
| format('initial_tasks will be set to 1 (old schema enforced single-task)', | ||
| '' | ||
| ) AS details |
There was a problem hiding this comment.
The DATA_BACKFILL_COMPLETED branch calls format('initial_tasks will be set to 1 (old schema enforced single-task)', ''), but the format string has no placeholders. In PostgreSQL, passing more arguments than placeholders raises too many arguments for format, so the query will fail whenever there are completed steps (which is the normal case) and the pre‑migration check cannot run. Drop the unused argument or add an explicit %s placeholder so the query always executes.
Useful? React with 👍 / 👎.
🔍 Preview Deployment: Website✅ Deployment successful! 🔗 Preview URL: https://pr-243.pgflow.pages.dev 📝 Details:
_Last updated: _ |
🔍 Preview Deployment: Playground✅ Deployment successful! 🔗 Preview URL: https://pr-243--pgflow-demo.netlify.app 📝 Details:
_Last updated: _ |
Merge activity
|
## Summary Updates the pre-migration check script (`PRE_MIGRATION_CHECK_20251006073122.sql`) to provide clearer, more accurate messaging about data migrations. Also updates the changeset documentation to match. ## Problem The original pre-migration check output was misleading: - Used vague labels like `ISSUE_1_AUTO_FIXED` and `ISSUE_2_AUTO_FIXED` - Didn't clearly communicate that these are **expected** data migrations - Missing reporting for completed steps that need backfilling - Could give false confidence that everything is "auto-fixed" without explaining the criticality ## Changes ### Pre-migration Check Script - ✅ Renamed output types from `ISSUE_*_AUTO_FIXED` to descriptive `DATA_*` labels - ✅ Added `DATA_BACKFILL_COMPLETED` row to report completed steps needing backfill - ✅ Improved detail messages to clearly state what will happen during migration - ✅ Added critical warning about packaged migration version bug - ✅ Better documentation explaining how to interpret results **Before:** ``` type | identifier | details ----------------------|-------------------------|--------------------------- ISSUE_2_AUTO_FIXED | run=8805ef09 step=embed | status=started → will set initial_tasks=1 INFO_SUMMARY | total_step_states=114 | created=0 started=1 completed=113 failed=0 ``` **After:** ``` type | identifier | details ---------------------------|---------------------------|------------------------------------------ DATA_BACKFILL_STARTED | run=8805ef09 step=embed | initial_tasks will be set to 1 (inferred from remaining_tasks=1) DATA_BACKFILL_COMPLETED | Found 113 completed steps | initial_tasks will be set to 1 (old schema enforced single-task) INFO_SUMMARY | total_step_states=114 | created=0 started=1 completed=113 failed=0 ``` ### Changeset Documentation - ✅ Updated example output to match new check format - ✅ Replaced markdown callouts with emojis (⚠️ 💡 📝) - ✅ Clarified interpretation guidance ## Testing Verified against real production-like data: 1. ✅ Created test database with pgmq extension 2. ✅ Imported schema and data dump (114 step_states: 1 started, 113 completed) 3. ✅ Ran pre-migration check - output is clear and accurate 4. ✅ Ran full migration - completed successfully 5. ✅ Verified all data properly migrated (initial_tasks backfilled correctly) 6. ✅ Verified constraints enforce correctness post-migration ## Migration Verification Note During testing, we confirmed: - **Local migration file** (`20251006073122_pgflow_add_map_step_type.sql`) is **CORRECT** ✅ - Properly splits ALTER TABLE statements - Backfills data between schema changes - Handles started steps correctly - **Packaged migration** (from published npm package) is **BROKEN** ❌ - Combines column addition and constraint addition in single ALTER TABLE - Will FAIL on databases with started steps - Package needs rebuild before next release ## Files Changed - `pkgs/core/queries/PRE_MIGRATION_CHECK_20251006073122.sql` - Improved clarity and accuracy - `.changeset/add-map-step-type-infrastructure.md` - Updated documentation and examples ## Related - Original migration PR: #234 - Issue: Misleading pre-migration check output
Summary
Updates the pre-migration check script (
PRE_MIGRATION_CHECK_20251006073122.sql) to provide clearer, more accurate messaging about data migrations. Also updates the changeset documentation to match.Problem
The original pre-migration check output was misleading:
ISSUE_1_AUTO_FIXEDandISSUE_2_AUTO_FIXEDChanges
Pre-migration Check Script
ISSUE_*_AUTO_FIXEDto descriptiveDATA_*labelsDATA_BACKFILL_COMPLETEDrow to report completed steps needing backfillBefore:
After:
Changeset Documentation
Testing
Verified against real production-like data:
Migration Verification Note
During testing, we confirmed:
Local migration file (
20251006073122_pgflow_add_map_step_type.sql) is CORRECT ✅Packaged migration (from published npm package) is BROKEN ❌
Files Changed
pkgs/core/queries/PRE_MIGRATION_CHECK_20251006073122.sql- Improved clarity and accuracy.changeset/add-map-step-type-infrastructure.md- Updated documentation and examplesRelated