relational-jdbc: add DB-agnostic idempotency store + model by huaxingao · Pull Request #3584 · apache/polaris

huaxingao · 2026-01-27T23:39:12Z

#3205 followup:

Introduce JDBC model for idempotency records via ModelIdempotencyRecord
Add database-agnostic implementation RelationalJdbcIdempotencyStore implementing IdempotencyStore.
Standardize core domain type location by moving IdempotencyRecord to polaris-core under org.apache.polaris.core.entity and updating IdempotencyStore imports accordingly.
Update tests by replacing PostgresIdempotencyStoreIT with RelationalJdbcIdempotencyStorePostgresIT

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)

huaxingao · 2026-01-28T17:17:55Z

cc @singhpk234 @flyrain Could you please take a look when you have a moment?

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

...olaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

singhpk234 · 2026-01-31T17:53:23Z

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

+    QueryGenerator.PreparedQuery update =
+        new QueryGenerator.PreparedQuery(
+            sql,
+            List.of(Timestamp.from(now), Timestamp.from(now), realmId, idempotencyKey, executorId));


see insteadt of using Timestamp.now() we can use the injected Clock ?

We’re not using Timestamp.now() here — the store takes Instant now as a parameter and only converts via Timestamp.from(now)

singhpk234 · 2026-02-01T06:11:50Z

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

+      if (datasourceOperations.isConstraintViolation(e)) {
+        return new ReserveResult(ReserveResultType.DUPLICATE, load(realmId, idempotencyKey));
+      }
+      throw new RuntimeException("Failed to reserve idempotency key", e);


can we have a defined exception ? what error code we wanna throw in case for the request ?

Added IdempotencyPersistenceException

singhpk234 · 2026-02-01T06:14:23Z

polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java

+   * Normalized identifier of the resource affected by the operation.
+   *
+   * <p>This should be derived from the request (for example, a canonicalized path like {@code
+   * "tables/ns.tbl"}), not from a generated internal entity id. This ensures the binding is


how do we protect against drop + create and renames ?

Do we define canonicalized path somewhere? Should we avoid duplication for a canonicalized path? A path like this tables/ns.tbl is very likely causing conflicts, as different catalogs could have the same namespace and table names.

singhpk234 · 2026-02-01T06:15:23Z

...olaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java

