This is an automated email from the ASF dual-hosted git repository. yanxinyi pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/master by this push: new 0373358 PHOENIX-6124 : Conditionally block adding/dropping a column on a parent view 0373358 is described below commit 0373358bc6545845e0a44dce4e112e7ec9a5a01c Author: Viraj Jasani <vjas...@apache.org> AuthorDate: Tue Oct 20 19:30:00 2020 +0530 PHOENIX-6124 : Conditionally block adding/dropping a column on a parent view Signed-off-by: Xinyi Yan <yanxi...@apache.org> --- .../AlterParentTableWithSysCatRollbackIT.java | 142 +++++++++++++++++++++ .../phoenix/coprocessor/MetaDataEndpointImpl.java | 119 ++++++++++++----- 2 files changed, 232 insertions(+), 29 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterParentTableWithSysCatRollbackIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterParentTableWithSysCatRollbackIT.java new file mode 100644 index 0000000..8bf5405 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterParentTableWithSysCatRollbackIT.java @@ -0,0 +1,142 @@ +/* + * 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.phoenix.end2end; + +import org.apache.phoenix.exception.SQLExceptionCode; +import org.apache.phoenix.query.BaseTest; +import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; +import org.apache.phoenix.util.ReadOnlyProps; +import org.apache.phoenix.util.SchemaUtil; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.SQLException; +import java.util.Collections; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +@Category(NeedsOwnMiniClusterTest.class) +public class AlterParentTableWithSysCatRollbackIT extends BaseTest { + + private String getJdbcUrl() { + return "jdbc:phoenix:localhost:" + getUtility().getZkCluster().getClientPort() + + ":/hbase"; + } + + @BeforeClass + public static synchronized void doSetup() throws Exception { + Map<String, String> serverProps = Maps.newHashMapWithExpectedSize(1); + serverProps.put(QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK, "true"); + setUpTestDriver(new ReadOnlyProps(serverProps.entrySet().iterator()), + new ReadOnlyProps(Collections.emptyIterator())); + } + + @Test + public void testAddColumnOnParentTableView() throws Exception { + try (Connection conn = DriverManager.getConnection(getJdbcUrl())) { + String parentTableName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + String parentViewName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + String childViewName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + // create parent table + String ddl = "CREATE TABLE " + parentTableName + + " (col1 INTEGER NOT NULL, col2 INTEGER " + "CONSTRAINT pk PRIMARY KEY (col1))"; + conn.createStatement().execute(ddl); + + // create view from table + ddl = "CREATE VIEW " + parentViewName + " AS SELECT * FROM " + parentTableName; + conn.createStatement().execute(ddl); + try { + ddl = "ALTER TABLE " + parentTableName + " ADD col4 INTEGER"; + conn.createStatement().execute(ddl); + fail("ALTER TABLE should not be allowed on parent table"); + } catch (SQLException e) { + assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(), e.getErrorCode()); + } + + // create child view from above view + ddl = "CREATE VIEW " + childViewName + "(col3 INTEGER) AS SELECT * FROM " + + parentViewName; + conn.createStatement().execute(ddl); + try { + ddl = "ALTER VIEW " + parentViewName + " ADD col4 INTEGER"; + conn.createStatement().execute(ddl); + fail("ALTER VIEW should not be allowed on parent view"); + } catch (SQLException e) { + assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(), e.getErrorCode()); + } + + // alter child view with add column should be allowed + ddl = "ALTER VIEW " + childViewName + " ADD col4 INTEGER"; + conn.createStatement().execute(ddl); + } + } + + @Test + public void testDropColumnOnParentTableView() throws Exception { + try (Connection conn = DriverManager.getConnection(getJdbcUrl())) { + String parentTableName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + String parentViewName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + String childViewName = SchemaUtil.getTableName(generateUniqueName(), + generateUniqueName()); + // create parent table + String ddl = "CREATE TABLE " + parentTableName + + " (col1 INTEGER NOT NULL, col2 INTEGER, col3 VARCHAR " + + "CONSTRAINT pk PRIMARY KEY (col1))"; + conn.createStatement().execute(ddl); + + // create view from table + ddl = "CREATE VIEW " + parentViewName + " AS SELECT * FROM " + parentTableName; + conn.createStatement().execute(ddl); + try { + ddl = "ALTER TABLE " + parentTableName + " DROP COLUMN col2"; + conn.createStatement().execute(ddl); + fail("ALTER TABLE DROP COLUMN should not be allowed on parent table"); + } catch (SQLException e) { + assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(), e.getErrorCode()); + } + + // create child view from above view + ddl = "CREATE VIEW " + childViewName + "(col5 INTEGER) AS SELECT * FROM " + + parentViewName; + conn.createStatement().execute(ddl); + try { + ddl = "ALTER VIEW " + parentViewName + " DROP COLUMN col2"; + conn.createStatement().execute(ddl); + fail("ALTER VIEW DROP COLUMN should not be allowed on parent view"); + } catch (SQLException e) { + assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(), e.getErrorCode()); + } + + // alter child view with drop column should be allowed + ddl = "ALTER VIEW " + childViewName + " DROP COLUMN col2"; + conn.createStatement().execute(ddl); + } + } +} \ No newline at end of file diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java index a8965af..1f7957f 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java @@ -95,6 +95,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.NavigableMap; +import java.util.Optional; import java.util.Properties; import java.util.Set; @@ -2577,9 +2578,87 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr EnvironmentEdgeManager.currentTimeMillis(), table, tableNamesToDelete, sharedTablesToDelete); } - private MetaDataMutationResult - mutateColumn(List<Mutation> tableMetadata, ColumnMutator mutator, int clientVersion, PTable parentTable) - throws IOException { + /** + * Validate if mutation is allowed on parent table/view + * based on their child views. + * If this method returns MetaDataMutationResult, mutation is not allowed, + * and returned object will contain returnCode + * (MutationCode) to indicate the underlying + * problem (validation failure code). + * + * @param expectedType expected type of PTable + * @param clientTimeStamp client timestamp, e.g check + * {@link MetaDataUtil#getClientTimeStamp(List)} + * @param tenantId tenant Id + * @param schemaName schema name + * @param tableName table name + * @param childViews child views of table or parent view. + * Usually this is an empty list + * passed to this method, and this method will add + * child views retrieved using + * {@link #findAllChildViews(long, int, byte[], byte[], byte[])} + * @param clientVersion client version, used to determine if + * mutation is allowed. + * @return Optional.empty() if mutation is allowed on parent + * table/view. If not allowed, returned Optional object + * will contain metaDataMutationResult with MutationCode. + * @throws IOException if something goes wrong while retrieving + * child views using + * {@link #findAllChildViews(long, int, byte[], byte[], byte[])} + * @throws SQLException if something goes wrong while retrieving + * child views using + * {@link #findAllChildViews(long, int, byte[], byte[], byte[])} + */ + private Optional<MetaDataMutationResult> validateIfMutationAllowedOnParent( + final PTableType expectedType, final long clientTimeStamp, + final byte[] tenantId, final byte[] schemaName, + final byte[] tableName, final List<PTable> childViews, + final int clientVersion) throws IOException, SQLException { + boolean isMutationAllowed = true; + if (expectedType == PTableType.TABLE || + expectedType == PTableType.VIEW) { + childViews.addAll(findAllChildViews(clientTimeStamp, clientVersion, + tenantId, schemaName, tableName)); + if (!childViews.isEmpty()) { + // From 4.15 onwards we allow SYSTEM.CATALOG to split and no + // longer propagate parent + // metadata changes to child views. + // If the client is on a version older than 4.15 we have to + // block adding a column to a parent table/view as we no + // longer lock the parent table on the server side + // while creating a child view to prevent conflicting changes. + // This is handled on the client side from 4.15 onwards. + // Also if + // QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK is + // true, we block adding a column to a parent table/view so + // that we can rollback the upgrade if required. + if (clientVersion < MIN_SPLITTABLE_SYSTEM_CATALOG) { + isMutationAllowed = false; + LOGGER.error("Unable to add or drop a column as the " + + "client is older than {}", + MIN_SPLITTABLE_SYSTEM_CATALOG_VERSION); + } else if (allowSplittableSystemCatalogRollback) { + isMutationAllowed = false; + LOGGER.error("Unable to add or drop a column as the {} " + + "config is set to true", + QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK); + } + } + } + if (!isMutationAllowed) { + MetaDataMutationResult metaDataMutationResult = + new MetaDataMutationResult( + MetaDataProtocol.MutationCode.UNALLOWED_TABLE_MUTATION, + EnvironmentEdgeManager.currentTimeMillis(), null); + return Optional.of(metaDataMutationResult); + } + return Optional.empty(); + } + + private MetaDataMutationResult mutateColumn( + final List<Mutation> tableMetadata, + final ColumnMutator mutator, final int clientVersion, + final PTable parentTable) throws IOException { byte[][] rowKeyMetaData = new byte[5][]; MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata, rowKeyMetaData); byte[] tenantId = rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX]; @@ -2603,32 +2682,14 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr List<RowLock> locks = Lists.newArrayList(); try { - if (expectedType == PTableType.TABLE) { - childViews = findAllChildViews(clientTimeStamp, clientVersion, tenantId, - schemaName, tableName); - - if (!childViews.isEmpty()) { - // From 4.15 onwards we allow SYSTEM.CATALOG to split and no longer propagate parent - // metadata changes to child views. - // If the client is on a version older than 4.15 we have to block adding a column to a - // parent able as we no longer lock the parent table on the server side while creating a - // child view to prevent conflicting changes. This is handled on the client side from - // 4.15 onwards. - // Also if QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK is true, we block adding - // a column to a parent table so that we can rollback the upgrade if required. - if (clientVersion < MIN_SPLITTABLE_SYSTEM_CATALOG) { - LOGGER.error("Unable to add or drop a column as the client is older " - + "than " + MIN_SPLITTABLE_SYSTEM_CATALOG_VERSION); - return new MetaDataProtocol.MetaDataMutationResult(MetaDataProtocol.MutationCode.UNALLOWED_TABLE_MUTATION, - EnvironmentEdgeManager.currentTimeMillis(), null); - } else if (allowSplittableSystemCatalogRollback) { - LOGGER.error("Unable to add or drop a column as the " - + QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK - + " config is set to true"); - return new MetaDataProtocol.MetaDataMutationResult(MetaDataProtocol.MutationCode.UNALLOWED_TABLE_MUTATION, - EnvironmentEdgeManager.currentTimeMillis(), null); - } - } + Optional<MetaDataMutationResult> mutationResult = + validateIfMutationAllowedOnParent(expectedType, + clientTimeStamp, tenantId, schemaName, tableName, + childViews, clientVersion); + // only if mutation is allowed, we should get Optional.empty() + // here + if (mutationResult.isPresent()) { + return mutationResult.get(); } acquireLock(region, key, locks);