Conversation
5d27208 to
313f0d9
Compare
mdashti
left a comment
There was a problem hiding this comment.
I reviewed this together with paradedb/paradedb#6370, which pins the paradedb/pgrx branch (this PR plus cargo fmt). I ran the commands against pg_search, a crate nested in a workspace, and hit a few things the tests (all single-crate, no git) don't cover. Comments are inlined below.
| @@ -0,0 +1,13 @@ | |||
| To add more context from the ParadeDB side: we have been working to improve extension upgrade and migration workflows for a while. | |||
There was a problem hiding this comment.
These three markdown files at the repo root (comment-issue-2375.md, feature-request-sql-migration-fragments.md, migration-target-and-base-schema-fixes.md) look like working notes that slipped into the commit. The content is already in #2379 and in docs/src/extension/migrations.md.
| if trimmed.is_empty() || !trimmed.ends_with(".sql") { | ||
| continue; | ||
| } | ||
| let full_path = repo_root.join(trimmed); |
There was a problem hiding this comment.
git diff --name-only prints paths relative to the repository root, whatever the cwd is. But repo_root here is the crate directory (Check::execute passes crate_dir). For any crate that isn't at the git root, full_path becomes <crate>/<crate>/sql/unreleased/..., nothing matches, and --base lints nothing while reporting success. I hit this with pg_search inside the ParadeDB workspace: a fragment that re-creates a function from another unreleased fragment fails without --base and passes with --base HEAD~1. git rev-parse --show-toplevel (or git diff --relative run from the crate dir) would fix it. The e2e test in cli_migrate.rs never initialises a git repo, so a nested-crate case there would catch this.
| pub fn clean_version_string(ver: &str) -> &str { | ||
| let ver = ver.trim(); | ||
| let ver = ver.strip_prefix('v').unwrap_or(ver); | ||
| ver.split('-').next().unwrap_or(ver) |
There was a problem hiding this comment.
Dropping everything after - makes pre-release targets impossible. assemble 0.26.0-rc.1 writes ext--0.25.9--0.26.0.sql with UPDATE TO '0.26.0' inside, while the crate (and so default_version) says 0.26.0-rc.1. Postgres compares extension versions as opaque strings, so the rc has no upgrade path to itself. semver::Version already parses and orders pre-releases (0.26.0-rc.1 < 0.26.0). I think only the v prefix needs stripping here, and get_existing_sql_targets would then keep the exact target string for the filename.
| let manifest_ver_str = package_manifest.package_version().ok(); | ||
| let manifest_ver = manifest_ver_str.as_deref().and_then(version::parse_version_lossy); | ||
|
|
||
| let (prev_ver, target_ver) = if let Some(latest_target) = |
There was a problem hiding this comment.
This assembles whenever fragments exist, even if sql/ already has a script targeting the manifest version (fragments kept with --preserve-fragments for an rc, or a CI job that ran migrate assemble first). The same fragments then sit twice on the ALTER EXTENSION ... UPDATE path. In ParadeDB's upgrade job, ext--0.25.9--999.0.0.sql is copied, then Assembling unreleased fragments for 999.0.0 -> 999.0.1 follows and default_version is bumped to 999.0.1. Any plain CREATE FUNCTION in a fragment fails on the second pass. Shouldn't this skip (or at least warn) when <ext>--*--<manifest_version>.sql already exists?
One more limit, maybe for the docs: the ephemeral target is always latest + 1. Once a dev database is on 0.25.10, every fragment that lands later still assembles as 0.25.9 -> 0.25.10, and ALTER EXTENSION ... UPDATE becomes a no-op for that database.
| build_base_path(&pg_config, &package_manifest_path, &profile, self.target.as_deref())? | ||
| }; | ||
|
|
||
| let unreleased_mode = if self.assemble_unreleased { |
There was a problem hiding this comment.
install has --no-assemble-unreleased, but package only offers Disallow or assemble. When fragments are preserved on purpose (rc builds), neither fits: the default errors, and --assemble-unreleased stacks them on top of the already assembled script. Is it worth a skip mode here too? For projects where sql/unreleased/ is never empty on main (ParadeDB), the Disallow default also turns every cargo pgrx package on a dev checkout into an error. A warning may be the friendlier default.
|
|
||
| assemble_release_upgrade_script(params)?; | ||
|
|
||
| if self.json { |
There was a problem hiding this comment.
This rebuilds the plan after the fragments were consumed, so --json reports is_empty: true and an empty fragments list for the run that just assembled them. Building the plan once before assemble_release_upgrade_script and printing it here would fix that.
| { | ||
| return Ok(v.to_string()); | ||
| } | ||
| return Ok(cleaned.to_string()); |
There was a problem hiding this comment.
If the manifest version isn't below the target, this still returns it as the predecessor. So assemble 0.25.0 in a crate at 0.26.0 with an empty sql/ yields ext--0.26.0--0.25.0.sql. The error just below seems the safer way to handle that case.
| /// Install without running | ||
| #[clap(long)] | ||
| install_only: bool, | ||
| pub(crate) install_only: bool, |
There was a problem hiding this comment.
Do these need pub(crate) now? Nothing outside this module accesses them.
| std::fs::create_dir_all(root.join("tests").join("pg_regress").join("expected"))?; | ||
| std::fs::create_dir_all(root.join("tests").join("pg_regress").join("sql"))?; | ||
| std::fs::create_dir_all(root.join("sql"))?; | ||
| std::fs::create_dir_all(root.join("sql").join("unreleased"))?; |
There was a problem hiding this comment.
The PR description mentions a .gitkeep, but this only creates the directory, which git won't track. Either drop the mention or write the file.
| 4. Copies the compiled base schema to `my_ext--0.25.1.sql` so `CREATE EXTENSION` installs directly at the new version without traversing upgrade paths. | ||
| 5. Updates `default_version` in the installed `my_ext.control` file. | ||
|
|
||
| Your Git working tree remains completely clean while testing migrations locally. To disable ephemeral assembly during install or run, pass `--no-assemble-unreleased`. |
There was a problem hiding this comment.
What happens to a dev database once the fragments ship? It sits on the ephemeral 0.25.10, the next checkout has no fragments, and sql/ only has 0.25.9--0.26.0. So there is no path from 0.25.10 anywhere, and the DB needs DROP EXTENSION. Fine for a scratch DB, but it's the opposite of the promise in this paragraph, so I'd mention it here.
Fixes #2379. See the issue for background, design rationale, and complete workflow details.
Summary
Introduces first-class support for modular SQL migration fragments in
cargo-pgrxto eliminate merge conflicts on monolithic upgrade scripts, simplify cross-branch cherry-picking, and automate extension upgrade testing.Key Capabilities
sql/unreleased/): Developers commit discrete SQL fragments (<PR>.<slug>.sql) declaring prerequisites (-- depends-on:). Fragments are topologically sorted during assembly.cargo pgrx install,run, andtestassemble fragments into an ephemeral upgrade script and matching base schema directly within Postgres's extension directory, updating the installed.controlfile without dirtying the Git working tree.cargo pgrx migrate assemble): Consolidates unreleased fragments into a permanent release upgrade script (ext--<old>--<new>.sql), deletes consumed fragments, and optionally updatesdefault_versionin the.controlfile (--update-control).cargo pgrx packagefails if unreleased fragments are present unless--assemble-unreleasedis passed, ensuring development fragments do not leak into release distributions.cargo pgrx migrate check/lint): Validates fragment DAGs for cycles, verifies that modifications to unreleased objects declare prerequisites, checks for mixed released/unreleased objects (--deny-mixed-objects), and supports git diff scoping (--base <REF>).cargo pgrx migrate info): Previews predecessor/target versions and ordered fragment plans in text or JSON.Documentation & Scaffolding
docs/src/extension/migrations.mdand documented CLI commands incargo-pgrx/README.md.cargo pgrx newnow scaffoldssql/unreleased/with a.gitkeep.