Support staged table creates in commitTransaction by ayushtkn · Pull Request #4939 · apache/polaris

ayushtkn · 2026-06-30T11:19:59Z

Implement staged creates in the transaction workspace. Polaris previously rejected these and authorized
the whole batch with TABLE_WRITE_PROPERTIES + TABLE_CREATE on every
table identifier.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot

Pull request overview

This pull request adds support for “staged” table creates (i.e., UpdateTableRequest with AssertTableDoesNotExist) within commitTransaction by buffering creates/updates in a transaction workspace, deferring location-overlap validation, and then committing both creates and updates atomically in the metastore.

Changes:

Add staged-create detection + namespace-based authorization for commitTransaction, and execute staged creates within the transaction workspace.
Extend the transaction workspace metastore wrapper to buffer entity creations (in addition to updates) and defer certain validations until commit time.
Introduce commitTransactionBatch in the metastore interface/implementation and add new tests for staged creates + mixed transactions + authz.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java	Implements staged-create handling in `commitTransaction`, including new authz flow, deferred validation, and atomic batch commit.
polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java	Buffers entity creations and defers list/overlap validation behavior during the transaction loop.
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java	Adds `commitTransactionBatch` API (default implementation) to support atomic create+update commits.
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java	Implements transactional `commitTransactionBatch` to commit creates first, then CAS updates in one DB transaction.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java	Adds integration-style tests for staged-create success and failure cases via REST API.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogHandlerAuthzTest.java	Updates/extends authorization tests to cover staged-create privilege requirements and mixed transactions.

Comments suppressed due to low confidence (2)

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:1430

commitTransactionBatch failures due to ENTITY_ALREADY_EXISTS will currently be surfaced as CommitFailedException, which makes staged-create-on-existing-table return the wrong Iceberg exception type (tests expect AlreadyExistsException, and clients generally treat this as a conflict). Consider mapping result.alreadyExists() to AlreadyExistsException before falling back to CommitFailedException for other statuses.

    if (!result.isSuccess()) {
      // TODO: Retries and server-side cleanup on failure, review possible exceptions
      throw new CommitFailedException(
          "Transaction commit failed with status: %s, extraInfo: %s",
          result.getReturnStatus(), result.getExtraInformation());
    }

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:1294

baseCatalog's metastore manager is swapped to TransactionWorkspaceMetaStoreManager but only restored on the success path. If any exception is thrown while processing changesByTable (or while building metadata/committing), LocalIcebergCatalog will remain wired to the workspace manager for the remainder of the handler lifecycle, which can lead to confusing follow-on failures. Please wrap the workspace section in a try/finally that always restores setMetaStoreManager(metaStoreManager()).

    // Swap in TransactionWorkspaceMetaStoreManager so all mutations (both creates and updates)
    // go into an in-memory buffer that we commit as a single atomic unit at the end.
    // Location validation is deferred: the workspace returns no-op results for validation
    // calls during the loop, and real validation happens after the loop with the real metastore.
    TransactionWorkspaceMetaStoreManager transactionMetaStoreManager =
        new TransactionWorkspaceMetaStoreManager(diagnostics(), metaStoreManager());
    ((LocalIcebergCatalog) baseCatalog).setMetaStoreManager(transactionMetaStoreManager);

dimas-b · 2026-07-01T15:27:44Z

@ayushtkn : Thanks for the contribution!

I only briefly looked at the PR, so I might have missed something 😅 I wonder what end-to-end flow is envisioned? Is it 1) do a staged create N times, 2) write table files, 3) commit all staged tables via the commitTransaction API?

It's a pretty big change. I think it deserved a dev ML discussion 🙂

ayushtkn · 2026-07-01T16:06:10Z

Thanx @dimas-b for taking a look. yep, it is the same

Client calls catalog.buildTable(id, schema).stageCreate() N times — builds table metadata locally, no server roundtrip
Client optionally writes data files to the staged locations
Client calls catalog.commitTransaction() — sends a single CommitTransactionRequest containing all staged creates (identified by AssertTableDoesNotExist requirement) and any regular updates.