-      new PostgreSQLContainer<>("postgres:17.5-alpine");
+      new PostgreSQLContainer<>(
+              containerSpecHelper("postgres", PostgresRelationalJdbcLifeCycleManagement.class)
+                  .dockerImageName(null)


why null ? can we do lastest tag ?

null is intentional here: containerSpecHelper(...).dockerImageName(null) means “use the pinned default from Dockerfile-postgres-version and allow overrides via system props/env.

singhpk234 · 2026-02-01T06:17:13Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+  public static String fullyQualifiedTableName(@Nonnull String tableName) {
+    return getFullyQualifiedTableName(tableName);
+  }


I am confused, why create a new public function ? can we not have private function itself made public ?

Right, we don't need this. removed.

singhpk234 · 2026-02-01T06:21:14Z

.../main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java

+  ModelIdempotencyRecord CONVERTER =
+      ImmutableModelIdempotencyRecord.builder()
+          .realmId("")
+          .idempotencyKey("")
+          .operationType("")
+          .resourceId("")
+          .createdAt(Instant.EPOCH)
+          .updatedAt(Instant.EPOCH)
+          .expiresAt(Instant.EPOCH)
+          .build();


I didn't fully get the rationale of this
orthognally if its absolutely required lets make it static ?

I’ve refactored this to avoid the dummy instance by introducing a static fromRow(ResultSet) and a stateless CONVERTER singleton that delegates to it.

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

singhpk234 · 2026-02-01T20:44:24Z

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

+import org.apache.polaris.persistence.relational.jdbc.models.ImmutableModelIdempotencyRecord;
+import org.apache.polaris.persistence.relational.jdbc.models.ModelIdempotencyRecord;
+
+public class RelationalJdbcIdempotencyStore implements IdempotencyStore {


How is this obj created ? How is core gonna use it

RelationalJdbcIdempotencyStore is just the relational-jdbc backend for the IdempotencyStore interface. It’s constructed with a DataSource + RelationalJdbcConfiguration and then the request/idempotency layer calls reserve/load/updateHeartbeat/finalize/purgeExpired through the IdempotencyStore SPI.

singhpk234 · 2026-02-01T20:46:22Z

.../main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java

+  String REALM_ID = "realm_id";
+  String IDEMPOTENCY_KEY = "idempotency_key";
+  String OPERATION_TYPE = "operation_type";
+  String RESOURCE_ID = "resource_id";
+
+  String HTTP_STATUS = "http_status";
+  String ERROR_SUBTYPE = "error_subtype";
+  String RESPONSE_SUMMARY = "response_summary";
+  String RESPONSE_HEADERS = "response_headers";
+  String FINALIZED_AT = "finalized_at";
+
+  String CREATED_AT = "created_at";
+  String UPDATED_AT = "updated_at";
+  String HEARTBEAT_AT = "heartbeat_at";
+  String EXECUTOR_ID = "executor_id";
+  String EXPIRES_AT = "expires_at";


we should add code comments for each of the fields like we do in the rest of the model

singhpk234 · 2026-02-01T20:48:56Z

...g/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java

+    Optional<IdempotencyRecord> existing = load(realmId, idempotencyKey);
+    if (existing.isEmpty()) {
+      return HeartbeatResult.NOT_FOUND;
+    }
+
+    IdempotencyRecord record = existing.get();
+    if (record.getHttpStatus() != null) {
+      return HeartbeatResult.FINALIZED;
+    }
+    if (record.getExecutorId() == null || !record.getExecutorId().equals(executorId)) {
+      return HeartbeatResult.LOST_OWNERSHIP;
+    }
+
+    QueryGenerator.PreparedQuery update =
+        QueryGenerator.generateUpdateQuery(
+            ModelIdempotencyRecord.SELECT_COLUMNS,
+            ModelIdempotencyRecord.TABLE_NAME,
+            Map.of(
+                ModelIdempotencyRecord.HEARTBEAT_AT,
+                Timestamp.from(now),
+                ModelIdempotencyRecord.UPDATED_AT,
+                Timestamp.from(now)),
+            Map.of(
+                ModelIdempotencyRecord.REALM_ID,
+                realmId,
+                ModelIdempotencyRecord.IDEMPOTENCY_KEY,
+                idempotencyKey,
+                ModelIdempotencyRecord.EXECUTOR_ID,
+                executorId),
+            Map.of(),
+            Map.of(),
+            Set.of(ModelIdempotencyRecord.HTTP_STATUS),
+            Set.of());
+
+    try {
+      int updated = datasourceOperations.executeUpdate(update);
+      if (updated > 0) {
+        return HeartbeatResult.UPDATED;
+      }
+    } catch (SQLException e) {
+      throw new RuntimeException("Failed to update idempotency heartbeat", e);
+    }
+
+    // Raced with finalize/ownership loss; re-check to return a meaningful result.
+    Optional<IdempotencyRecord> after = load(realmId, idempotencyKey);
+    if (after.isEmpty()) {
+      return HeartbeatResult.NOT_FOUND;
+    }
+    if (after.get().getHttpStatus() != null) {
+      return HeartbeatResult.FINALIZED;
+    }
+    return HeartbeatResult.LOST_OWNERSHIP;
+  }


would it not need to be in a transaction ?

The actual heartbeat change is done via a single atomic UPDATE ... WHERE realm/key/executor AND http_status IS NULL, so it doesn’t require an explicit transaction for correctness.

singhpk234 · 2026-02-01T21:03:37Z

...olaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java

+              containerSpecHelper("postgres", PostgresRelationalJdbcLifeCycleManagement.class)
+                  .dockerImageName(null)
+                  .asCompatibleSubstituteFor("postgres"))
+          .withDatabaseName("polaris_db")


we set the foloowing in setup too, why we need here again ?

singhpk234 · 2026-02-03T03:30:32Z

These were final set of comments :), can't think of anymore, thanks @huaxingao for working on it and driving the whole idempotency effort !

flyrain

Thanks @huaxingao for the PR. LGTM overall. Left some comments.

flyrain · 2026-02-06T00:25:35Z

polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java

+   * Normalized identifier of the resource affected by the operation.
+   *
+   * <p>This should be derived from the request (for example, a canonicalized path like {@code
+   * "tables/ns.tbl"}), not from a generated internal entity id. This ensures the binding is


Do we define canonicalized path somewhere? Should we avoid duplication for a canonicalized path? A path like this tables/ns.tbl is very likely causing conflicts, as different catalogs could have the same namespace and table names.

flyrain · 2026-02-06T02:27:39Z

.../main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java

+ * <p>This follows the same pattern as {@link ModelEvent}, separating the storage representation
+ * from the core domain model while still providing {@link Converter} helpers.
+ */
+@PolarisImmutable


Do we need it to be PolarisImmutable? We didn't do that for ModelEntity, and a few others.

