Great to see this topic being raised, and I agree it's a good time to hash out most of our bulk-mutation atomicity semantics.
One main thing I want to make sure we've rolled into this discussion is all the prior analysis from the "persistence refactor" work about 1.5 years ago: https://docs.google.com/document/d/1U9rprj8w8-Q0SnQvRMvoVlbX996z-89eOkVwWTQaZG0/edit?tab=t.0#heading=h.rg4mbxsmdvk4 In particular, a couple key areas: 1. Directly related to this thread's rpoposal - whether to incorporate lower-level primitives of the "batch update" concept ( https://docs.google.com/document/d/1U9rprj8w8-Q0SnQvRMvoVlbX996z-89eOkVwWTQaZG0/edit?tab=t.0#bookmark=id.5tgsfvekfuf8) in the lower-level BasePersistence - I think the stub paragraph in there about "reflect the commitTransaction use case without using runInTransaction blocks" could relate well to what Dmitri says about "introducing explicit start/end boundaries for each REST API request at the Persistence layer" - essentially if our SPI "collects" the batch of updates and the boundary is the commit/flush, and persistence backends that *do* support atomic-batch can map cleanly. 2. Whether the reasoning for keeping the best-effort/multi-phase decisions for the "special low-evel requirements" that explicitly call out createCatalog/createPrincipal/bootstrap/dropTableWithPurge still hold - factors into the question earlier in this thread about whether the complexity of the real batchUpdate is worth it for the cases that are "good enough" as non-atomic + cleanup. One thing is that it seems like we've taken as a premise in this thread that the pattern TransactionWorkspaceMetaStoreManager uses is broken, with the reasoning being: So a caller of one method on > TransactionWorkspaceMetaStoreManager, after the call succeeds, still cannot > be sure that the change is effective because the "real" change is made only > later, in a batched way, which includes other changes and the batch may > fail as a whole I can be convinced that this may be a problem in the Polaris control flow, *but* I don't agree that it's inherently wrong. We'd have to ask ourselves whether/why the LocalIcebergCatalog *depends* on that "promise" of whether the change is effective, vs whether the final verdict of success occurs at a higher-level API-handler layer. In particular, the idiom of the TransactionWorkspace maps to fairly standard patterns, but I can believe that it diverges in key ways that render the mental model broken. I just don't see those key divergences identified clearly yet. The analogy to underlying RDBMS systems is basically: 1. AtomicMetaStoreManager -- "SET autocommit=1", *and* lacks atomic composite mutation statements - MetaStoreManager methods implicitly convey certain "composite" statements that for AtomicMetaStoreManager are silently decomposed into multiple substatements that are non-atomic across each other 2. TransactionalMetaStoreManager -- "SET autocommit=1" but the implied composite mutation statements from MetaStoreManager interface are effectively internally atomic, and may rely on low-level transactions across actually decomposite multi-statement commits 3. TransactionWorkspaceMetaStoreManager-- "SET autocommit=0/ START TRANSACTION" - things happening between START TRANSACTION and COMMIT don't *locally* know whether they were transactionally flushed or not, but the whole series of high-level mutations *beyond* the implied composite mutation statements now behave as a single atomic unit And it's a common pattern in code then if you have helper functions that do low-level mutations, those helper functions don't need to know whether the top-level driver is using a "SET autocommit=1" or a "START TRANSACTION" -- the multi-statement transactional driver still reuses the low-level code and gets to decide when to actually flush the whole thing. The benefit of this approach is that the core "helper function" is the shared Iceberg-core logic doesn't need to know about multi-table transactions - LocalIcebergCatalog, the BaseTableOperations stuff, etc. We want to minimize having forks in the parts that inherit the Iceberg nuts-and-bolts implementations (the doRefresh/doCommit dance, single-table UpdateRequirements, etc). One example of how this asssumption might *break* is if Icebeg-core itself wants the multi-table semantics to be first-class, *not* just in the Catalog layer. So if a core nominally single-table update can include, for example, an UpdateRequirement tuple that refers to the state of *another* table in the same catalog, then it makes a lot more sense to force the LocalIcebergCatalog layer to be transaction-queueing aware. So if I were to take the devil's advocate stance against my own previous stance of defending the TransactionalWorkspaceMetaStoreManager, I'd say that certain emerging features such as the split between Definer and Invoker views are likely to mor tightly-couple entities that used to behave independently in the shared Iceberg-core layer, supporting the idea that our LocalIcebergCatalog *should* be what explicitly manages the queueing of mutations into a transactional commit set, and a data point against trying to model the shared layer under the same MetaStoreManager APIs as the "autocommit=1" model. That said, if the cross-entity UpdateRequirements are read-only, I'm still not sure it supplants the TransactionWorkspace/autocommit=0 model, because the state-requirements could still be expressible in the per-mutation model like updateEntityIf*Dependencies*NotChanged(entity, crossEntityDependencyVersions) On Wed, Jul 22, 2026 at 3:32 PM Yufei Gu <[email protected]> wrote: > 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 > > > >> > > > > > >> > > > > >> > > > > > > > > > >
