[I] Transaction commits against a base it never validated, so an external writer cannot make a commit conditional [iceberg-rust]

["AndreaBozzo (via GitHub)" <gi...@apache.org>]

AndreaBozzo opened a new issue, #3134:
URL: https://github.com/apache/iceberg-rust/issues/3134

   ## What happens
   
   A `Transaction` built from table state X commits successfully after the table
   has moved to Y. `Transaction::do_commit` reloads the table, sees the base is
   stale, replaces it, and re-derives the action's `TableRequirement`s from Y. The
   state the caller inspected before building the transaction never constrains the
   commit.
   
   This is correct for an unconditional append, which commutes. It is not correct
   for a commit that carries a precondition, and there is currently no way for an
   external crate to express one.
   
   ## Reproduction
   
   No arrow, no parquet, no data files. Depends only on `iceberg`, `tokio` and
   `tempfile`. Two transactions carrying snapshot summary properties are enough,
   because the issue is in how `Transaction` picks its base rather than in what the
   action writes.
   
   ```rust
   // Writer A loads the table and correctly observes that epoch 1 is absent.
   let a_base = catalog.load_table(&ident).await?;
   assert!(epochs(&a_base).is_empty());
   
   // Writer B does the same and commits first.
   let b_base = catalog.load_table(&ident).await?;
   let tx = Transaction::new(&b_base);
   let action = tx.fast_append()
       .set_snapshot_properties(HashMap::from([("example.epoch".to_string(), "1".to_string())]));
   action.apply(tx)?.commit(&catalog).await?;
   
   // A commits the transaction it built from `a_base`, which is now stale.
   // Expected: an error, so A can re-read and discover it lost.
   // Actual:   Ok.
   let tx = Transaction::new(&a_base);
   let action = tx.fast_append()
       .set_snapshot_properties(HashMap::from([("example.epoch".to_string(), "1".to_string())]));
   action.apply(tx)?.commit(&catalog).await?;
   
   assert_eq!(epochs(&catalog.load_table(&ident).await?), vec!["1", "1"]);
   ```
   
   ```
   B committed epoch 1
   A commit result: Ok
   epochs recorded in the table: ["1", "1"]
   ```
   
   The table is created with `commit.retry.num-retries = 0`, to show the retry loop
   is not the cause. `do_commit` rebases at the top of every call, including the
   first, before any failure has occurred.
   
   Full runnable crate: https://github.com/AndreaBozzo/iceberg-stale-base-repro
   
   ## Why it matters
   
   The concrete case is idempotent writes from an external engine. A writer that
   crashes between committing and observing success must be able to retry without
   producing a second copy, which needs a commit conditional on "this identifier
   has not already been applied". Delta expresses this with its `txn` action;
   Iceberg's specification supports it through requirements and the atomic
   metadata-pointer swap. In `iceberg-rust` it currently cannot be expressed.
   
   ## Why there is no workaround
   
   - `TransactionAction` is `pub(crate)` (`transaction/action.rs:37`), so an
     external crate cannot define an action whose `commit(&table)` re-validates
     against the refreshed base. That method is called with exactly the right table
     by `do_commit`; it just is not reachable.
   - `TableCommit`'s builder is `pub(crate)` too (`catalog/mod.rs:375`), so a caller
     cannot construct a commit carrying its own
     `TableRequirement::RefSnapshotIdMatch`, even though
     `Catalog::update_table(TableCommit)` is public. The doc comment is explicit
     that `Transaction` is the intended path.
   - Checking before `Transaction::commit` does not help, because the rebase
     discards the base the check was made against.
   
   ## The Java library has this hook
   
   `SnapshotProducer.validate(TableMetadata currentMetadata, Snapshot snapshot)` is
   `protected` (`core/src/main/java/org/apache/iceberg/SnapshotProducer.java:281`).
   `apply()` calls it (line 372), and the commit loop calls `apply()` on every retry
   attempt (lines 485-497), so an operation re-validates against the refreshed base
   each time. `BaseRowDelta`, `BaseReplacePartitions`, `BaseRewriteFiles` and
   `StreamingDelete` all override it.
   
   `iceberg-rust` has the structurally identical hook in
   `TransactionAction::commit(&Table)`, invoked by `do_commit` against the refreshed
   table. The difference is only that Java's is subclassable and Rust's is
   `pub(crate)`.
   
   ## Possible directions
   
   1. Make `TransactionAction` public, so a caller can define an action that
      validates against the refreshed base inside the existing loop. Smallest
      change, and it matches the Java model.
   2. Allow a caller-supplied `TableRequirement` on a `Transaction`, so the base a
      caller checked can be pinned across the rebase.
   3. If the rebase is intended to be unconditional, document that a `Transaction`
      carries no guarantee about the base it was constructed from, so callers do
      not build preconditions on it.
   
   Happy to open a PR for (1) if that is the direction maintainers prefer.
   
   ## Checked against `main`
   
   All three code references above are current on `main`, not only on the 0.10.1
   release. I searched the tracker and did not find this covered; the nearest
   neighbours looked like #964 (commit retries, closed — its step 2, "store the
   update actions and reapply them to the table when the commit fails", is where
   the rebase comes from) and #1939 / #3019, which are other requests to attach
   semantics to a commit atomically.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org