The server only sees step 3. This PR implements the server-side handling for that request when it includes staged creates alongside regular updates.

Sure, I will shoot a mail to dev@ for more feedback

snazy

I think we should separate two concepts here:

stage-create=true is the staging step on createTable. It initializes metadata and returns it without creating the table.
commitTransaction is a commit step. It should commit table changes that the client already constructed.

This PR seems to implement part of the staged-create flow inside commitTransaction by detecting AssertTableDoesNotExist and building metadata from empty/newTableOps.
I’m not sure that is the right responsibility for this endpoint.
If commitTransaction supports creates, I would expect it to commit an already-formed create commit entry, not perform the staging behavior itself.

Can we point to the IRC flow/client behavior this is matching?
In particular, does an Iceberg REST client send assert-create entries in CommitTransactionRequest after a prior stage-create call, and are we only completing that commit here?
If so, the PR should make that explicit and test that flow. If not, I think we should clarify whether this is actually supported by IRC before adding this path.

snazy · 2026-07-02T13:39:04Z

-                      "Unsupported operation: commitTransaction containing SetLocation"
-                          + " for table '%s' and new location '%s'",
-                      change.identifier(), ((MetadataUpdate.SetLocation) singleUpdate).location());
+    // Track staged-creates for deferred location validation after the real metastore is restored.


I’m concerned about the location-check deferral here.

During the workspace phase, listEntities returns empty and hasOverlappingSiblings returns "no overlap", so LocalIcebergCatalog’s normal overlap validation is effectively bypassed.
The PR then revalidates staged creates after restoring the real metastore, but that validation happens before commitTransactionBatch persists the entities.
Unless the final persistence step also enforces the no-overlap invariant in the same transaction/lock, there is a TOCTOU window where another request can create or move a table into the same location after validation passes.

There is also a non-concurrent gap: the workspace bypass applies to regular updates too.
A normal update can change write.data.path/write.metadata.path through table properties, but only stagedCreateEntries are revalidated after restore.
So location-changing updates in the same commitTransaction may skip overlap validation entirely.

Can we keep overlap validation inside the final atomic persistence path, or at least revalidate every pending create/update whose effective locations changed immediately inside the transaction that writes them?

You make a great point on the location overlap validations. I’ve updated the PR to completely close the non-concurrent gap (Issue 2).

