feat(persistence): replace EventEntity.REALM_SCOPED sentinel with nullable catalog_id#4981
feat(persistence): replace EventEntity.REALM_SCOPED sentinel with nullable catalog_id#4981sririshindra wants to merge 2 commits into
Conversation
b8d56d3 to
d2e5d4e
Compare
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @sririshindra !
This PR LGTM in general, but let's collect some more reviews as it's a major change.
Do you feel like also starting a dev discussion for visibility?
| case POSTGRES -> 4; // PostgreSQL has schemas v1, v2, v3, v4 | ||
| case COCKROACHDB -> 4; // CockroachDB schema version kept in sync with PostgreSQL | ||
| case H2 -> 4; // H2 uses same schemas as PostgreSQL | ||
| case POSTGRES -> 5; // PostgreSQL has schemas v1, v2, v3, v4, v5 |
There was a problem hiding this comment.
side note: it is unfortunate that changes to events tables require a schema version bump that also affects metastore tables 🤷 Would it be nicer to evolve events schema and metastore scheme separately (not in this PR, but in general)? WDYT?
There was a problem hiding this comment.
Makes sense @dimas-b , I will send an email to the dev mailing list.
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| ### Highlights | ||
|
|
||
| ### Upgrade notes | ||
| - Relational JDBC: a new schema version 5 makes the `events.catalog_id` column nullable. Events that |
There was a problem hiding this comment.
I'm wondering how long should we support old schemas? With our support rolling upgrades policy (https://polaris.apache.org/releases/1.6.0/evolution/#managing-polaris-database), after we release 1.7 with this change, can we eliminate the sentinel completely in 1.9?
There was a problem hiding this comment.
I 've been meaning to start this thread on dev for a long time, but never find a break to just write that email 😅
Feel free to initiate this conversation.
sririshindra
left a comment
sririshindra
left a comment
There was a problem hiding this comment.
Thanks for the reviews, @dimas-b and @nandorKollar . I will send a follow up email to the dev mailing list as suggested.
|
Related |
|
@sririshindra : Please resolve merge conflicts |
…lable catalog_id Events not scoped to a catalog (principal, policy, rate-limiting, etc.) previously stored the magic string "__realm__" in events.catalog_id because the column was NOT NULL. Introduce relational-JDBC schema v5 with a nullable events.catalog_id and persist SQL NULL instead: - Remove the REALM_SCOPED constant from EventEntity; catalogId is now @nullable end to end (entity, listener, ModelEvent). - Add schema-v5.sql for postgres/h2/cockroachdb and bump the latest schema version to 5. - Keep a version-gated shim in JdbcBasePersistenceImpl.writeEvents that writes the legacy sentinel on pre-v5 schemas (no automated migrations), and coalesce "__realm__" back to null on read. - Document the one-time operator migration in CHANGELOG.md and the relational-jdbc metastore docs. Fixes apache#4674
…nto ModelEvent.fromEvent Follow the ModelEntity.fromEntity(entity, schemaVersion) pattern so both halves of the sentinel round-trip (write substitution and read coalescing) live in ModelEvent, and drop the redundant second immutable build in the event write path. Also document that the sentinel string stays reserved on all schema versions, and fix the copied changelog headers in the v5 schema files to state the actual v4 -> v5 delta.
e632926 to
3ea3437
Compare
|
I'm planning to merge on July 19 unless concerns are raised in GH or of the |
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM Thanks @sririshindra !
| CockroachDB, and H2), then restart Polaris: | ||
|
|
||
| ```sql | ||
| ALTER TABLE polaris_schema.events ALTER COLUMN catalog_id DROP NOT NULL; |
There was a problem hiding this comment.
Small doc nit: this block is presented as working on PostgreSQL, CockroachDB, and H2, but ALTER COLUMN ... DROP NOT NULL is the Postgres/Cockroach spelling — H2 historically uses ALTER COLUMN catalog_id SET NULL. The "adjust the ALTER syntax to your database if needed" hedge covers it, and H2 is effectively test-only, so this is optional — but if you want it copy-pasteable, worth naming H2's variant. (I couldn't verify the exact H2 version's syntax here, so flag if this is already fine.)
There was a problem hiding this comment.
This PR has been in review for a long time 😅 Given the low impact, let's merge "as is". Follow-up doc improvements are welcome, of course.
|
@sririshindra : A v5 schema was introduced via #5086. Sorry about conflicts on your PR but the other one merged first 🤷 Please adjust current v5 DDL according to this PR (no need to add v6 since this scheme version has not been released yet). |
5 participants
Events that are not scoped to a catalog (principal, principal-role, policy, credential,
transaction, config, rate-limiting, etc.) previously persisted the magic string
"__realm__"in the
events.catalog_idcolumn because that column wasNOT NULL. This replaces the sentinelwith a proper SQL
NULL.Changes
REALM_SCOPEDconstant fromEventEntity;catalogIdis now@Nullableend toend (entity, listener,
ModelEvent).events.catalog_id,and bumped
DatabaseType.getLatestSchemaVersion()to 5.JdbcBasePersistenceImpl.writeEventsthatstill writes the legacy
"__realm__"sentinel whenschemaVersion < 5(Polaris has no automatedschema migrations), and coalesces the sentinel back to
nullon read inModelEvent.fromResultSet.CHANGELOG.mdand the relational-jdbc metastore docs with the one-time operatormigration SQL for existing v3/v4 deployments.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)