You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@polaris.apache.org by si...@apache.org on 2026/05/21 15:54:52 UTC
(polaris) branch main updated: fix: prevent grants_version_going_backward under concurrent grants (#4358)
This is an automated email from the ASF dual-hosted git repository.
singhpk234 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 5041165eb5 fix: prevent grants_version_going_backward under concurrent grants (#4358)
5041165eb5 is described below
commit 5041165eb538e0f3d949af49b7ac87da21733de2
Author: Prashant Singh <35...@users.noreply.github.com>
AuthorDate: Thu May 21 08:54:46 2026 -0700
fix: prevent grants_version_going_backward under concurrent grants (#4358)
* fix: prevent grants_version_going_backward under concurrent grants
Two independent issues in AtomicOperationMetaStoreManager / JDBC persistence
could surface `IllegalStateException: grants_version_going_backward` and
silently lost grant-version increments on the JDBC backend under concurrent
write workload (reproducible via polaris-tools' create-dataset-simulation
benchmark, #3836):
* Read side: refreshResolvedEntity called lookupEntityVersions at t0 and
conditionally lookupEntity at t1, then returned the entity from t1 paired
with the grants version from t0. A concurrent persistNewGrantRecord that
bumped the entity row's grant_records_version between the two reads
produced a result where entity.getGrantRecordsVersion() exceeded the
reported grantsVersion, tripping ResolvedPolarisEntity's invariant. Fix:
when the entity is reloaded, use its own grant_records_version as the
reported value so the returned pair is internally consistent.
* Write side: JdbcBasePersistenceImpl.persistEntity CAS-checked
entity_version only, not grant_records_version, so two concurrent grant
bumps on the same grantee could both pass CAS and silently lose one
increment. Fix: include grant_records_version in the CAS WHERE clause,
matching the transactional path's CAS at
AbstractTransactionalPersistence.checkConditionsForWriteEntityInCurrentTxn.
Adds AtomicOperationMetaStoreManagerRefreshTest covering the read-side race
reproducer.
Fixes #3836
* Add comment explaining why grant_records_version is in the CAS clause
---
.../relational/jdbc/JdbcBasePersistenceImpl.java | 11 +-
.../AtomicOperationMetaStoreManager.java | 10 +-
...AtomicOperationMetaStoreManagerRefreshTest.java | 186 +++++++++++++++++++++
3 files changed, 203 insertions(+), 4 deletions(-)
diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
index 4a5b0fb982..8e1e534d7d 100644
--- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
+++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
@@ -214,6 +214,8 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
String.format("Failed to write entity due to %s", e.getMessage()), e);
}
} else {
+ // CAS on both entity_version and grant_records_version because grant operations only
+ // bump grant_records_version without touching entity_version.
Map<String, Object> params =
Map.of(
"id",
@@ -222,6 +224,8 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
originalEntity.getCatalogId(),
"entity_version",
originalEntity.getEntityVersion(),
+ "grant_records_version",
+ originalEntity.getGrantRecordsVersion(),
"realm_id",
realmId);
try {
@@ -237,8 +241,11 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
params));
if (rowsUpdated == 0) {
throw new RetryOnConcurrencyException(
- "Entity '%s' id '%s' concurrently modified; expected version %s",
- originalEntity.getName(), originalEntity.getId(), originalEntity.getEntityVersion());
+ "Entity '%s' id '%s' concurrently modified; expected entity_version=%s, grant_records_version=%s",
+ originalEntity.getName(),
+ originalEntity.getId(),
+ originalEntity.getEntityVersion(),
+ originalEntity.getGrantRecordsVersion());
}
} catch (SQLException e) {
throw new RuntimeException(
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java
index 85294ba21e..3614595902 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java
@@ -1788,12 +1788,18 @@ public class AtomicOperationMetaStoreManager extends BaseMetaStoreManager {
entity = null;
}
+ // When the entity was reloaded, a concurrent writer may have bumped its
+ // grant_records_version between our lookupEntityVersions and lookupEntity reads. Use the
+ // entity's own value so (entity, grantRecordsVersion) stays internally consistent.
+ int reportedGrantRecordsVersion =
+ entity != null ? entity.getGrantRecordsVersion() : entityVersions.grantRecordsVersion();
+
// TODO: Consider whether it's necessary to look up grant records atomically with the entity
// or if it's actually safe as independent lookups. Document the nuances of independent lookups.
// load the grant records if required
final List<PolarisGrantRecord> grantRecords;
- if (entityVersions.grantRecordsVersion() != entityGrantRecordsVersion) {
+ if (reportedGrantRecordsVersion != entityGrantRecordsVersion) {
if (entityType.isGrantee()) {
grantRecords =
new ArrayList<>(ms.loadAllGrantRecordsOnGrantee(callCtx, entityCatalogId, entityId));
@@ -1806,7 +1812,7 @@ public class AtomicOperationMetaStoreManager extends BaseMetaStoreManager {
}
// return the result
- return new ResolvedEntityResult(entity, entityVersions.grantRecordsVersion(), grantRecords);
+ return new ResolvedEntityResult(entity, reportedGrantRecordsVersion, grantRecords);
}
/** {@inheritDoc} */
diff --git a/polaris-core/src/test/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManagerRefreshTest.java b/polaris-core/src/test/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManagerRefreshTest.java
new file mode 100644
index 0000000000..d19d47c939
--- /dev/null
+++ b/polaris-core/src/test/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManagerRefreshTest.java
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.when;
+
+import java.time.Clock;
+import java.util.Collections;
+import java.util.List;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.PolarisDefaultDiagServiceImpl;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisChangeTrackingVersions;
+import org.apache.polaris.core.entity.PolarisEntitySubType;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.dao.entity.BaseResult;
+import org.apache.polaris.core.persistence.dao.entity.ResolvedEntityResult;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+/** Regression tests for {@link AtomicOperationMetaStoreManager#refreshResolvedEntity} (#3836). */
+public class AtomicOperationMetaStoreManagerRefreshTest {
+
+ private static final long CATALOG_ID = 100L;
+ private static final long ENTITY_ID = 200L;
+ private static final long PARENT_ID = 50L;
+ private static final String ENTITY_NAME = "catalog_admin";
+
+ private PolarisDiagnostics diagnostics;
+ private BasePersistence metaStore;
+ private PolarisCallContext callCtx;
+ private AtomicOperationMetaStoreManager manager;
+
+ @BeforeEach
+ public void setUp() {
+ diagnostics = new PolarisDefaultDiagServiceImpl();
+ metaStore = Mockito.mock(BasePersistence.class);
+ callCtx = new PolarisCallContext(() -> "testRealm", metaStore);
+ manager = new AtomicOperationMetaStoreManager(Clock.systemUTC(), diagnostics);
+ }
+
+ private PolarisBaseEntity buildEntity(int entityVersion, int grantRecordsVersion) {
+ return new PolarisBaseEntity.Builder()
+ .catalogId(CATALOG_ID)
+ .id(ENTITY_ID)
+ .parentId(PARENT_ID)
+ .typeCode(PolarisEntityType.CATALOG_ROLE.getCode())
+ .subTypeCode(PolarisEntitySubType.NULL_SUBTYPE.getCode())
+ .name(ENTITY_NAME)
+ .entityVersion(entityVersion)
+ .grantRecordsVersion(grantRecordsVersion)
+ .createTimestamp(System.currentTimeMillis())
+ .build();
+ }
+
+ /**
+ * A concurrent writer bumps grant_records_version between lookupEntityVersions (t0=5) and
+ * lookupEntity (t1=7). The returned grantRecordsVersion must come from the entity, so the pair
+ * (entity, grantRecordsVersion) satisfies {@link ResolvedPolarisEntity}'s invariant.
+ */
+ @Test
+ public void testRefreshPairsEntityWithItsOwnGrantRecordsVersion() {
+ PolarisBaseEntity liveEntity = buildEntity(2, 7);
+
+ when(metaStore.lookupEntityVersions(any(), any()))
+ .thenReturn(List.of(new PolarisChangeTrackingVersions(2, 5)));
+ when(metaStore.lookupEntity(any(), anyLong(), anyLong(), anyInt())).thenReturn(liveEntity);
+ when(metaStore.loadAllGrantRecordsOnGrantee(any(), anyLong(), anyLong())).thenReturn(List.of());
+ when(metaStore.loadAllGrantRecordsOnSecurable(any(), anyLong(), anyLong()))
+ .thenReturn(List.of());
+
+ ResolvedEntityResult result =
+ manager.refreshResolvedEntity(
+ callCtx, 1, 3, PolarisEntityType.CATALOG_ROLE, CATALOG_ID, ENTITY_ID);
+
+ assertThat(result.isSuccess()).isTrue();
+ assertThat(result.getEntity()).isNotNull();
+ assertThat(result.getEntity().getGrantRecordsVersion()).isEqualTo(7);
+ assertThat(result.getEntity().getEntityVersion()).isEqualTo(2);
+ assertThat(result.getGrantRecordsVersion()).isEqualTo(7);
+ }
+
+ /** When entityVersion matches, the entity is not reloaded and the returned entity is null. */
+ @Test
+ public void testRefreshReturnsNullEntityWhenVersionUnchanged() {
+ when(metaStore.lookupEntityVersions(any(), any()))
+ .thenReturn(List.of(new PolarisChangeTrackingVersions(2, 9)));
+ when(metaStore.loadAllGrantRecordsOnGrantee(any(), anyLong(), anyLong())).thenReturn(List.of());
+ when(metaStore.loadAllGrantRecordsOnSecurable(any(), anyLong(), anyLong()))
+ .thenReturn(List.of());
+
+ ResolvedEntityResult result =
+ manager.refreshResolvedEntity(
+ callCtx, 2, 3, PolarisEntityType.CATALOG_ROLE, CATALOG_ID, ENTITY_ID);
+
+ assertThat(result.isSuccess()).isTrue();
+ assertThat(result.getEntity()).isNull();
+ assertThat(result.getGrantRecordsVersion()).isEqualTo(9);
+ assertThat(result.getEntityGrantRecords()).isNotNull();
+ Mockito.verify(metaStore, Mockito.never()).lookupEntity(any(), anyLong(), anyLong(), anyInt());
+ }
+
+ @Test
+ public void testRefreshReturnsNotFoundWhenEntityPurged() {
+ when(metaStore.lookupEntityVersions(any(), any())).thenReturn(Collections.singletonList(null));
+
+ ResolvedEntityResult result =
+ manager.refreshResolvedEntity(
+ callCtx, 1, 1, PolarisEntityType.CATALOG_ROLE, CATALOG_ID, ENTITY_ID);
+
+ assertThat(result.getReturnStatus()).isEqualTo(BaseResult.ReturnStatus.ENTITY_NOT_FOUND);
+ }
+
+ /** Both versions match: no reloads, fast path. */
+ @Test
+ public void testRefreshIsNoopWhenNothingChanged() {
+ when(metaStore.lookupEntityVersions(any(), any()))
+ .thenReturn(List.of(new PolarisChangeTrackingVersions(4, 12)));
+
+ ResolvedEntityResult result =
+ manager.refreshResolvedEntity(
+ callCtx, 4, 12, PolarisEntityType.CATALOG_ROLE, CATALOG_ID, ENTITY_ID);
+
+ assertThat(result.isSuccess()).isTrue();
+ assertThat(result.getEntity()).isNull();
+ assertThat(result.getGrantRecordsVersion()).isEqualTo(12);
+ assertThat(result.getEntityGrantRecords()).isNull();
+
+ Mockito.verify(metaStore, Mockito.never()).lookupEntity(any(), anyLong(), anyLong(), anyInt());
+ Mockito.verify(metaStore, Mockito.never())
+ .loadAllGrantRecordsOnGrantee(any(), anyLong(), anyLong());
+ Mockito.verify(metaStore, Mockito.never())
+ .loadAllGrantRecordsOnSecurable(any(), anyLong(), anyLong());
+ }
+
+ /**
+ * Caller's grants snapshot agrees with lookupEntityVersions (both 5) but a concurrent bump means
+ * the reloaded entity carries gv=6. Grants must be reloaded so the caller receives a
+ * (grantRecords, grantRecordsVersion) pair consistent with the reloaded entity.
+ */
+ @Test
+ public void testRefreshReloadsGrantsWhenEntityCarriesNewerGrantRecordsVersion() {
+ PolarisBaseEntity liveEntity = buildEntity(2, 6);
+
+ when(metaStore.lookupEntityVersions(any(), any()))
+ .thenReturn(List.of(new PolarisChangeTrackingVersions(2, 5)));
+ when(metaStore.lookupEntity(any(), anyLong(), anyLong(), anyInt())).thenReturn(liveEntity);
+ when(metaStore.loadAllGrantRecordsOnGrantee(any(), anyLong(), anyLong())).thenReturn(List.of());
+ when(metaStore.loadAllGrantRecordsOnSecurable(any(), anyLong(), anyLong()))
+ .thenReturn(List.of());
+
+ ResolvedEntityResult result =
+ manager.refreshResolvedEntity(
+ callCtx, 1, 5, PolarisEntityType.CATALOG_ROLE, CATALOG_ID, ENTITY_ID);
+
+ assertThat(result.isSuccess()).isTrue();
+ assertThat(result.getEntity()).isNotNull();
+ assertThat(result.getEntity().getGrantRecordsVersion()).isEqualTo(6);
+ assertThat(result.getGrantRecordsVersion()).isEqualTo(6);
+ assertThat(result.getEntityGrantRecords()).isNotNull();
+ Mockito.verify(metaStore).loadAllGrantRecordsOnGrantee(any(), anyLong(), anyLong());
+ Mockito.verify(metaStore).loadAllGrantRecordsOnSecurable(any(), anyLong(), anyLong());
+ }
+}