This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 8d1b4c14be [MINOR] fix(postgresql): drop constraint-backed indexes
correctly (#13922)
8d1b4c14be is described below
commit 8d1b4c14bec9f2cd7ac9d08071c16cb654ca2e8c
Author: Qi Yu <[email protected]>
AuthorDate: Mon Oct 19 17:40:45 2026 +0800
[MINOR] fix(postgresql): drop constraint-backed indexes correctly (#13922)
### What changes were proposed in this pull request?
Fix deletion of PostgreSQL constraint-backed primary-key and unique
indexes by issuing only `ALTER TABLE ... DROP CONSTRAINT`. [PostgreSQL
automatically removes the backing index with the
constraint](https://www.postgresql.org/docs/current/sql-altertable.html),
so a second `DROP INDEX` incorrectly fails after the constraint has been
dropped.
Apply `IF EXISTS` to `DROP CONSTRAINT` when requested. Add non-Docker
SQL-generation coverage for strict, optional, and defaulted requests;
correct the existing SQL expectations; add catalog integration coverage
for both index types with `ifExists` unset, false, and true, including
repeated deletion.
### Why are the changes needed?
Deleting a primary-key or unique constraint-backed index currently fails
unless `ifExists=true`, because the generated SQL attempts to drop its
backing index twice. A missing constraint also incorrectly fails with
`ifExists=true`, because that flag is applied only to the second
statement.
This fixes indexes represented by PostgreSQL constraints, including
those created by Gravitino. Support for native standalone `CREATE UNIQUE
INDEX` objects is outside this change.
### Does this PR introduce _any_ user-facing change?
Deleting an existing constraint-backed index succeeds with the default
`ifExists=false`, while `ifExists=true` correctly ignores an absent
constraint. Strict requests still fail when the constraint is absent. No
API or configuration changes.
### How was this patch tested?
- All 50 PostgreSQL catalog non-Docker unit tests passed.
- Spotless checks and `git diff --check` passed.
- Five focused PostgreSQL 13 Docker tests passed: three new
nullable/false/true request cases, the existing mixed-alter catalog
test, and the updated legacy SQL expectations.
- All 7 existing `TestTableUpdatesRequest` tests passed, including JSON
deserialization with `ifExists` omitted.
- Temporarily restoring the old production implementation made all three
new catalog integration cases fail with the original
missing-index/constraint errors; restored the fix and reran all 50
module unit tests successfully.
---
.../operation/PostgreSqlTableOperations.java | 38 +++++-----------
.../integration/test/CatalogPostgreSqlIT.java | 51 ++++++++++++++++++++++
.../operation/TestPostgreSqlTableOperations.java | 8 +---
...TestPostgreSqlTableOperationsSqlGeneration.java | 36 +++++++++++++++
4 files changed, 100 insertions(+), 33 deletions(-)
diff --git
a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
index bf6d7e99ee..ca5606370a 100644
---
a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
+++
b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
@@ -414,33 +414,17 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
@VisibleForTesting
static String deleteIndexDefinition(String tableName,
TableChange.DeleteIndex deleteIndex) {
- StringBuilder sqlBuilder = new StringBuilder();
- sqlBuilder
- .append("ALTER TABLE ")
- .append(PG_QUOTE)
- .append(tableName)
- .append(PG_QUOTE)
- .append(" DROP CONSTRAINT ")
- .append(PG_QUOTE)
- .append(deleteIndex.getName())
- .append(PG_QUOTE)
- .append(";\n");
- if (deleteIndex.isIfExists()) {
- sqlBuilder
- .append("DROP INDEX IF EXISTS ")
- .append(PG_QUOTE)
- .append(deleteIndex.getName())
- .append(PG_QUOTE)
- .append(";");
- } else {
- sqlBuilder
- .append("DROP INDEX ")
- .append(PG_QUOTE)
- .append(deleteIndex.getName())
- .append(PG_QUOTE)
- .append(";");
- }
- return sqlBuilder.toString();
+ // Gravitino creates primary and unique indexes as constraints. Dropping
the constraint
+ // also removes its backing index, so a second DROP INDEX would fail after
a successful drop.
+ return String.format(
+ "ALTER TABLE %s%s%s DROP CONSTRAINT %s%s%s%s;",
+ PG_QUOTE,
+ tableName,
+ PG_QUOTE,
+ deleteIndex.isIfExists() ? "IF EXISTS " : "",
+ PG_QUOTE,
+ deleteIndex.getName(),
+ PG_QUOTE);
}
@VisibleForTesting
diff --git
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java
index 126ae8315d..5c635817e6 100644
---
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java
+++
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java
@@ -48,6 +48,7 @@ import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
import
org.apache.gravitino.catalog.postgresql.integration.test.service.PostgreSqlService;
import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.dto.requests.TableUpdateRequest;
import org.apache.gravitino.exceptions.ConnectionFailedException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.exceptions.SchemaAlreadyExistsException;
@@ -85,6 +86,9 @@ import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestInstance.Lifecycle;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
@Tag("gravitino-docker-test")
@TestInstance(Lifecycle.PER_CLASS)
@@ -1682,6 +1686,53 @@ public class CatalogPostgreSqlIT extends BaseIT {
Types.ListType.of(Types.IntegerType.get(), true),
loadedTable.columns()[2].dataType());
}
+ @ParameterizedTest
+ @NullSource
+ @ValueSource(booleans = {false, true})
+ void testDeleteConstraintBackedIndexes(Boolean ifExists) {
+ NameIdentifier ident =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("test_delete_indexes"));
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ tableCatalog.createTable(
+ ident,
+ new Column[] {
+ Column.of("id", Types.IntegerType.get(), null, false, false, null),
+ Column.of("code", Types.IntegerType.get(), null, false, false, null)
+ },
+ null,
+ createProperties(),
+ Transforms.EMPTY_TRANSFORM,
+ Distributions.NONE,
+ new SortOrder[0],
+ new Index[] {
+ Indexes.primary("delete_pk", new String[][] {{"id"}}),
+ Indexes.unique("delete_uk", new String[][] {{"code"}})
+ });
+
+ TableUpdateRequest.DeleteTableIndexRequest primary =
+ new TableUpdateRequest.DeleteTableIndexRequest("delete_pk", ifExists);
+ TableUpdateRequest.DeleteTableIndexRequest unique =
+ new TableUpdateRequest.DeleteTableIndexRequest("delete_uk", ifExists);
+ Table updated = tableCatalog.alterTable(ident, primary.tableChange());
+ Assertions.assertEquals(1, updated.index().length);
+ Assertions.assertEquals("delete_uk", updated.index()[0].name());
+
+ updated = tableCatalog.alterTable(ident, unique.tableChange());
+ Assertions.assertEquals(0, updated.index().length);
+ Assertions.assertEquals(0, tableCatalog.loadTable(ident).index().length);
+
+ // IF EXISTS applies to the constraint, including after both indexes have
been removed.
+ tableCatalog.alterTable(
+ ident,
+ TableChange.deleteIndex("delete_pk", true),
+ TableChange.deleteIndex("delete_uk", true));
+ RuntimeException missing =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> tableCatalog.alterTable(ident,
TableChange.deleteIndex("delete_pk", false)));
+ Assertions.assertTrue(missing.getMessage().contains("delete_pk"),
missing.getMessage());
+ }
+
@Test
void testOperationTableIndex() {
String tableName = GravitinoITUtils.genRandomName("test_add_index");
diff --git
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java
index 9fb4aafa49..cef450d4dc 100644
---
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java
+++
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java
@@ -723,15 +723,11 @@ public class TestPostgreSqlTableOperations extends
TestPostgreSql {
// Test delete index definition.
TableChange.DeleteIndex deleteIndex = new
TableChange.DeleteIndex("test_pk", false);
result = PostgreSqlTableOperations.deleteIndexDefinition(tableName,
deleteIndex);
- Assertions.assertEquals(
- "ALTER TABLE \"t1\" DROP CONSTRAINT \"test_pk\";\n" + "DROP INDEX
\"test_pk\";", result);
+ Assertions.assertEquals("ALTER TABLE \"t1\" DROP CONSTRAINT \"test_pk\";",
result);
deleteIndex = new TableChange.DeleteIndex("test_2_pk", true);
result = PostgreSqlTableOperations.deleteIndexDefinition(tableName,
deleteIndex);
- Assertions.assertEquals(
- "ALTER TABLE \"t1\" DROP CONSTRAINT \"test_2_pk\";\n"
- + "DROP INDEX IF EXISTS \"test_2_pk\";",
- result);
+ Assertions.assertEquals("ALTER TABLE \"t1\" DROP CONSTRAINT IF EXISTS
\"test_2_pk\";", result);
}
@Test
diff --git
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java
index 2862f98412..38f439ac20 100644
---
a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java
+++
b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java
@@ -24,6 +24,7 @@ import org.apache.gravitino.catalog.jdbc.JdbcTable;
import
org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter;
import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter;
import
org.apache.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter;
+import org.apache.gravitino.dto.requests.TableUpdateRequest;
import org.apache.gravitino.rel.TableChange;
import org.apache.gravitino.rel.expressions.distributions.Distributions;
import org.apache.gravitino.rel.expressions.literals.Literals;
@@ -32,6 +33,8 @@ import org.apache.gravitino.rel.indexes.Indexes;
import org.apache.gravitino.rel.types.Types;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
public class TestPostgreSqlTableOperationsSqlGeneration {
@@ -78,6 +81,39 @@ public class TestPostgreSqlTableOperationsSqlGeneration {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDeleteIndexesDropsOnlyConstraints(boolean ifExists) {
+ TestablePostgreSqlTableOperations ops = new
TestablePostgreSqlTableOperations();
+
+ String sql =
+ ops.alterTableSql(
+ "test_table",
+ TableChange.deleteIndex("primary_key", ifExists),
+ TableChange.deleteIndex("unique_key", ifExists));
+
+ String optional = ifExists ? "IF EXISTS " : "";
+ Assertions.assertEquals(
+ "ALTER TABLE \"test_table\" DROP CONSTRAINT "
+ + optional
+ + "\"primary_key\";\n"
+ + "ALTER TABLE \"test_table\" DROP CONSTRAINT "
+ + optional
+ + "\"unique_key\";",
+ sql);
+ }
+
+ @Test
+ void testDeleteIndexWithoutIfExistsUsesStrictConstraintDrop() {
+ TableUpdateRequest request =
+ new TableUpdateRequest.DeleteTableIndexRequest("primary_key", null);
+ TestablePostgreSqlTableOperations ops = new
TestablePostgreSqlTableOperations();
+
+ Assertions.assertEquals(
+ "ALTER TABLE \"test_table\" DROP CONSTRAINT \"primary_key\";",
+ ops.alterTableSql("test_table", request.tableChange()));
+ }
+
@Test
public void testCreateTableWithEmptyStringDefaultValue() {
TestablePostgreSqlTableOperations ops = new
TestablePostgreSqlTableOperations();