Regular updates that mutate their locations (write.data.path, write.metadata.path, or table location) via commitTransaction are now explicitly tracked during the workspace phase. After the real metastore is restored, they are piped through validateTableLocationUpdate alongside the new staged creations. This ensures users cannot bypass location validation by changing properties inside a multi-table commit. (I've also added testUpdateChangingWriteDataPathFailsOnOverlap to explicitly test this).

Regarding the TOCTOU concurrency window between that deferred validation and the final persistence (Issue 1): you are absolutely right that a race exists here. However, while looking into how to safely push that validation down into commitTransactionBatch under the database lock, I realized that the standard single-table doCommit path in BasePolarisTableOperations actually suffers from this exact same TOCTOU race today.

In the existing updateTable path:

Validation (Read Tx): validateNoLocationOverlap runs a standalone read transaction via listEntities/hasOverlappingSiblings.

Cloud I/O: The thread then performs network I/O to write the new metadata file.

Persist (Write Tx): updateEntityPropertiesIfNotChanged runs in its own separate ms.runInTransaction() block.

Because these are completely separate metastore transactions separated by network I/O, concurrent requests can easily slip in and claim the location. Unless I misunderstood something here.

See LocalIcebergCatalog$BasePolarisTableOperations.doCommit() — validateNoLocationOverlap at L1766 runs in a read transaction, then updateEntityPropertiesIfNotChanged at L2677 persists in a separate write transaction

Fixing this systemic race requires pushing catalog-aware location extraction down into the generic persistence layer, and ensuring all Metastore implementations gracefully handle nested transactions or provide a pre-commit validation hook. Since this is a broader architectural limitation of Polaris affecting all location-mutating operations, I think we should track the TOCTOU fix as a separate issue rather than expanding the scope of this PR. Does that sound reasonable?

The TOCTOU problem wrt location checks exists in the per-table create/update flows too, does it not?

yes, what I could decode

Thanks for confirming. I think it deserves a separate discussion. I think it's way beyond the scope of current PR 😅

ayushtkn · 2026-07-02T16:45:36Z

Thanx @snazy for the review & feedback, To answer the questions

The Iceberg REST client explicitly sends an AssertTableDoesNotExist requirement alongside a list of updates during a commitTransaction, rather than sending an already-formed create commit entry. Because the REST spec's CommitTransactionRequest only accepts a list of UpdateTableRequest objects, the server never receives a fully constructed TableMetadata object from the client. It only receives the delta.

From the Iceberg Code

  public CommitTransactionRequest(List<UpdateTableRequest> tableChanges) {
    this.tableChanges = tableChanges;
    validate();
  }

https://github.com/apache/iceberg/blob/da8ff447a23447733ea6231625cc4d468245b090/core/src/main/java/org/apache/iceberg/rest/requests/CommitTransactionRequest.java#L29-L32

Because of this, the server is forced to reconstruct the staging behavior by building the metadata from empty and applying the client's update log.

We can see this exact behavior codified in the Apache Iceberg reference REST server implementation

1. Detecting the Create
In CatalogHandlers.java, the server explicitly detects a table creation within a transaction by looking for the AssertTableDoesNotExist requirement:

Iceberg ref:

  private static boolean isCreate(UpdateTableRequest request) {
    boolean isCreate =
        request.requirements().stream()

https://github.com/apache/iceberg/blob/6976e020b894f6a6777704df2b8c4458cb291ae9/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L578-L581

2. Building from Empty
If isCreate evaluates to true, the handler routes to the create() method, which literally builds the table from an empty state and applies the updates sequentially—exactly matching the approach in this PR:

Iceberg ref:

    TableMetadata.Builder builder =
        formatVersion.map(TableMetadata::buildFromEmpty).orElseGet(TableMetadata::buildFromEmpty);
    request.updates().forEach(update -> update.applyTo(builder));

https://github.com/apache/iceberg/blob/6976e020b894f6a6777704df2b8c4458cb291ae9/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L604-L606

Some Test refernce in Iceberg:
https://github.com/apache/iceberg/blob/6976e020b894f6a6777704df2b8c4458cb291ae9/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java#L3274-L3280

Ref from RestTableOPerarations, that it handles create
https://github.com/apache/iceberg/blob/da8ff447a23447733ea6231625cc4d468245b090/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java#L157-L173

Regarding adding a Test, I added testFullStagedCreateFlowMatchingIcebergRestClient, let me know if it works

ayushtkn · 2026-07-09T08:07:52Z

@snazy / @dimas-b any feedback / suggestions around this.

It's a pretty big change. I think it deserved a dev ML discussion

It is here, seems quiet though
https://lists.apache.org/thread/2x25mj2pv981ocj83l8wfpr8yqvpplmg

dimas-b

Hi @ayushtkn , the idea behind this PR seems reasonable to me 👍

However, the Persistence API change probably needs to be reworked to allow committing via a single RDBMS Tx over JDBC.

Some more specific comments below.

dimas-b · 2026-07-10T19:57:50Z

+    try {
+      changesByTable.forEach(
+          (tableIdentifier, changes) -> {
+            boolean isStagedCreate = changes.stream().anyMatch(CatalogHandlerUtils::isCreate);


Why are we calling it "staged"? How is it different from a normal create? As far as I can tell, there is no difference 🤔

dimas-b · 2026-07-10T19:59:16Z

+            // does not exist, ALL requests for that table must be staged-creates.
+            if (isStagedCreate
+                && changes.stream().anyMatch(change -> !CatalogHandlerUtils.isCreate(change))) {
+              throw new BadRequestException(


Is it normal for a create request to have more than one UpdateTableRequest element?

Should we simply check that there is one and it satisfies isCreate()?

dimas-b · 2026-07-10T20:08:07Z

+                UpdateTableRequest filteredChange = applyUpdateFilters(change);
+                filteredChange.requirements().forEach(req -> req.validate((TableMetadata) null));
+                for (MetadataUpdate singleUpdate : filteredChange.updates()) {
+                  singleUpdate.applyTo(metadataBuilder);


Should we delegate to CatalogHandlerUtils.create() perhaps? 🤔

dimas-b · 2026-07-10T20:13:30Z

-                      "Unsupported operation: commitTransaction containing SetLocation"
-                          + " for table '%s' and new location '%s'",
-                      change.identifier(), ((MetadataUpdate.SetLocation) singleUpdate).location());
+    // Track staged-creates for deferred location validation after the real metastore is restored.


The TOCTOU problem wrt location checks exists in the per-table create/update flows too, does it not?

dimas-b · 2026-07-10T20:19:06Z

+      @NonNull List<EntityWithPath> updates) {
+    TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore());
+
+    return ms.runInTransaction(


@ayushtkn : I'm not sure whether you're aware of this, but this code will NOT be used for JDBC Persistence... Just FYI 🤷 Check usage paths to confirm, if you prefer.

dimas-b · 2026-07-10T20:20:48Z

+      @NonNull List<EntityWithPath> creates,
+      @NonNull List<EntityWithPath> updates) {
+    for (EntityWithPath create : creates) {
+      EntityResult result = createEntityIfNotExists(callCtx, create.catalogPath(), create.entity());


In current JDBC persistence code each of those changes will run in a separate RDBMS Tx, AFAIK. It is based on AtomicOperationMetaStoreManager 🤷

Thanx. Have added AtomicOperationMetaStoreManager.commitTransactionBatch which should take care, I have added tests around that as well in the AtomicOperationMetaStoreManagerTest

dimas-b · 2026-07-10T20:23:48Z

+              "Transaction contains multiple creates pointing to the same location: %s", location);
+        }
+      }
+      ((LocalIcebergCatalog) baseCatalog)


nit: this cast is done multiple times. It might be worth introducing a strongly typed variable for this.

dimas-b · 2026-07-10T20:28:47Z

+   */
+  void validateTableLocationUpdate(TableIdentifier tableIdentifier, TableMetadata tableMetadata) {
+    PolarisResolvedPathWrapper resolvedTableEntities =
+        resolvedEntityView.getPassthroughResolvedPath(


This is called before changes are committed. Can it detect overlaps within the multi-table commit batch?

Added validateNoOverlapWithinBatch that runs first with a pairwise
StorageLocation.isChildOf check across all location-affecting entries (creates + location-changing updates) in the batch

Copilot AI review requested due to automatic review settings June 30, 2026 11:20

github-project-automation Bot added this to Basic Kanban Board Jun 30, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jun 30, 2026

Copilot started reviewing on behalf of ayushtkn June 30, 2026 11:20 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Comment thread .../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Outdated

Comment thread .../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Outdated

snazy reviewed Jul 2, 2026

View reviewed changes

dimas-b reviewed Jul 10, 2026

View reviewed changes

ayushtkn force-pushed the staged_create branch from 8acc0e3 to d025503 Compare July 11, 2026 03:11

Support staged table creates in commitTransaction

15a0203

ayushtkn force-pushed the staged_create branch from d025503 to 15a0203 Compare July 11, 2026 03:55

trigger ci

8197d5f

Uh oh!

Conversation

ayushtkn commented Jun 30, 2026

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

dimas-b commented Jul 1, 2026

Uh oh!

ayushtkn commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushtkn commented Jul 2, 2026

Uh oh!

ayushtkn commented Jul 9, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ayushtkn commented Jul 1, 2026 •

edited

Loading