JDBC: Use single DDL init script - #5349
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates JDBC initialization around a single database-specific schema.sql, removing schema-version selection and legacy DDL resources.
Changes:
- Removes schema-version bootstrap options, models, and branching.
- Consolidates H2, PostgreSQL, and CockroachDB schemas.
- Updates persistence code, tests, and integrations.
Unresolved findings include existing-schema compatibility failures, ineffective configuration behavior, stale documentation, and a test compilation error.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Reviewed change |
|---|---|
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java |
Removes schema-version bootstrap configuration. |
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java |
Removes schema options API. |
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/BootstrapOptions.java |
Removes schema options from bootstrap configuration. |
persistence/relational-jdbc/src/test/resources/h2/schema-v0.sql |
Removes legacy H2 fixture. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/SchemaVersions.java |
Removes schema discovery utility. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java |
Updates query tests. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/models/ModelEventTest.java |
Updates event conversion tests. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcEventsPersistenceTest.java |
Updates event persistence tests. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java |
Removes obsolete bootstrap tests. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImplTest.java |
Updates persistence tests; remaining schema-version arguments cause compilation failure. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperationsTest.java |
Updates model construction. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java |
Removes schema-version setup. |
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplPostgresSchemaIT.java |
Uses the unified schema. |
persistence/relational-jdbc/src/main/resources/postgres/schema.sql |
Defines the consolidated PostgreSQL schema. |
persistence/relational-jdbc/src/main/resources/postgres/schema-v4.sql |
Removes legacy PostgreSQL schema. |
persistence/relational-jdbc/src/main/resources/postgres/schema-v3.sql |
Removes legacy PostgreSQL schema. |
persistence/relational-jdbc/src/main/resources/postgres/schema-v2.sql |
Removes legacy PostgreSQL schema. |
persistence/relational-jdbc/src/main/resources/postgres/schema-v1.sql |
Removes legacy PostgreSQL schema. |
persistence/relational-jdbc/src/main/resources/h2/schema.sql |
Defines the consolidated H2 schema. |
persistence/relational-jdbc/src/main/resources/h2/schema-v4.sql |
Removes legacy H2 schema. |
persistence/relational-jdbc/src/main/resources/h2/schema-v3.sql |
Removes legacy H2 schema. |
persistence/relational-jdbc/src/main/resources/h2/schema-v2.sql |
Removes legacy H2 schema. |
persistence/relational-jdbc/src/main/resources/h2/schema-v1.sql |
Removes legacy H2 schema. |
persistence/relational-jdbc/src/main/resources/cockroachdb/schema.sql |
Defines the consolidated CockroachDB schema. |
persistence/relational-jdbc/src/main/resources/cockroachdb/schema-v4.sql |
Removes legacy CockroachDB schema. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java |
Removes schema-version query parameters. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/SchemaVersion.java |
Removes the schema-version model. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelEvent.java |
Removes legacy event-write branching; existing v3/v4 schemas may reject writes. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelEntity.java |
Uses current entity columns; existing v1 schemas lack a required column. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java |
Bootstraps with one schema; the fallback configuration is no longer effective. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java |
Removes schema selection utilities. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java |
Removes schema-version persistence logic. |
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java |
Resolves a single schema resource; migration documentation remains stale. |
extensions/metrics-reports/persistence/relational-jdbc/src/test/java/org/apache/polaris/extension/metrics/jdbc/JdbcMetricsPersistenceTest.java |
Uses the consolidated schema fixture. |
Suppressed comments (1)
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:143
- This now runs the idempotent
schema.sqlagainst every bootstrap, but that script only creates missing objects; it does not upgrade existing v1/v2 tables to addlocation_without_schemeor existing v3/v4eventstables to makecatalog_idnullable. The version-aware compatibility path was removed at the same time, so an un-migrated existing database will fail entity queries or event writes (the newModelEventsends SQL NULL). Please retain legacy/migration handling or reject old schemas with an actionable upgrade requirement.
datasourceOperations.executeScript(
datasourceOperations.getDatabaseType().openInitScriptResource());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .grantRecordsVersion(r.getInt("grant_records_version")) | ||
| .locationWithoutScheme( | ||
| this.schemaVersion >= 2 ? r.getString("location_without_scheme") : null) | ||
| .locationWithoutScheme(r.getString("location_without_scheme")) |
There was a problem hiding this comment.
The idea is to NOT retain old schemas. Polaris does not offer migration scripts (yet?), so we expect users to migrate the schema before upgrading Polaris. This hasn't changed. The changelog notices may help by indicating how to evolve the schema.
| static ModelEvent fromEvent(EventEntity event) { | ||
| if (event == null) return null; | ||
|
|
||
| String catalogId = event.getCatalogId(); | ||
| if (schemaVersion < 5 && catalogId == null) { | ||
| // Schema versions < 5 declare events.catalog_id NOT NULL; store the legacy sentinel there. | ||
| catalogId = LEGACY_REALM_SCOPED_CATALOG_ID; | ||
| } | ||
| return ImmutableModelEvent.builder() | ||
| .catalogId(catalogId) | ||
| .catalogId(event.getCatalogId()) |
There was a problem hiding this comment.
Same here: we expect users to migrate the schema before upgrading Polaris.
| Polaris does not run automated schema migrations. Each Polaris binary ships a single `schema.sql` | ||
| for each supported database type; bootstrapping executes that script with `CREATE TABLE IF NOT | ||
| EXISTS` and `CREATE INDEX IF NOT EXISTS` statements, so it is safe to run on every startup without | ||
| risk of data loss. |
There was a problem hiding this comment.
With-out safe fall-back, should we add wording around schema changes introduces by a newer release must be applied manually before upgrading?
| UPDATE polaris_schema.version SET version_value = 5 WHERE version_key = 'version'; | ||
| ``` | ||
| There is no support for bootstrapping a newer Polaris binary against an older schema version. | ||
| If you need the old schema, use the matching older Polaris binary. |
There was a problem hiding this comment.
Should we add section for upgrading from older version to the newer release as couple DDL are must-have due to lack of fall-back such as following:
ALTER TABLE polaris_schema.events ALTER COLUMN catalog_id DROP NOT NULL;
UPDATE polaris_schema.events SET catalog_id = NULL WHERE catalog_id = '__realm__';
|
Hello @adutra , I had been really busy for the last couple of weeks and finally got time to catch up on this thread/change and thanks @flyrain for tagging. Couple things I would like to confirm:
Thanks, |
|
Hi @MonkeyCanCode thank you for the thorough review! Your concerns are spot on; here is what I modified:
PTAL 🙏 |
0843d1c to
d51736c
Compare
| } | ||
|
|
||
| @VisibleForTesting | ||
| static PreparedQuery generateEntityTableExistQuery() { |
40ad548 to
d65679c
Compare
d65679c to
aa9f003
Compare
|
FYI I had to rebase a few times to fix conflicts with other PRs, but this should be reviewable again. |
Hello @adutra , I will take another look tomorrow. Thanks, |
| -- CockroachDB schema v5 (matching PostgreSQL schema v5) | ||
| -- Schema version is kept in sync with PostgreSQL to ensure correct column selection in ModelEntity. | ||
| -- Changes from v4: | ||
| -- * Removed the `idempotency_records` table (the durable idempotency store it | ||
| -- backed was never wired into any request path and has been removed) | ||
| -- * `events.catalog_id` is nullable; events that are not catalog-scoped store NULL (issue #4674) |
There was a problem hiding this comment.
[doubt] i wonder having historical schema in the code base helped in readability and users identifying what changed across version for example in v5 ... this is what changed from v4 ....
How do we feel on this aspect, i understand the motivation of this change and i think its reasonable, just brining this aspect from readibilty and book-keeping to know your thoughts ?
There was a problem hiding this comment.
I think that this aspect has now moved to git history. Accumulating comments since v1 on the same script would quickly become visual clutter after a dozen versions.
There was a problem hiding this comment.
Thanks @adutra for tackling this! I fully support removing the complex runtime logic around bootstrapping historical schemas.
However, I do see significant value in keeping the explicit versioned schema files in the release repository. When users perform multi-version upgrades (e.g., upgrading directly from 1.1 to 1.7), comparing schema differences between major versions is critical for planning migrations safely. Relying on Git history to piece together intermediate schema states across multiple releases can be cumbersome and error-prone(for example, users don't care the schema changes within a version), whereas shipping explicit versioned schema files gives operators a clean, direct diff.
Given that maintaining these static SQL files adds relatively low overhead compared to the clarity they provide for upgrades, I think it's a trade-off worth considering. What do you think?
There was a problem hiding this comment.
I mentioned git history for archeological search mostly, but from a user perspective, the idea is that we are now providing the full migration steps in the docs, see relational-jdbc.md in this PR:
I think that is a far superior UX since users don't need to bother comparing schema differences, neither by diffing files or through git log. And it's not a huge burden for committers to add a small SQL snippet there each time they modify the current schema, so I'd say it's a win-win approach.
If we keep versioned schema files in-tree, that imho beats the purpose of unifying DDL init scripts, and this PR becomes a moot point.
WDYT?
There was a problem hiding this comment.
I am fine with both. But I do like the change @adutra posted above, it is a lot easier to read and prep for an upgrade. There is an extra sql for v4 to v5 upgrade which is not documented in https://polaris.apache.org/releases/1.7.0/metastores/relational-jdbc/#upgrading-to-schema-v5:
DROP TABLE IF EXISTS idempotency_records;
Should we fix the doc as well?
There was a problem hiding this comment.
Yes. No related to this. I will raise a PR to fix existed doc tonight and tag u for review.
There was a problem hiding this comment.
If we keep versioned schema files in-tree, that imho beats the purpose of unifying DDL init scripts, and this PR becomes a moot point.
I agree that one current schema.sql should be the only bootstrap input, but that does not conflict with separate migration artifacts. schema.sql initializes an empty database, while a migration transforms persisted state and is now the only supported upgrade path. Could we keep each migration as executable SQL and run it in CI against a populated fixture from the released schema? The docs can remain the operator-facing UX, but the SQL they show would come from an upgrade path we actually exercise.
There was a problem hiding this comment.
that we are now providing the full migration steps in the docs, see relational-jdbc.md i
+1 to this, @adutra to keeping this as manual instructions, which could prompt user to take apt actions we kind of did this when we introduced a backward incompatible change https://github.com/apache/polaris/pull/2674/changes
having this automated via CLI could next step which could be desirable to folks upgrading but we need to think this more as just updating schema might not be a complete migration script.
on an orthogonal note i would still say its good to have these versions, may be we can move this to historical and keep current-schema, if someone wants to see the complete stuff what was bootstrapped, with that being said i will leave it to better judgement of the community !
There was a problem hiding this comment.
It's a good idea to document the diff or the migration steps in our website. Thanks @adutra for adding it! I still think the versioned file should be kept for users to refer, to reason, or to build their own migration tools. They are serving as the source of truth for the schema changes without worrying about any unsync in the doc.
37e09dc to
98b075e
Compare
|
FYI: rebased and squashed again due to the merge of #4945 . |
|
|
||
| This implementation leverages Quarkus for datasource management and supports configuration through | ||
| environment variables or JVM -D flags at startup. For more information, refer to the [Quarkus configuration reference](https://quarkus.io/guides/config-reference#env-file). | ||
| The Relational JDBC metastore relies on a Quarkus-managed datasource. For more information, refer to |
There was a problem hiding this comment.
FYI, I seized the opportunity for a complete overhaul of this page, around the following structure:
- Configuration
- SQL Schema: what
schema.sqlis, the version table, schema naming - Setting Up a Fresh Deployment (create schema + boostrap)
- Upgrading Polaris: apply migration + optionally bootstrap; migration SQL snippets per version
- Schema Version Reference: a mapping table of schema versions to Polaris versions
d9f4bd6 to
73bdffe
Compare
effe05c to
81620be
Compare
|
|
||
| For this release ({{< releaseVersion >}}), the corresponding SQL files are: | ||
|
|
||
| * PostgreSQL: [schema.sql]({{< github-polaris "persistence/relational-jdbc/src/main/resources/postgres/schema.sql" >}}) |
There was a problem hiding this comment.
FYI I discovered that the github-polaris shortcode has a bug, it's being fixed here: #5411
|
|
||
| ```sql | ||
| ALTER TABLE events ALTER COLUMN catalog_id DROP NOT NULL; | ||
| UPDATE events SET catalog_id = NULL WHERE catalog_id = '__realm__'; |
There was a problem hiding this comment.
This can be a huge change set... potentially 🤔 However, the idea is clear and should allow the admin user to figure out what to do.
|
@flyrain and @MonkeyCanCode are you OK merging this PR? Thanks! |
Hello @adutra , Sorry for the delay, had been crazy busy on my end recently. No concern with this PR but a NIT a commented couple days back. I will fix that in the public page later this weekend. Thanks, |
|
Will take a look today. Thanks for ping me, @adutra ! |
flyingImer
left a comment
There was a problem hiding this comment.
I'm aligned with removing the version-dependent runtime paths, but I don't think the current upgrade contract is merge-ready. I left two blocking migration comments because the documented path can fail or delete existing data. I also left two narrower comments on CockroachDB coverage and lifecycle wording.
| ### Migration From Schema v2 to v3 | ||
|
|
||
| ```sql | ||
| DROP TABLE IF EXISTS policy_mapping_record; |
There was a problem hiding this comment.
This migration deletes all existing policy mappings. IIRC, the released v2 and v3 definitions of policy_mapping_record are unchanged, and v3 removes this DROP while adding the events table. The DROP belongs to a full bootstrap script, not a state-preserving migration. Could we preserve the existing rows here and cover that with an upgrade test?
| CREATE INDEX IF NOT EXISTS idx_locations | ||
| ON entities USING btree (realm_id, parent_id, location_without_scheme) | ||
| WHERE location_without_scheme IS NOT NULL; | ||
| UPDATE version SET version_value = 2 WHERE version_key = 'version'; |
There was a problem hiding this comment.
IIUC, databases bootstrapped by the released 1.0.0 and 1.0.1 v1 scripts do not have the version table. This UPDATE therefore fails after the preceding DDL and leaves the migration without recording v2. Should we create and initialize the version record as part of this transition for those released schemas?
|
|
||
| The SQL statements below are for PostgreSQL; other databases may require syntax changes. | ||
|
|
||
| ### Migration From Schema v5 to v6 |
There was a problem hiding this comment.
This is more than a syntax difference for CockroachDB. Its v5 to v6 upgrade also needs three indexes currently listed only in CHANGELOG, including the grant index intended to avoid realm-wide scans. Since CHANGELOG links this page as the full upgrade path, I feel we should include a complete CockroachDB transition here before recording v6?
| ``` | ||
| H2 is unaffected. | ||
| - Relational JDBC: The per-schema-version runtime fallback has been removed. The migration to schema | ||
| v6 is now **required** before starting this version of Polaris. Polaris will fail to start with a |
There was a problem hiding this comment.
With the default relational service configuration, compatibility is validated on the first request to each realm, not during startup. relational-jdbc.md already describes that lifecycle. Could we align this upgrade note and the PR description with it, or move the check into startup if fail-to-start is the intended guarantee?
Per-database-type DDL scripts (`schema-v1.sql` through `schema-v6.sql`) are replaced by a single `schema.sql` per database type. Bootstrap always runs this script idempotently (`CREATE TABLE IF NOT EXISTS` / `CREATE INDEX IF NOT EXISTS`), so there is no longer any need for version-aware fallback logic at runtime. The runtime fallback, `SchemaVersion`, `SchemaOptions`, `SchemaVersions`, and `JdbcBootstrapUtils` are removed. In their place, a fail-fast `validateSchemaCompatibility()` check reads `POLARIS_SCHEMA.VERSION` at startup and throws an actionable `IllegalStateException` if the recorded version does not match `CURRENT_SCHEMA_VERSION`, rather than silently degrading. The relational-jdbc documentation is extended with a full schema-version history table and step-by-step migration SQL for every transition from v1 (Polaris 1.0.0-incubating) through v6 (this release).
822e950 to
6f26a55
Compare
|
|
||
| ```sql | ||
| DROP INDEX IF EXISTS idx_idemp_realm_expires; | ||
| DROP TABLE IF EXISTS idempotency_records; |
There was a problem hiding this comment.
This should be DROP INDEX.
Context: https://lists.apache.org/thread/jcl1ctf9sr1fkbw0lhsgjgc2s8j4b98o
Per-database-type DDL scripts (
schema-v1.sqlthroughschema-v6.sql)are replaced by a single
schema.sqlper database type. Bootstrap alwaysruns this script idempotently (
CREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS), so there is no longer any need forversion-aware fallback logic at runtime.
The runtime fallback,
SchemaVersion,SchemaOptions,SchemaVersions,and
JdbcBootstrapUtilsare removed. In their place, a fail-fastvalidateSchemaCompatibility()check readsPOLARIS_SCHEMA.VERSIONatstartup and throws an actionable
IllegalStateExceptionif the recordedversion does not match
CURRENT_SCHEMA_VERSION, rather than silentlydegrading.
The relational-jdbc documentation is extended with a full schema-version
history table and step-by-step migration SQL for every transition from
v1 (Polaris 1.0.0-incubating) through v6 (this release).
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)