Hi Prithvi and All, This is a good discussion!
I'd like to highlight a different aspect, though. With the atomic MetaStore approach (one persistence method = one atomic change) approach, we cannot establish any expectations about reads that happen between changes. In many cases, assuming a JDBC backend, the end-to-end request execution logic appears to assume that the RDBMS operates at the Serializable isolation level. Each individual entity has optimistic locking checks, which may look like Serializable isolation level is not required, but if we consider that Polaris reads and processes entity properties for some unchanged entities, per-entity optimistic locking does not seem sufficient. I'd like to propose introducing explicit start/end boundaries for each REST API request at the Persistence layer. In JDBC, these boundaries will clearly map to transaction start/commit and allow reads and writes to be reconciled by the RDBMS (i.e. enforce Serializable isolation). A side effect of this is that Polaris will probably have to support retries for transient persistence errors like Serializability failures. Another side effect is that with this approach, idempotency data could be stored in a separate table and still participate in the JDBC transaction that changes entities. I do not have any concrete code changes in mind yet. This is for discussion. WDYT? Thanks, Dmitri. On Tue, Jul 14, 2026 at 6:20 PM Prithvi S <[email protected]> wrote: > 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 > >> > > > >> > > >> > > >
