diff --git a/fe/fe-core/src/main/java/com/starrocks/qe/ConnectProcessor.java b/fe/fe-core/src/main/java/com/starrocks/qe/ConnectProcessor.java index 883684572fc35f..4cf654bf2ae2fa 100644 --- a/fe/fe-core/src/main/java/com/starrocks/qe/ConnectProcessor.java +++ b/fe/fe-core/src/main/java/com/starrocks/qe/ConnectProcessor.java @@ -775,17 +775,37 @@ private void handleFieldList() throws IOException { ctx.getState().setError("Unknown database(" + ctx.getDatabase() + ")"); return; } - Locker locker = new Locker(); - locker.lockDatabase(db.getId(), LockType.READ); + // Internal catalog: lookup resolves to Database.nameToTable (a ConcurrentHashMap, + // lock-free safe). External catalog: lookup routes through ConnectorMetadata, whose + // concurrency is connector-managed. No FE write path takes the LockManager lock on + // external db ids, so the intensive lock acquired below is harmless overhead on + // external catalogs and the unlocked lookup changes nothing for them. + Table table; try { - // we should get table through metadata manager - Table table = ctx.getGlobalStateMgr().getMetadataMgr().getTable( + table = ctx.getGlobalStateMgr().getMetadataMgr().getTable( ctx, ctx.getCurrentCatalog(), ctx.getDatabase(), tableName); - if (table == null) { + } catch (StarRocksConnectorException e) { + LOG.error("errors happened when getting table {}", tableName, e); + ctx.getState().setEof(); + return; + } + if (table == null) { + ctx.getState().setError("Unknown table(" + tableName + ")"); + return; + } + + Locker locker = new Locker(); + locker.lockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); + try { + // Revalidate by name to detect concurrent DROP/RENAME between unlocked lookup + // and lock acquisition. The table id is stable, so id-based revalidation would + // miss a RENAME that re-binds the name to a different table. Internal-catalog + // only: LocalMetastore doesn't track connector tables, so a re-fetch for + // external catalogs would always return null and falsely fail. + if (db.getCatalogName() == null && db.getTable(tableName) != table) { ctx.getState().setError("Unknown table(" + tableName + ")"); return; } - MysqlSerializer serializer = ctx.getSerializer(); MysqlChannel channel = ctx.getMysqlChannel(); @@ -800,7 +820,7 @@ private void handleFieldList() throws IOException { } catch (StarRocksConnectorException e) { LOG.error("errors happened when getting table {}", tableName, e); } finally { - locker.unLockDatabase(db.getId(), LockType.READ); + locker.unLockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); } ctx.getState().setEof(); } diff --git a/fe/fe-core/src/main/java/com/starrocks/qe/ShowExecutor.java b/fe/fe-core/src/main/java/com/starrocks/qe/ShowExecutor.java index c9d39c9f07e23e..bf6d29933560c6 100644 --- a/fe/fe-core/src/main/java/com/starrocks/qe/ShowExecutor.java +++ b/fe/fe-core/src/main/java/com/starrocks/qe/ShowExecutor.java @@ -848,49 +848,70 @@ private ShowResultSet showCreateInternalCatalogTable(ShowCreateTableStmt showStm Database db = GlobalStateMgr.getCurrentState().getLocalMetastore().getDb(showStmt.getDb()); MetaUtils.checkDbNullAndReport(db, showStmt.getDb()); List> rows = Lists.newArrayList(); - Locker locker = new Locker(); - locker.lockDatabase(db.getId(), LockType.READ); - try { - TableName tableName = new TableName(showStmt.getCatalogName(), showStmt.getDb(), showStmt.getTable()); - Table table = MetaUtils.getSessionAwareTable(connectContext, db, tableName); - if (table == null) { - if (showStmt.getType() != ShowCreateTableStmt.CreateTableType.MATERIALIZED_VIEW) { - ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, showStmt.getTable()); - } else { - // For Sync Materialized View, it is a mv index inside OLAP table, - // so we can not get it from database. - for (Table tbl : GlobalStateMgr.getCurrentState().getLocalMetastore().getTables(db.getId())) { - if (tbl.getType() == Table.TableType.OLAP) { - OlapTable olapTable = (OlapTable) tbl; - List visibleMaterializedViews = - olapTable.getVisibleIndexMetas(); - for (MaterializedIndexMeta mvMeta : visibleMaterializedViews) { - if (olapTable.getIndexNameByMetaId(mvMeta.getIndexMetaId()).equals(showStmt.getTable())) { - if (mvMeta.getOriginStmt() == null) { - String mvName = olapTable.getIndexNameByMetaId(mvMeta.getIndexMetaId()); - rows.add(Lists.newArrayList(showStmt.getTable(), - ShowMaterializedViewStatus.buildCreateMVSql(olapTable, - mvName, mvMeta), "utf8", "utf8_general_ci")); - } else { - rows.add(Lists.newArrayList(showStmt.getTable(), mvMeta.getOriginStmt(), - "utf8", "utf8_general_ci")); - } - - ShowResultSetMetaData showResultSetMetaData = ShowResultSetMetaData.builder() - .addColumn(new Column("Materialized View", - TypeFactory.createVarcharType(20))) - .addColumn(new Column("Create Materialized View", - TypeFactory.createVarcharType(30))) - .build(); - return new ShowResultSet(showResultSetMetaData, rows); + TableName tableName = new TableName(showStmt.getCatalogName(), showStmt.getDb(), showStmt.getTable()); + // Lookup is ConcurrentHashMap-backed (Database.nameToTable) for internal catalog and + // throws SemanticException if the table is missing, so it is safe outside the lock. + Table table = MetaUtils.getSessionAwareTable(connectContext, db, tableName); + // TODO(sync-mv): the (table == null) branch below is currently unreachable - + // MetaUtils.getSessionAwareTable throws SemanticException on miss rather than + // returning null (since #43162, "temporary table part-1"), so the sync-MV + // fallback never runs. As a side effect, SHOW CREATE MATERIALIZED VIEW + // has been broken since that change (sync MVs live as MaterializedIndexMeta + // inside an OLAP table, not as separately-registered Tables, so they are missed + // by the throwing lookup above). The fallback is kept here as a marker until a + // follow-up commit reworks the lookup to make this path reachable. When that + // happens, the iteration must run under DB READ (or per-table READ) - it reads + // HashMap-backed OlapTable index metadata which is mutated by concurrent + // ALTER/rollup. Today the unlocked iteration is harmless because it never + // executes. + if (table == null) { + if (showStmt.getType() != ShowCreateTableStmt.CreateTableType.MATERIALIZED_VIEW) { + ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, showStmt.getTable()); + } else { + // For Sync Materialized View, it is a mv index inside OLAP table, + // so we can not get it from database. + for (Table tbl : GlobalStateMgr.getCurrentState().getLocalMetastore().getTables(db.getId())) { + if (tbl.getType() == Table.TableType.OLAP) { + OlapTable olapTable = (OlapTable) tbl; + List visibleMaterializedViews = + olapTable.getVisibleIndexMetas(); + for (MaterializedIndexMeta mvMeta : visibleMaterializedViews) { + if (olapTable.getIndexNameByMetaId(mvMeta.getIndexMetaId()).equals(showStmt.getTable())) { + if (mvMeta.getOriginStmt() == null) { + String mvName = olapTable.getIndexNameByMetaId(mvMeta.getIndexMetaId()); + rows.add(Lists.newArrayList(showStmt.getTable(), + ShowMaterializedViewStatus.buildCreateMVSql(olapTable, + mvName, mvMeta), "utf8", "utf8_general_ci")); + } else { + rows.add(Lists.newArrayList(showStmt.getTable(), mvMeta.getOriginStmt(), + "utf8", "utf8_general_ci")); } + + ShowResultSetMetaData showResultSetMetaData = ShowResultSetMetaData.builder() + .addColumn(new Column("Materialized View", + TypeFactory.createVarcharType(20))) + .addColumn(new Column("Create Materialized View", + TypeFactory.createVarcharType(30))) + .build(); + return new ShowResultSet(showResultSetMetaData, rows); } } } - ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, showStmt.getTable()); } + ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, showStmt.getTable()); + } + } + Locker locker = new Locker(); + locker.lockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); + try { + // Revalidate by name to detect concurrent DROP/RENAME between unlocked lookup + // and lock acquisition. The table id is stable for a given Table object, so an + // id-based check would miss a RENAME that re-binds the name to a different + // (or no) table; revalidate via the same lookup path so both regular and + // temporary tables are handled correctly. + if (MetaUtils.getSessionAwareTable(connectContext, db, tableName) != table) { + ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, showStmt.getTable()); } - List createTableStmt = Lists.newArrayList(); AstToStringBuilder.getDdlStmt(table, createTableStmt, null, null, false, true /* hide password */); if (createTableStmt.isEmpty()) { @@ -943,7 +964,7 @@ private ShowResultSet showCreateInternalCatalogTable(ShowCreateTableStmt showStm return new ShowResultSet(showResultMetaFactory.getMetadata(showStmt), rows); } } finally { - locker.unLockDatabase(db.getId(), LockType.READ); + locker.unLockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); } } @@ -1882,7 +1903,7 @@ public ShowResultSet visitShowTabletStatement(ShowTabletStmt statement, ConnectC dbName = db.getFullName(); Locker locker = new Locker(); - locker.lockDatabase(db.getId(), LockType.READ); + locker.lockTableWithIntensiveDbLock(db.getId(), tableId, LockType.READ); try { Table table = GlobalStateMgr.getCurrentState().getLocalMetastore().getTable(db.getId(), tableId); if (!(table instanceof OlapTable)) { @@ -1943,7 +1964,7 @@ public ShowResultSet visitShowTabletStatement(ShowTabletStmt statement, ConnectC } } finally { - locker.unLockDatabase(db.getId(), LockType.READ); + locker.unLockTableWithIntensiveDbLock(db.getId(), tableId, LockType.READ); } } while (false); @@ -1962,19 +1983,25 @@ public ShowResultSet visitShowTabletStatement(ShowTabletStmt statement, ConnectC Database db = globalStateMgr.getLocalMetastore().getDb(dbName); MetaUtils.checkDbNullAndReport(db, dbName); + // Lookup is ConcurrentHashMap-backed for the internal catalog and throws on missing + // table, so it is safe outside the lock. + TableName tableName = new TableName(tableRef.getCatalogName(), tableRef.getDbName(), + tableRef.getTableName()); + Table table = MetaUtils.getSessionAwareTable(context, db, tableName); + if (!table.isNativeTableOrMaterializedView()) { + ErrorReport.reportSemanticException(ErrorCode.ERR_NOT_OLAP_TABLE, statement.getTableName()); + } + Locker locker = new Locker(); - locker.lockDatabase(db.getId(), LockType.READ); + locker.lockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); try { - Table table = MetaUtils.getSessionAwareTable( - context, db, new TableName(tableRef.getCatalogName(), tableRef.getDbName(), - tableRef.getTableName())); - if (table == null) { - ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, statement.getTableName()); - } - if (!table.isNativeTableOrMaterializedView()) { - ErrorReport.reportSemanticException(ErrorCode.ERR_NOT_OLAP_TABLE, statement.getTableName()); + // Revalidate by name to detect concurrent DROP/RENAME between unlocked lookup + // and lock acquisition. Id-based check would miss a RENAME that re-binds the + // name to a different table; re-running the same lookup catches that. + if (MetaUtils.getSessionAwareTable(context, db, tableName) != table) { + ErrorReport.reportSemanticException(ErrorCode.ERR_BAD_TABLE_ERROR, + statement.getTableName()); } - Pair privResult = Authorizer.checkPrivForShowTablet( context, db.getFullName(), table); if (!privResult.first) { @@ -2068,7 +2095,7 @@ context, db, new TableName(tableRef.getCatalogName(), tableRef.getDbName(), rows.add(oneTablet); } } finally { - locker.unLockDatabase(db.getId(), LockType.READ); + locker.unLockTableWithIntensiveDbLock(db.getId(), table.getId(), LockType.READ); } }