Hi Qiegang, thanks a lot for the fix! Sorry for the late reply. My main concern with #5820 is that it introduces conflicts between transactions that should be able to succeed independently. For example: 1. T1 updates table A and checks that table C is unchanged. 2. T2 updates table B and checks that table C is unchanged.
Neither transaction actually changes C, so both should succeed. With #5820, however, each transaction bumps C's entity version, causing the other to fail its version check. If many transactions share the same dependency, this could lead to unnecessary conflicts and retries. I think this is the strongest reason to prefer checking these conditions atomically without writing the entities. I'm leaning toward #5961 for that reason, though we should still review the SPI design and compatibility implications. Please let us know when your PR is ready for review. Yufei On Tue, Nov 3, 2026 at 11:41 AM Qiegang Long <[email protected]> wrote: > > Hi all, > > I'd like to discuss an addition to the metastore SPI, proposed in > https://github.com/apache/polaris/pull/5961 to fix > https://github.com/apache/polaris/issues/5807. CONTRIBUTING asks for > extension-point changes to be agreed here first, so I've marked the PR > as a draft for now. > > THE PROBLEM > > A multi-table commit can list a table with requirements but no updates, > to assert that the table is unchanged. Polaris validates those > requirements, but such a table is not part of the atomic write. So two > concurrent commits that each change one table and assert the other can > both succeed (write skew). The issue has a reproduction script; it also > reproduces on relational-jdbc with PostgreSQL. > > The root cause is in the SPI: the only condition a write can express is > "the entity I am writing is still at version v". There is no way to say > "this entity I only read is unchanged" in the same atomic write. > > This looks like a common pattern in the metastore, not specific to #5807: > AtomicOperationMetaStoreManager, which backs relational-jdbc, has 18 > TODOs noting that a check and its write are not atomic (parent paths on > create and update, children on drop, rename targets). > > THE PROPOSAL > > A conditional entity write: a set of entity writes applied atomically, > and only if a set of conditions holds. > > // PolarisMetaStoreManager > ConditionalEntityWriteResult writeEntitiesIfConditionsHold( > PolarisCallContext callCtx, ConditionalEntityWrite write); > > // BasePersistence > void checkAndWriteEntities( > PolarisCallContext callCtx, > List<ConditionalEntityWrite.Condition> conditions, > List<BasePersistence.EntityChange> changes); > > - Conditions: Condition.Unchanged (the entity is still at this version; > it is not written). > - Writes: Write.Update, the existing compare-and-swap update. > - All-or-nothing. A failed condition or write gives > TARGET_ENTITY_CONCURRENTLY_MODIFIED and nothing is written, as if the > write ran alone at serializable isolation. > - Conditions, writes and entity changes are sealed types, at both > layers. New kinds are added as new members, not as new methods. > - updateEntitiesPropertiesIfNotChanged becomes a default method on top > of it, so existing callers are unchanged. > > It is implemented and tested on all four backends. > > COMPATIBILITY > > This adds abstract methods to PolarisMetaStoreManager and > BasePersistence, so custom implementations must implement them. New > types of condition (for example "the parent still exists") or write > (for example create and drop) are added as new members of the sealed > types, without new methods or signature changes. Each new type is still > a contract addition that every backend has to implement; a backend that > doesn't know a kind rejects it at run time, not at compile time. > > WHERE ELSE IT COULD BE USED > > A rough grouping of the persistence SPI's write methods, by what they > would need from the primitive (not yet implemented): > > - Uses it today: updateEntitiesPropertiesIfNotChanged. > - Entity-only extensions: createEntityIfNotExists, > createEntitiesIfNotExist, updateEntityPropertiesIfNotChanged, > renameEntity, dropEntityIfExists. Creates and drops would move > together: Write.Create with a condition that the parent still exists, > and Write.Drop with a check that the entity has no children. > > Most of the race-prone call sites I looked at in the catalog layer > (create or drop under a namespace that changes concurrently, location > overlap) map onto these. > > **Out of scope**: writes beyond entities, such as grant records, policy > mappings and principal secrets. > > ALTERNATIVE > > #5820 also fixes #5807, without an SPI change. It adds each > requirement-only table to the commit as an update, which rewrites the > table's entity with a new version. With that approach, commits that > only assert the same table conflict with each other, and every asserted > table's version is bumped. #5961 checks such tables without writing > them, and verifies that the table still points to the metadata the > requirements were validated against. > > QUESTIONS > > 1. Is adding these SPI methods acceptable, given that they break custom > metastore implementations once? > 2. Is the shape right: separate conditions and writes, sealed kinds, > all-or-nothing, one conflict status? > 3. #5961 or #5820 for #5807? > > Feedback welcome, here or on the PR. > > Thanks, > Qiegang
