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
