Thanks for raising this, Prithvi! The examples you shared are valid cases where we want to ensure consistency. As I mentioned in another thread, a lot of them are known issues, and `TransactionalMetaStoreManagerImpl` already handles most of these cases well.
> 1. Is extending `BasePersistence` the right place? I think so. A good example is the `TransactionalPersistence` interface, which extends `BasePersistence` by introducing methods for transactional operations. 2. I think starting with a narrow PR first makes sense, for example, focusing on grant/revoke. A smaller scope is usually easier to review and tends to result in a higher quality PR. 3. In terms of the API shape, I don't think there's any urgency to introduce an abstraction yet, but I'm open to introducing something like a `ChangeSet` to group related changes. It could be similar to the `TableCommitActions` class introduced in this PR: https://github.com/apache/polaris/pull/5088. 4. Same as 3. I don't think we need to decide this now. 5. I think it's worth wrapping this in a transaction. Please take a look at the existing implementation here: https://github.com/apache/polaris/blob/b81766a70575b3c1ba122c2b7f389f591a9c7c53/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java#L965 Yufei On Tue, Jul 14, 2026 at 7:14 PM Dmitri Bourlatchkov <[email protected]> wrote: > 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 > > >> > > > > >> > > > >> > > > > > >
