This is an automated email from the ASF dual-hosted git repository. rajeshbabu 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 1740409 PHOENIX-4753 Remove the need for users to have Write access to the Phoenix SYSTEM STATS TABLE to drop tables(Rajeshbabu) 1740409 is described below commit 1740409a56a621452b32771ca80d16b61f25c4a7 Author: Rajeshbabu Chintaguntla <rchintagun...@cloudera.com> AuthorDate: Fri May 8 20:49:40 2020 +0530 PHOENIX-4753 Remove the need for users to have Write access to the Phoenix SYSTEM STATS TABLE to drop tables(Rajeshbabu) --- .../apache/phoenix/end2end/BasePermissionsIT.java | 130 +++++++++++++++++++-- .../phoenix/coprocessor/MetaDataEndpointImpl.java | 58 ++++++++- .../phoenix/coprocessor/MetaDataProtocol.java | 9 +- .../coprocessor/PhoenixAccessController.java | 16 ++- .../org/apache/phoenix/schema/MetaDataClient.java | 27 ----- .../java/org/apache/phoenix/util/MetaDataUtil.java | 61 ++++++---- 6 files changed, 230 insertions(+), 71 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java index f722c02..f2a6b9d 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java @@ -19,12 +19,6 @@ package org.apache.phoenix.end2end; import com.google.common.base.Joiner; import com.google.common.base.Throwables; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - import java.io.IOException; import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedExceptionAction; @@ -61,6 +55,7 @@ import org.apache.phoenix.query.QueryConstants; import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.schema.NewerSchemaAlreadyExistsException; import org.apache.phoenix.schema.TableNotFoundException; +import org.apache.phoenix.util.MetaDataUtil; import org.apache.phoenix.util.PhoenixRuntime; import org.apache.phoenix.util.SchemaUtil; import org.junit.Before; @@ -71,6 +66,8 @@ import org.junit.runners.MethodSorters; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.junit.Assert.*; + @Category(NeedsOwnMiniClusterTest.class) @FixMethodOrder(MethodSorters.NAME_ASCENDING) public abstract class BasePermissionsIT extends BaseTest { @@ -151,6 +148,7 @@ public abstract class BasePermissionsIT extends BaseTest { Configuration config = testUtil.getConfiguration(); enablePhoenixHBaseAuthorization(config); configureNamespacesOnServer(config, isNamespaceMapped); + configureStatsConfigurations(config); config.setBoolean(LocalHBaseCluster.ASSIGN_RANDOM_PORTS, true); testUtil.startMiniCluster(1); @@ -208,6 +206,12 @@ public abstract class BasePermissionsIT extends BaseTest { conf.set(QueryServices.IS_NAMESPACE_MAPPING_ENABLED, Boolean.toString(isNamespaceMapped)); } + private static void configureStatsConfigurations(Configuration conf) { + conf.set(QueryServices.STATS_GUIDEPOST_WIDTH_BYTES_ATTRIB, Long.toString(20)); + conf.set(QueryServices.STATS_UPDATE_FREQ_MS_ATTRIB, Long.toString(5)); + conf.set(QueryServices.MAX_SERVER_METADATA_CACHE_TIME_TO_LIVE_MS_ATTRIB, Long.toString(5)); + conf.set(QueryServices.USE_STATS_FOR_PARALLELIZATION, Boolean.toString(true)); + } public static HBaseTestingUtility getUtility(){ return testUtil; } @@ -384,13 +388,17 @@ public abstract class BasePermissionsIT extends BaseTest { } AccessTestAction createTable(final String tableName) throws SQLException { + return createTable(tableName, NUM_RECORDS); + } + + AccessTestAction createTable(final String tableName, int numRecordsToInsert) throws SQLException { return new AccessTestAction() { @Override public Object run() throws Exception { try (Connection conn = getConnection(); Statement stmt = conn.createStatement();) { assertFalse(stmt.execute("CREATE TABLE " + tableName + "(pk INTEGER not null primary key, data VARCHAR, val integer)")); try (PreparedStatement pstmt = conn.prepareStatement("UPSERT INTO " + tableName + " values(?, ?, ?)")) { - for (int i = 0; i < NUM_RECORDS; i++) { + for (int i = 0; i < numRecordsToInsert; i++) { pstmt.setInt(1, i); pstmt.setString(2, Integer.toString(i)); pstmt.setInt(3, i); @@ -404,6 +412,19 @@ public abstract class BasePermissionsIT extends BaseTest { }; } + AccessTestAction updateStatsOnTable(final String tableName) throws SQLException { + return new AccessTestAction() { + @Override + public Object run() throws Exception { + try (Connection conn = getConnection(); Statement stmt = conn.createStatement();) { + assertFalse(stmt.execute("UPDATE STATISTICS " + tableName + " SET \"" + + QueryServices.STATS_GUIDEPOST_WIDTH_BYTES_ATTRIB + "\" = 5")); + } + return null; + } + }; + } + private AccessTestAction createMultiTenantTable(final String tableName) throws SQLException { return new AccessTestAction() { @Override @@ -440,6 +461,37 @@ public abstract class BasePermissionsIT extends BaseTest { } + private AccessTestAction deleteDataFromStatsTable() throws SQLException { + return new AccessTestAction() { + @Override + public Object run() throws Exception { + try (Connection conn = getConnection(); Statement stmt = conn.createStatement();) { + conn.setAutoCommit(true); + assertNotEquals(0, stmt.executeUpdate("DELETE FROM SYSTEM.STATS")); + } + return null; + } + }; + + } + + private AccessTestAction readStatsAfterTableDelete(String physicalTableName) throws SQLException { + return new AccessTestAction() { + @Override + public Object run() throws Exception { + try (Connection conn = getConnection(); Statement stmt = conn.createStatement();) { + conn.setAutoCommit(true); + ResultSet rs = stmt.executeQuery("SELECT count(*) from SYSTEM.STATS" + + " WHERE PHYSICAL_NAME = '"+ physicalTableName +"'"); + rs.next(); + assertEquals(0, rs.getInt(1)); + } + return null; + } + }; + + } + // Attempts to read given table without verifying data // AccessDeniedException is only triggered when ResultSet#next() method is called // The first call triggers HBase Scan object @@ -1220,6 +1272,69 @@ public abstract class BasePermissionsIT extends BaseTest { } @Test + public void testDeletingStatsShouldNotFailWithADEWhenTableDropped() throws Throwable { + final String schema = "STATS_ENABLED"; + final String tableName = "DELETE_TABLE_IT"; + final String phoenixTableName = schema + "." + tableName; + final String indexName1 = tableName + "_IDX1"; + final String lIndexName1 = tableName + "_LIDX1"; + final String viewName1 = schema+"."+tableName + "_V1"; + final String viewIndexName1 = tableName + "_VIDX1"; + grantSystemTableAccess(); + try { + superUser1.runAs(new PrivilegedExceptionAction<Void>() { + @Override + public Void run() throws Exception { + try { + verifyAllowed(createSchema(schema), superUser1); + //Neded Global ADMIN for flush operation during drop table + AccessControlClient.grant(getUtility().getConnection(),regularUser1.getName(), Permission.Action.ADMIN); + if (isNamespaceMapped) { + grantPermissions(regularUser1.getName(), schema, Permission.Action.CREATE); + grantPermissions(AuthUtil.toGroupEntry(GROUP_SYSTEM_ACCESS), schema, Permission.Action.CREATE); + } else { + grantPermissions(regularUser1.getName(), + NamespaceDescriptor.DEFAULT_NAMESPACE.getName(), Permission.Action.CREATE); + grantPermissions(AuthUtil.toGroupEntry(GROUP_SYSTEM_ACCESS), + NamespaceDescriptor.DEFAULT_NAMESPACE.getName(), Permission.Action.CREATE); + } + } catch (Throwable e) { + if (e instanceof Exception) { + throw (Exception)e; + } else { + throw new Exception(e); + } + } + return null; + } + }); + + verifyAllowed(createTable(phoenixTableName, 100), regularUser1); + verifyAllowed(createIndex(indexName1,phoenixTableName),regularUser1); + verifyAllowed(createLocalIndex(lIndexName1, phoenixTableName), regularUser1); + verifyAllowed(createView(viewName1,phoenixTableName),regularUser1); + verifyAllowed(createIndex(viewIndexName1, viewName1), regularUser1); + verifyAllowed(updateStatsOnTable(phoenixTableName), regularUser1); + Thread.sleep(10000); + // Normal deletes should fail when no write permissions given on stats table. + verifyDenied(deleteDataFromStatsTable(), AccessDeniedException.class, regularUser1); + verifyAllowed(dropIndex(viewIndexName1, viewName1), regularUser1); + verifyAllowed(dropView(viewName1),regularUser1); + verifyAllowed(dropIndex(indexName1, phoenixTableName), regularUser1); + Thread.sleep(3000); + verifyAllowed(readStatsAfterTableDelete(SchemaUtil.getPhysicalHBaseTableName( + schema, indexName1, isNamespaceMapped).getString()), regularUser1); + verifyAllowed(dropIndex(lIndexName1, phoenixTableName), regularUser1); + verifyAllowed(dropTable(phoenixTableName), regularUser1); + Thread.sleep(3000); + verifyAllowed(readStatsAfterTableDelete(SchemaUtil.getPhysicalHBaseTableName( + schema, tableName, isNamespaceMapped).getString()), regularUser1); + } finally { + revokeAll(); + } + } + + @Test public void testUpsertIntoImmutableTable() throws Throwable { final String schema = generateUniqueName(); final String tableName = generateUniqueName(); @@ -1292,5 +1407,4 @@ public abstract class BasePermissionsIT extends BaseTest { } }; } - } 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 f371f98..cdd8400 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 @@ -2215,7 +2215,7 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr String tableType = request.getTableType(); byte[] schemaName = null; byte[] tableName = null; - + boolean dropTableStats = false; try { List<Mutation> tableMetadata = ProtobufUtil.getMutations(request); List<Mutation> childLinkMutations = Lists.newArrayList(); @@ -2326,8 +2326,16 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr } done.run(MetaDataMutationResult.toProto(result)); + dropTableStats = true; } finally { ServerUtil.releaseRowLocks(locks); + if(dropTableStats) { + Thread statsDeleteHandler = new Thread(new StatsDeleteHandler(env, + loadedTable, tableNamesToDelete, sharedTablesToDelete), + "thread-statsdeletehandler"); + statsDeleteHandler.setDaemon(true); + statsDeleteHandler.start(); + } } } catch (Throwable t) { LOGGER.error("dropTable failed", t); @@ -2336,6 +2344,50 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr } } + class StatsDeleteHandler implements Runnable { + PTable deletedTable; + List<byte[]> physicalTableNames; + List<MetaDataProtocol.SharedTableState> sharedTableStates; + RegionCoprocessorEnvironment env; + + StatsDeleteHandler(RegionCoprocessorEnvironment env, PTable deletedTable, List<byte[]> physicalTableNames, + List<MetaDataProtocol.SharedTableState> sharedTableStates) { + this.deletedTable = deletedTable; + this.physicalTableNames = physicalTableNames; + this.sharedTableStates = sharedTableStates; + this.env = env; + } + + @Override + public void run() { + try { + User.runAsLoginUser(new PrivilegedExceptionAction<Object>() { + @Override + public Object run() throws Exception { + try (PhoenixConnection connection = + QueryUtil.getConnectionOnServer(env.getConfiguration()) + .unwrap(PhoenixConnection.class)) { + try{ + MetaDataUtil.deleteFromStatsTable(connection, deletedTable, + physicalTableNames, sharedTableStates); + LOGGER.info("Table stats deleted successfully. "+ + deletedTable.getPhysicalName().getString()); + } catch(Throwable t) { + LOGGER.warn("Exception while deleting stats of table " + + deletedTable.getPhysicalName().getString() + + " please check and delete stats manually"); + } + } + return null; + } + }); + } catch (IOException e) { + LOGGER.warn("Exception while deleting stats of table " + + deletedTable.getPhysicalName().getString() + + " please check and delete stats manually"); + } + } + } private RowLock acquireLock(Region region, byte[] lockKey, List<RowLock> locks) throws IOException { RowLock rowLock = region.getRowLock(lockKey, false); if (rowLock == null) { @@ -3990,8 +4042,8 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements RegionCopr } private TableName getParentPhysicalTableName(PTable table) { - return table - .getType() == PTableType.VIEW + return (table + .getType() == PTableType.VIEW || (table.getType() == INDEX && table.getViewIndexId() != null)) ? TableName.valueOf(table.getPhysicalName().getBytes()) : table.getType() == PTableType.INDEX ? TableName diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java index 9fb308f..eb20a3f 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java @@ -32,12 +32,7 @@ import org.apache.phoenix.coprocessor.generated.PFunctionProtos; import org.apache.phoenix.hbase.index.util.VersionUtil; import org.apache.phoenix.parse.PFunction; import org.apache.phoenix.parse.PSchema; -import org.apache.phoenix.schema.PColumn; -import org.apache.phoenix.schema.PColumnImpl; -import org.apache.phoenix.schema.PName; -import org.apache.phoenix.schema.PNameFactory; -import org.apache.phoenix.schema.PTable; -import org.apache.phoenix.schema.PTableImpl; +import org.apache.phoenix.schema.*; import org.apache.phoenix.schema.types.PDataType; import org.apache.phoenix.util.ByteUtil; import org.apache.phoenix.util.MetaDataUtil; @@ -194,7 +189,7 @@ public abstract class MetaDataProtocol extends MetaDataService { private List<PName> physicalNames; private PDataType viewIndexIdType; private Long viewIndexId; - + public SharedTableState(PTable table) { this.tenantId = table.getTenantId(); this.schemaName = table.getSchemaName(); diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java index 2bb9011..609f2d1 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java @@ -123,7 +123,11 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { public void preGetTable(ObserverContext<PhoenixMetaDataControllerEnvironment> ctx, String tenantId, String tableName, TableName physicalTableName) throws IOException { if (!accessCheckEnabled) { return; } - requireAccess("GetTable" + tenantId, physicalTableName, Action.READ, Action.EXEC); + if(this.execPermissionsCheckEnabled) { + requireAccess("GetTable" + tenantId, physicalTableName, Action.READ, Action.EXEC); + } else { + requireAccess("GetTable" + tenantId, physicalTableName, Action.READ); + } } @Override @@ -131,10 +135,10 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { Configuration conf = env.getConfiguration(); this.accessCheckEnabled = conf.getBoolean(QueryServices.PHOENIX_ACLS_ENABLED, QueryServicesOptions.DEFAULT_PHOENIX_ACLS_ENABLED); - if (!this.accessCheckEnabled) { - LOGGER.warn( - "PhoenixAccessController has been loaded with authorization checks disabled."); - } + if (!this.accessCheckEnabled) { + LOGGER.warn( + "PhoenixAccessController has been loaded with authorization checks disabled."); + } this.execPermissionsCheckEnabled = conf.getBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, AccessControlConstants.DEFAULT_EXEC_PERMISSION_CHECKS); if (env instanceof PhoenixMetaDataControllerEnvironment) { @@ -245,7 +249,7 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { // skip check for local index if (physicalTableName != null && !parentPhysicalTableName.equals(physicalTableName) && !MetaDataUtil.isViewIndex(physicalTableName.getNameAsString())) { - List<Action> actions = Arrays.asList(Action.READ, Action.WRITE, Action.CREATE, Action.ADMIN); + List<Action> actions = new ArrayList<>(Arrays.asList(Action.READ, Action.WRITE, Action.CREATE, Action.ADMIN)); if(execPermissionsCheckEnabled) { actions.add(Action.EXEC); } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java index 4f64765..42f1025 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java @@ -3401,7 +3401,6 @@ public class MetaDataClient { for (PTable index : table.getIndexes()) { tableRefs.add(new TableRef(null, index, ts, false)); } - deleteFromStatsTable(tableRefs, ts); } if (!dropMetaData) { MutationPlan plan = new PostDDLCompiler(connection).compile(tableRefs, null, null, @@ -3418,32 +3417,6 @@ public class MetaDataClient { } } - private void deleteFromStatsTable(List<TableRef> tableRefs, long ts) throws SQLException { - boolean isAutoCommit = connection.getAutoCommit(); - try { - connection.setAutoCommit(true); - StringBuilder buf = new StringBuilder("DELETE FROM SYSTEM.STATS WHERE PHYSICAL_NAME IN ("); - for (TableRef ref : tableRefs) { - buf.append("'" + ref.getTable().getPhysicalName().getString() + "',"); - } - buf.setCharAt(buf.length() - 1, ')'); - if(tableRefs.get(0).getTable().getIndexType()==IndexType.LOCAL) { - buf.append(" AND COLUMN_FAMILY IN("); - if (tableRefs.get(0).getTable().getColumnFamilies().isEmpty()) { - buf.append("'" + QueryConstants.DEFAULT_LOCAL_INDEX_COLUMN_FAMILY + "',"); - } else { - for(PColumnFamily cf : tableRefs.get(0).getTable().getColumnFamilies()) { - buf.append("'" + cf.getName().getString() + "',"); - } - } - buf.setCharAt(buf.length() - 1, ')'); - } - connection.createStatement().execute(buf.toString()); - } finally { - connection.setAutoCommit(isAutoCommit); - } - } - private MutationCode processMutationResult(String schemaName, String tableName, MetaDataMutationResult result) throws SQLException { final MutationCode mutationCode = result.getMutationCode(); PName tenantId = connection.getTenantId(); diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java index c25f81d..d7dc375 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java @@ -23,14 +23,7 @@ import static org.apache.phoenix.util.SchemaUtil.getVarChars; import java.io.IOException; import java.sql.SQLException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.NavigableMap; +import java.util.*; import com.google.common.annotations.VisibleForTesting; import org.apache.hadoop.conf.Configuration; @@ -70,21 +63,10 @@ import org.apache.phoenix.hbase.index.util.VersionUtil; import org.apache.phoenix.jdbc.PhoenixConnection; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; import org.apache.phoenix.query.QueryConstants; -import org.apache.phoenix.schema.ColumnFamilyNotFoundException; -import org.apache.phoenix.schema.ColumnNotFoundException; -import org.apache.phoenix.schema.PColumn; -import org.apache.phoenix.schema.PColumnFamily; -import org.apache.phoenix.schema.PName; -import org.apache.phoenix.schema.PNameFactory; -import org.apache.phoenix.schema.PTable; +import org.apache.phoenix.schema.*; import org.apache.phoenix.schema.PTable.IndexType; import org.apache.phoenix.schema.PTable.LinkType; import org.apache.phoenix.schema.PTable.ViewType; -import org.apache.phoenix.schema.PTableType; -import org.apache.phoenix.schema.SequenceKey; -import org.apache.phoenix.schema.SortOrder; -import org.apache.phoenix.schema.TableNotFoundException; -import org.apache.phoenix.schema.TableProperty; import org.apache.phoenix.schema.types.PBoolean; import org.apache.phoenix.schema.types.PDataType; import org.apache.phoenix.schema.types.PInteger; @@ -1001,4 +983,43 @@ public class MetaDataUtil { } return col; } + + public static void deleteFromStatsTable(PhoenixConnection connection, + PTable table, List<byte[]> physicalTableNames, + List<MetaDataProtocol.SharedTableState> sharedTableStates) + throws SQLException { + boolean isAutoCommit = connection.getAutoCommit(); + try { + connection.setAutoCommit(true); + Set<String> physicalTablesSet = new HashSet<>(); + Set<String> columnFamilies = new HashSet<>(); + physicalTablesSet.add(table.getPhysicalName().getString()); + for(byte[] physicalTableName:physicalTableNames) { + physicalTablesSet.add(Bytes.toString(physicalTableName)); + } + for(MetaDataProtocol.SharedTableState s: sharedTableStates) { + physicalTablesSet.add(s.getPhysicalNames().get(0).getString()); + } + StringBuilder buf = new StringBuilder("DELETE FROM SYSTEM.STATS WHERE PHYSICAL_NAME IN ("); + Iterator itr = physicalTablesSet.iterator(); + while(itr.hasNext()) { + buf.append("'" + itr.next() + "',"); + } + buf.setCharAt(buf.length() - 1, ')'); + if(table.getIndexType()==IndexType.LOCAL) { + buf.append(" AND COLUMN_FAMILY IN("); + if (table.getColumnFamilies().isEmpty()) { + buf.append("'" + QueryConstants.DEFAULT_LOCAL_INDEX_COLUMN_FAMILY + "',"); + } else { + for(PColumnFamily cf : table.getColumnFamilies()) { + buf.append("'" + cf.getName().getString() + "',"); + } + } + buf.setCharAt(buf.length() - 1, ')'); + } + connection.createStatement().execute(buf.toString()); + } finally { + connection.setAutoCommit(isAutoCommit); + } + } }