I removed @PolarisImmutable as suggested. That required a small follow-up refactor in RelationalJdbcIdempotencyStore.reserve(): we no longer build an ImmutableModelIdempotencyRecord just to get the insert bindings; instead we construct the insert map/values directly (same columns/values, still uses QueryGenerator.generateInsertQuery). This keeps behavior the same while avoiding the Immutables-generated type.

flyrain · 2026-02-06T02:30:49Z

...-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java

  }

+  /** Returns the detected database type for this datasource. */
+  public DatabaseType databaseType() {


Do we need this new method given that we've got a getDatabaseType() already(line 82)? We could change the scope to public if needed.

Removed. Thanks!

flyrain · 2026-02-06T02:35:19Z

.../main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java

+
+  // Logical tenant / realm identifier.
+  String REALM_ID = "realm_id";
+  // Client-provided idempotency key.


Nit: We got so many nice comments here while there is no comment on the class IdempotencyRecord. I'd love to have these comments on the class IdempotencyRecord.

Added more comments. Thanks!

flyrain · 2026-02-06T02:45:29Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+   * <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the
+   * set clause so generated SQL and parameter order are stable.


nit: feel a bit easier to understand to use consistent, or matched. Just my personal preference. Feel free to ignore it.

Suggested change

* <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the

* set clause so generated SQL and parameter order are stable.

* <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the

* set clause so generated SQL and parameter order are consistent.

Fixed. Thanks for the suggestion!

flyrain · 2026-02-06T06:58:56Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+    validateColumns(columns, whereEquals.keySet());
+    validateColumns(columns, whereGreater.keySet());
+    validateColumns(columns, whereLess.keySet());
+    validateColumns(columns, whereIsNull);
+    validateColumns(columns, whereIsNotNull);


Do we need to validate them here as the method generateWhereClauseExtended() will validates them anyways?

You are right. Removed.

flyrain · 2026-02-06T06:59:35Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+    validateColumns(columns, setClause.keySet());
+    validateColumns(columns, whereEquals.keySet());
+    validateColumns(columns, whereGreater.keySet());
+    validateColumns(columns, whereLess.keySet());
+    validateColumns(columns, whereIsNull);
+    validateColumns(columns, whereIsNotNull);


flyrain · 2026-02-06T07:01:37Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+    // Preserve the original behavior of rejecting unknown columns. This is used by SELECT query
+    // generation too, not only by callers of the extended UPDATE/DELETE helpers.


Nit: I guess this comment is more like a dev log. Do we need them as a comment?

flyrain · 2026-02-06T07:21:22Z

...tional-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java

+      @Nonnull Set<String> tableColumns, @Nonnull Set<String> columns) {
+    for (String column : columns) {
+      if (!tableColumns.contains(column) && !column.equals("realm_id")) {
+        throw new IllegalArgumentException("Invalid query column: " + column);


realm_id is treated as a special implicit column for some models but explicitly included in SELECT_COLUMNS for ModelIdempotencyRecord. It might be cleaner to do something similar to other model, which keep realm_id out by only using ALL_COLUMNS instead of having SELECT_COLUMNS. WDYT?

Makes sense. Fixed.

singhpk234

+1 LGTM overall

relational-jdbc: add DB-agnostic idempotency store + model

8a2ba83

github-project-automation bot added this to Basic Kanban Board Jan 27, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jan 27, 2026

spotless apply

236cb0f

singhpk234 reviewed Jan 31, 2026

View reviewed changes

huaxingao added 3 commits January 31, 2026 15:10

extend QueryGenerator for idempotency queries

d2ef040

add PG profile

fc2ac61

spotlessApply

a3427fc

singhpk234 reviewed Feb 2, 2026

View reviewed changes

address comments

323484b

flyrain reviewed Feb 6, 2026

View reviewed changes

singhpk234 reviewed Feb 6, 2026

View reviewed changes

huaxingao added 2 commits February 6, 2026 15:38

address comments

feceb77

fix test failure

6d20fa3

		* <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the
		* set clause so generated SQL and parameter order are stable.

		// Preserve the original behavior of rejecting unknown columns. This is used by SELECT query
		// generation too, not only by callers of the extended UPDATE/DELETE helpers.

Conversation

huaxingao commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

huaxingao commented Jan 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

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!

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!

singhpk234 commented Feb 3, 2026

Uh oh!

flyrain 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!

Choose a reason for hiding this comment

huaxingao commented Jan 27, 2026 •

edited

Loading

singhpk234 Feb 1, 2026 •

edited

Loading