Hi all, Following feedback from @snazy and @dimas-b on https://github.com/apache/polaris/pull/5035, and building on the `MetaStoreChangeSet` / `commitTransactionBatch` direction introduced in PR #4939, I'd like to propose a design for making batch/ChangeSet operations the PRIMARY mutation API in Polaris, with single-entity operations riding on top, not the other way around.
issue: 1. BasePersistence contract says each method is atomic - yet real operations (grant, createCatalog, dropEntity) need multi-call sequences that can partially fail (e.g., grant record written but version bump lost). 2. flush() is a leaky abstraction - PR #5035 tried adding `flush()` to batch writes across SPI calls, but this violates the "no pending changes" contract. Backends that don't need it (NoSQL, EclipseLink) shouldn't see it. 3. TransactionWorkspaceMetaStoreManager is broken. It intercepts individual calls, returns success before persistence, and batches later. As Dmitr noted in this https://lists.apache.org/thread/rf5orxs815zs4h64p4rwp03q3pbgxb5r: "a caller of one method... after the call succeeds, still cannot be sure that the change is effective." 4. Two parallel MetaStoreManager implementations - `AtomicOperationMetaStoreManager` (for JDBC/NoSQL) and `TransactionalMetaStoreManagerImpl` (for EclipseLink) duplicate logic but with different transaction models. Maintenance burden. Proposed Design: "ChangeSet-First" API Core Principle All mutations flow through a single `commitChangeSet(ChangeSet)` path. Single-entity operations (`createEntityIfNotExists`, `grantPrivilege`, etc.) are thin wrappers that build a `ChangeSet` and delegate to `commitChangeSet`. 1. BasePersistence SPI (minimal addition) Add ONE new method to `BasePersistence`: ```java /* * Atomically commit a mixed set of entity mutations and grant-record changes. * Either every change is applied durably and becomes visible together, or none * are applied. Implementations that cannot support mixed atomic commits MUST * throw {@link UnsupportedOperationException}. * * <p>The default implementation throws UnsupportedOperationException, preserving * backward compatibility for existing backends. */ default void commitChangeSet( @NonNull PolarisCallContext callCtx, @NonNull List<EntityMutation> entityMutations, @NonNull List<GrantMutation> grantMutations) { throw new UnsupportedOperationException( "Backend does not support atomic mixed commits; use individual operations"); } ``` Where: - `EntityMutation` = (entity, originalEntity, type: CREATE/UPDATE/DELETE) - `GrantMutation` = (grantRecord, type: CREATE/DELETE) choosing this because: - It keeps `BasePersistence` atomic per-call (no flush, no pending state) - It groups everything that needs to be atomic into ONE call - Backends that can do it (JDBC with transactions, EclipseLink) override it - Backends that can't (simple KV) keep the default and callers use fallback 2. PolarisMetaStoreManager (reformulate all mutations) `commitTransactionBatch(MetaStoreChangeSet)` becomes THE method. All other mutation methods get default implementations in terms of it: ```java // ALREADY EXISTS in PR #4939 - we expand it default @NonNull EntitiesResult commitTransactionBatch( @NonNull PolarisCallContext callCtx, @NonNull MetaStoreChangeSet changeSet) { // fallback: call individual methods in sequence (NOT atomic) for (EntityWithPath create : changeSet.creates()) { ... } for (EntityWithPath update : changeSet.updates()) { ... } } // NEW: Single-entity operations ride on top of commitTransactionBatch default @NonNull EntityResult createEntityIfNotExists( @NonNull PolarisCallContext callCtx, @Nullable List<PolarisEntityCore> catalogPath, @NonNull PolarisBaseEntity entity) { return commitTransactionBatch(callCtx, MetaStoreChangeSet.ofCreate(catalogPath, entity)) .toSingleResult(); } default @NonNull EntitiesResult updateEntitiesPropertiesIfNotChanged( @NonNull PolarisCallContext callCtx, @NonNull List<EntityWithPath> entities) { return commitTransactionBatch(callCtx, MetaStoreChangeSet.ofUpdates(entities)); } ``` Then `AtomicOperationMetaStoreManager` ONLY overrides `commitTransactionBatch`: ```java @Override public @NonNull EntitiesResult commitTransactionBatch( @NonNull PolarisCallContext callCtx, @NonNull MetaStoreChangeSet changeSet) { BasePersistence ms = callCtx.getMetaStore(); // Build entity mutations (creates + updates) List<EntityMutation> entityMutations = buildEntityMutations(changeSet); // Build grant mutations from the ChangeSet context (if any) List<GrantMutation> grantMutations = buildGrantMutations(changeSet); if (ms.supportsAtomicMixedCommit()) { // JDBC path: one transaction, everything atomic ms.commitChangeSet(callCtx, entityMutations, grantMutations); } else { // Fallback: sequence of individual operations (eventual consistency) return super.commitTransactionBatch(callCtx, changeSet); } } ``` 3. AtomicOperationMetaStoreManager refactor Move ALL complex operations (grant, revoke, createCatalog, dropEntity) to build a `ChangeSet` and call `commitTransactionBatch`: Example: grantPrivilege ```java // OLD: 3 separate persistence calls, no atomicity ms.writeToGrantRecords(callCtx, grantRecord); ms.writeEntity(callCtx, updatedGrantee, false, granteeEntity); ms.writeEntity(callCtx, updatedSecurable, false, securableEntity); // NEW: build ChangeSet, commit once MetaStoreChangeSet changeSet = MetaStoreChangeSet.builder() .addUpdate(granteeEntity, updatedGrantee) .addUpdate(securableEntity, updatedSecurable) .addGrant(grantRecord) .build(); return commitTransactionBatch(callCtx, changeSet); ``` Example: dropEntity ```java // OLD: delete entity, then delete grants, then bump versions — 3+ calls // NEW: build ChangeSet with deletes + updates, commit once MetaStoreChangeSet changeSet = MetaStoreChangeSet.builder() .addDelete(entity) .addDeletes(grantRecords) .addUpdates(relatedEntitiesWithBumpedVersions) .build(); return commitTransactionBatch(callCtx, changeSet); ``` 4. JdbcBasePersistenceImpl Implement `commitChangeSet` using a single transaction: ```java @Override public void commitChangeSet( @NonNull PolarisCallContext callCtx, @NonNull List<EntityMutation> entityMutations, @NonNull List<GrantMutation> grantMutations) { datasourceOperations.runWithinTransaction(connection -> { for (EntityMutation em : entityMutations) { switch (em.type()) { case CREATE -> persistEntity(connection, em.entity(), null); case UPDATE -> persistEntity(connection, em.entity(), em.originalEntity()); case DELETE -> deleteEntity(connection, em.entity()); } } for (GrantMutation gm : grantMutations) { switch (gm.type()) { case CREATE -> persistGrantRecord(connection, gm.grantRecord()); case DELETE -> deleteGrantRecord(connection, gm.grantRecord()); } } return true; }); } ``` No ThreadLocal. No flush(). Just one transaction per `commitChangeSet` call. 5. TransactionWorkspaceMetaStoreManager We can DEPRECATE IT. The "accumulate in memory, commit once" pattern moves to the CALLER (e.g., IcebergCatalogHandler for commitTransaction): ```java // In IcebergCatalogHandler MetaStoreChangeSet.Builder builder = MetaStoreChangeSet.builder(); for (TableCommit commit : commits) { builder.addUpdate(commit.oldEntity(), commit.newEntity()); } return metaStoreManager.commitTransactionBatch(callCtx, builder.build()); ``` No more "workspace that pretends to persist but doesn't." The caller builds the ChangeSet explicitly. Implementation Plan if this is fine :) 1. Phase 1 (this PR): Add `commitChangeSet` to `BasePersistence`, expand `MetaStoreChangeSet` to include deletes and grants, refactor `AtomicOperationMetaStoreManager` to use `commitTransactionBatch` for grant operations + createCatalog + dropEntity. 2. Phase 2 (follow-up): Refactor remaining single-entity methods to default to `commitTransactionBatch`. Deprecate `TransactionWorkspaceMetaStoreManager`. 3. Phase 3 (follow-up): Unify `AtomicOperationMetaStoreManager` and `TransactionalMetaStoreManagerImpl` - both use the same `commitTransactionBatch` path, just with different `BasePersistence` backends. I have a few queries as well: 1. Should `commitChangeSet` live on `BasePersistence` or a new sub-interface? (I lean toward `BasePersistence` with a default throw.. simplest migration.) 2. Should we keep `writeEntities` or deprecate it in favor of `commitChangeSet`? (I lean toward keep but document that `commitChangeSet` is preferred for mixed operations.) 3. How do we handle the `PolarisMetaStoreManager` methods that do reads before writes (e.g., `grantPrivilege` loads grantee/securable before updating)? (I lean toward: reads happen in `AtomicOperationMetaStoreManager`, then the computed ChangeSet is committed. Reads are not part of the atomic boundary.) Looking forward to feedback! On Tue, Jul 14, 2026 at 3:21 AM Prithvi S <[email protected]> wrote: > Hi Robert, > > Makes sense👍, thanks for checking this. I've dropped > writeEntitiesAndGrantRecords from BasePersistence entirely. Instead added > a small flush() default no-op. JDBC now batches writes internally and the > manager calls flush() after atomic sequences. NoSQL is unaffected.. keeps > the SPI clean :) > > Please take another look when you can > https://github.com/apache/polaris/pull/5035 > > Regards, > Prithvi > > On Mon, Jul 13, 2026 at 7:46 PM Robert Stupp <[email protected]> wrote: > >> Hi Prithvi, >> >> thanks for your contribution. >> >> I agree that the current create-catalog path for JDBC persistence is not >> atomic. >> For NoSQL, create-catalog already already guarantees the important >> invariant: >> the catalog is only made visible after the catalog admin role and initial >> grants >> have been successfully created. >> >> So I think this deserves a narrower fix for the JDBC persistence path, >> rather >> than a broad BasePersistence SPI change. >> >> Orthogonally, Polaris is moving toward treating built-in RBAC as one >> authorization implementation among others. That makes me hesitant to add >> new >> generic persistence SPI methods that are specifically shaped around >> built-in >> RBAC grant records. >> >> Robert >> >> >> On Sat, Jul 11, 2026 at 1:30 AM Prithvi S <[email protected]> >> wrote: >> >> > Hi all, >> > >> > FYR, I had to change the branch, so I created a new PR and closed the >> > mentioned PR. Please check this >> > https://github.com/apache/polaris/pull/5035 >> > instead of the mentioned PR ( >> https://github.com/apache/polaris/pull/5032) >> > in the discussion. >> > >> > Thanks! >> > Prithvi S >> > >> > On Sat, Jul 11, 2026 at 3:35 AM Prithvi S <[email protected]> >> > wrote: >> > >> > > Hi all, >> > > >> > > I’d like to open discussion on hardening partial-commit windows in the >> > > atomic metastore path (AtomicOperationMetaStoreManager + >> > BasePersistence). >> > > >> > > a little background, >> > > BasePersistence requires each SPI method to be atomic, but several >> > manager >> > > flows still compose multiple SPI calls: >> > > 1. Grant / revoke - write/delete a grant row, then separately CAS-bump >> > > grant_records_version on grantee and securable >> > > 2. createCatalog - create catalog + admin role + several grants as a >> > > sequence of writes >> > > 3. dropEntity - delete entity, delete grants, bump partner versions as >> > > separate steps >> > > >> > > If the server fails mid-sequence, we can leave partial state (grant >> > > without version bumps, catalog without admin role/grants, etc.). The >> code >> > > already documents some of this as acceptable eventual consistency / >> “drop >> > > and recreate,” with TODOs asking for bulk update of grants + entity >> > > versions. >> > > >> > > I opened a draft implementation to make the problem concrete: >> > > https://github.com/apache/polaris/pull/5032 >> > > >> > > It adds BasePersistence.writeEntitiesAndGrantRecords(...) (entity >> > > creates/updates with per-row CAS, entity deletes, grant >> inserts/deletes >> > in >> > > one all-or-nothing op) and migrates grant/revoke, createCatalog, and >> > > dropEntity in AtomicOperationMetaStoreManager to use it. >> > > >> > > before pushing this further (or reshaping it), I’d like the >> community’s >> > > view on the approach: >> > > >> > > 1. Is extending BasePersistence the right place? >> > > Is a first-class “entities + grants in one atomic op” method the >> > preferred >> > > contract for backends (JDBC today, others later), or should this stay >> > > backend-local / optional? >> > > >> > > 2. Scope of a first change >> > > Would you rather see >> > > • (A) Narrow first PR: only grant/revoke (highest concurrency / >> > > cache-invalidation impact, smallest SPI surface), or >> > > • (B) Broader SPI + migrate createCatalog / drop in the same change >> (what >> > > #5032 currently does), or >> > > • (C) SPI + tests only first, call-site migration in follow-ups? >> > > >> > > 3. API shape >> > > Six parallel lists (entitiesToWrite, originals, deletes, grants to >> > > write/delete) is simple but easy to misuse. Prefer a small structured >> > > batch/commit type instead? >> > > >> > > 4. What must be in-scope vs out-of-scope for “atomic” >> > > Even with this SPI, some windows remain intentionally outside (e.g. >> > > storage integration create, principal secrets delete, policy-mapping >> > > cleanup, cleanup task scheduling). Is that acceptable for v1, or >> should >> > the >> > > contract cover more? >> > > >> > > 5. createCatalog specifically >> > > The existing comments treat partial catalog init as recoverable via >> drop. >> > > Is full atomic create worth the complexity (pre-computed >> > > grant_records_version, mixed create+CAS on principal roles), or is >> > > grant/revoke enough for now? >> > > >> > > Regards, >> > > Prithvi S >> > > >> > >> >
