From 1e535f36a7a9425af223af3b7bd2a697b24e2b51 Mon Sep 17 00:00:00 2001 From: Rui Li Date: Mon, 22 Apr 2024 18:24:11 +0800 Subject: [PATCH] HIVE-28121: Use direct SQL for transactional altering table parameter (Rui Li, reviewed by Peter Vary) --- .../hive/metastore/HiveAlterHandler.java | 25 +++++++++++-------- .../hive/metastore/MetaStoreDirectSql.java | 9 +++++++ .../hadoop/hive/metastore/ObjectStore.java | 23 +++++++++++++++++ .../hadoop/hive/metastore/RawStore.java | 9 +++++++ .../hive/metastore/conf/MetastoreConf.java | 5 ++++ .../TestTablesCreateDropAlterTruncate.java | 10 +++++++- 6 files changed, 70 insertions(+), 11 deletions(-) diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java index f010f0d2cca8..cc96de8a636a 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java @@ -152,12 +152,7 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam String expectedValue = environmentContext != null && environmentContext.getProperties() != null ? environmentContext.getProperties().get(hive_metastoreConstants.EXPECTED_PARAMETER_VALUE) : null; - if (expectedKey != null) { - // If we have to check the expected state of the table we have to prevent nonrepeatable reads. - msdb.openTransaction(Constants.TX_REPEATABLE_READ); - } else { - msdb.openTransaction(); - } + msdb.openTransaction(); // get old table oldt = msdb.getTable(catName, dbname, name); if (oldt == null) { @@ -165,10 +160,20 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam Warehouse.getCatalogQualifiedTableName(catName, dbname, name) + " doesn't exist"); } - if (expectedKey != null && expectedValue != null - && !expectedValue.equals(oldt.getParameters().get(expectedKey))) { - throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '" - + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'"); + if (expectedKey != null && expectedValue != null) { + String newValue = newt.getParameters().get(expectedKey); + if (newValue == null) { + throw new MetaException(String.format("New value for expected key %s is not set", expectedKey)); + } + if (!expectedValue.equals(oldt.getParameters().get(expectedKey))) { + throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '" + + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'"); + } + long affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue); + if (affectedRows != 1) { + // make sure concurrent modification exception messages have the same prefix + throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is different"); + } } if (oldt.getPartitionKeysSize() != 0) { diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java index 4df43d628c32..0762a64a7b89 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java @@ -2555,4 +2555,13 @@ private void getStatsTableListResult( query.closeAll(); } } + + long updateTableParam(Table table, String key, String expectedValue, String newValue) { + String queryText = String.format("UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " + + "WHERE \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ? AND \"TBL_ID\" IN " + + "(SELECT \"TBL_ID\" FROM %s JOIN %s ON %s.\"DB_ID\" = %s.\"DB_ID\" WHERE \"TBL_NAME\" = '%s' AND \"NAME\" = '%s')", + TBLS, DBS, TBLS, DBS, table.getTableName(), table.getDbName()); + Query query = pm.newQuery("javax.jdo.query.SQL", queryText); + return (long) query.executeWithArray(newValue, key, expectedValue); + } } diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java index 815ee4184d8a..3dbde7039dd5 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java @@ -751,6 +751,29 @@ public boolean openTransaction(String isolationLevel) { return result; } + @Override + public long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException, NoSuchObjectException { + return new GetHelper(table.getCatName(), table.getDbName(), table.getTableName(), true, false) { + + @Override + protected String describeResult() { + return "Affected rows"; + } + + @Override + protected Long getSqlResult(GetHelper ctx) throws MetaException { + return directSql.updateTableParam(table, key, expectedValue, newValue); + } + + @Override + protected Long getJdoResult(GetHelper ctx) throws MetaException, NoSuchObjectException { + throw new UnsupportedOperationException( + "Cannot update parameter with JDO, make sure direct SQL is enabled"); + } + }.run(false); + } + /** * if this is the commit of the first open call then an actual commit is * called. diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java index a6ffcf417e92..8c41a40bcfa9 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java @@ -1657,4 +1657,13 @@ List getSchemaVersionsByColumns(String colName, String colNamespa Map> getPartitionColsWithStats(String catName, String dbName, String tableName) throws MetaException, NoSuchObjectException; + /** + * Updates a given table parameter with expected value. + * + * @return the number of rows updated + */ + default long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException, NoSuchObjectException { + throw new UnsupportedOperationException("This Store doesn't support updating table parameter with expected value"); + } } diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java index b8aab6037cb7..28f1c8f77209 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java @@ -419,6 +419,10 @@ public enum ConfVars { "Default transaction isolation level for identity generation."), DATANUCLEUS_USE_LEGACY_VALUE_STRATEGY("datanucleus.rdbms.useLegacyNativeValueStrategy", "datanucleus.rdbms.useLegacyNativeValueStrategy", true, ""), + DATANUCLEUS_QUERY_SQL_ALLOWALL("datanucleus.query.sql.allowAll", "datanucleus.query.sql.allowAll", + true, "In strict JDO all SQL queries must begin with \"SELECT ...\", and consequently it " + + "is not possible to execute queries that change data. This DataNucleus property when set to true allows " + + "insert, update and delete operations from JDO SQL. Default value is true."), DBACCESS_SSL_PROPS("metastore.dbaccess.ssl.properties", "hive.metastore.dbaccess.ssl.properties", "", "Comma-separated SSL properties for metastore to access database when JDO connection URL\n" + "enables SSL access. e.g. javax.net.ssl.trustStore=/tmp/truststore,javax.net.ssl.trustStorePassword=pwd."), @@ -1108,6 +1112,7 @@ public String toString() { ConfVars.DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK, ConfVars.DATANUCLEUS_TRANSACTION_ISOLATION, ConfVars.DATANUCLEUS_USE_LEGACY_VALUE_STRATEGY, + ConfVars.DATANUCLEUS_QUERY_SQL_ALLOWALL, ConfVars.DETACH_ALL_ON_COMMIT, ConfVars.IDENTIFIER_FACTORY, ConfVars.MANAGER_FACTORY_CLASS, diff --git a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java index a2ab9e807f37..f2d799b8baac 100644 --- a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java +++ b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java @@ -49,6 +49,7 @@ import org.apache.hadoop.hive.metastore.client.builder.PartitionBuilder; import org.apache.hadoop.hive.metastore.client.builder.TableBuilder; import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars; import org.apache.hadoop.hive.metastore.minihms.AbstractMetaStoreService; import org.apache.thrift.TApplicationException; import org.apache.thrift.TException; @@ -56,6 +57,7 @@ import org.apache.thrift.transport.TTransportException; import org.junit.After; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -105,7 +107,7 @@ public TestTablesCreateDropAlterTruncate(String name, AbstractMetaStoreService m @BeforeClass public static void startMetaStores() { - Map msConf = new HashMap(); + Map msConf = new HashMap(); // Enable trash, so it can be tested Map extraConf = new HashMap<>(); extraConf.put("fs.trash.checkpoint.interval", "30"); // FS_TRASH_CHECKPOINT_INTERVAL_KEY @@ -1086,6 +1088,8 @@ public void testAlterTableAlreadyExists() throws Exception { @Test public void testAlterTableExpectedPropertyMatch() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; EnvironmentContext context = new EnvironmentContext(); @@ -1099,6 +1103,8 @@ public void testAlterTableExpectedPropertyMatch() throws Exception { @Test(expected = MetaException.class) public void testAlterTableExpectedPropertyDifferent() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; EnvironmentContext context = new EnvironmentContext(); @@ -1118,6 +1124,8 @@ public void testAlterTableExpectedPropertyDifferent() throws Exception { */ @Test public void testAlterTableExpectedPropertyConcurrent() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; originalTable.getParameters().put("snapshot", "0");