From 7f8d242b370cfd5a258b688666562db6e5949b8a Mon Sep 17 00:00:00 2001 From: Zach Lite Date: Thu, 24 Aug 2023 15:48:31 -0400 Subject: [PATCH 1/2] ui: decouple sql API requests on databases page Previously, SQL execution requests for database details and span stats were done in the same HTTP request. The consequence of this coupling was that no information could be displayed until span stats were collected. Since span stats are much slower to collect than database details, this resulted in the potential for a very bad user experience. This commit separates span stats and database details into distinct HTTP requests, and updates the DatabasesPage component so that database details and span stats (and their associated network and query errors) can all be displayed independently. Epic: none Release note (ui change): Added progressive loading functionality to the databases page. --- .../cluster-ui/src/api/databaseDetailsApi.ts | 131 +++++++++++------- .../databaseDetailsConnected.ts | 3 +- .../cluster-ui/src/databases/combiners.ts | 25 +++- .../src/databasesPage/databaseTableCells.tsx | 37 +++-- .../databasesPage/databasesPage.stories.tsx | 26 ++-- .../src/databasesPage/databasesPage.tsx | 78 ++++++----- .../databasesPage/databasesPageConnected.ts | 12 +- .../databaseDetails.reducer.ts | 74 +++++++++- .../databaseDetails.saga.spec.ts | 10 +- .../databaseDetails/databaseDetails.saga.ts | 3 +- .../databaseDetailsSpanStats.saga.spec.ts | 111 +++++++++++++++ .../databaseDetailsSpanStats.saga.ts | 53 +++++++ .../cluster-ui/src/store/reducers.ts | 8 +- .../workspaces/cluster-ui/src/store/sagas.ts | 2 + .../db-console/src/redux/apiReducers.ts | 16 +++ .../db-console/src/util/api.spec.ts | 53 ++++--- .../databases/databasesPage/redux.spec.ts | 54 ++++---- .../views/databases/databasesPage/redux.ts | 6 + 18 files changed, 532 insertions(+), 170 deletions(-) create mode 100644 pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.spec.ts create mode 100644 pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.ts diff --git a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts index 9444ce642c2f..be1952e74739 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts @@ -19,6 +19,7 @@ import { SqlExecutionErrorMessage, SqlExecutionRequest, sqlResultsAreEmpty, + SqlExecutionResponse, SqlStatement, SqlTxnResult, txnResultIsEmpty, @@ -40,6 +41,10 @@ export type DatabaseDetailsReqParams = { csIndexUnusedDuration: string; }; +export type DatabaseDetailsSpanStatsReqParams = { + database: string; +}; + export type DatabaseDetailsResponse = { idResp: SqlApiQueryResponse; grantsResp: SqlApiQueryResponse; @@ -49,6 +54,11 @@ export type DatabaseDetailsResponse = { error?: SqlExecutionErrorMessage; }; +export type DatabaseDetailsSpanStatsResponse = { + spanStats: SqlApiQueryResponse; + error?: SqlExecutionErrorMessage; +}; + function newDatabaseDetailsResponse(): DatabaseDetailsResponse { return { idResp: { database_id: "" }, @@ -62,12 +72,6 @@ function newDatabaseDetailsResponse(): DatabaseDetailsResponse { zone_config_level: ZoneConfigurationLevel.CLUSTER, }, stats: { - spanStats: { - approximate_disk_bytes: 0, - live_bytes: 0, - total_bytes: 0, - range_count: 0, - }, replicaData: { replicas: [], regions: [], @@ -77,6 +81,18 @@ function newDatabaseDetailsResponse(): DatabaseDetailsResponse { }; } +function newDatabaseDetailsSpanStatsResponse(): DatabaseDetailsSpanStatsResponse { + return { + spanStats: { + approximate_disk_bytes: 0, + live_bytes: 0, + total_bytes: 0, + range_count: 0, + }, + error: undefined, + }; +} + // Database ID type DatabaseIdRow = { database_id: string; @@ -240,7 +256,7 @@ type DatabaseZoneConfigRow = { const getDatabaseZoneConfig: DatabaseDetailsQuery = { createStmt: dbName => { return { - sql: `SELECT + sql: `SELECT encode( crdb_internal.get_zone_config( (SELECT crdb_internal.get_database_id($1)) @@ -293,7 +309,6 @@ const getDatabaseZoneConfig: DatabaseDetailsQuery = { // Database Stats type DatabaseDetailsStats = { - spanStats: SqlApiQueryResponse; replicaData: SqlApiQueryResponse; indexStats: SqlApiQueryResponse; }; @@ -305,44 +320,38 @@ export type DatabaseSpanStatsRow = { range_count: number; }; -const getDatabaseSpanStats: DatabaseDetailsQuery = { - createStmt: dbName => { - return { - sql: `SELECT - sum(range_count) as range_count, - sum(approximate_disk_bytes) as approximate_disk_bytes, - sum(live_bytes) as live_bytes, - sum(total_bytes) as total_bytes - FROM crdb_internal.tenant_span_stats((SELECT crdb_internal.get_database_id($1)))`, - arguments: [dbName], - }; - }, - addToDatabaseDetail: ( - txn_result: SqlTxnResult, - resp: DatabaseDetailsResponse, - ) => { - if (txn_result && txn_result.error) { - resp.stats.spanStats.error = txn_result.error; - } - if (txnResultIsEmpty(txn_result)) { - return; - } - if (txn_result.rows.length === 1) { - const row = txn_result.rows[0]; - resp.stats.spanStats.approximate_disk_bytes = row.approximate_disk_bytes; - resp.stats.spanStats.range_count = row.range_count; - resp.stats.spanStats.live_bytes = row.live_bytes; - resp.stats.spanStats.total_bytes = row.total_bytes; - } else { - resp.stats.spanStats.error = new Error( - `DatabaseDetails - Span Stats, expected 1 row, got ${txn_result.rows.length}`, - ); - } - }, - handleMaxSizeError: (_dbName, _response, _dbDetail) => { - return Promise.resolve(false); - }, -}; +function formatSpanStatsExecutionResult( + res: SqlExecutionResponse, +): DatabaseDetailsSpanStatsResponse { + const out = newDatabaseDetailsSpanStatsResponse(); + + if (res.execution.txn_results.length === 0) { + return out; + } + + const txn_result = res.execution.txn_results[0]; + + if (txn_result && txn_result.error) { + // Copy the SQLExecutionError and the SqlTransactionResult error. + out.error = res.error; + out.spanStats.error = txn_result.error; + } + if (txnResultIsEmpty(txn_result)) { + return out; + } + if (txn_result.rows.length === 1) { + const row = txn_result.rows[0]; + out.spanStats.approximate_disk_bytes = row.approximate_disk_bytes; + out.spanStats.range_count = row.range_count; + out.spanStats.live_bytes = row.live_bytes; + out.spanStats.total_bytes = row.total_bytes; + } else { + out.spanStats.error = new Error( + `DatabaseDetails - Span Stats, expected 1 row, got ${txn_result.rows.length}`, + ); + } + return out; +} type DatabaseReplicasRegionsRow = { replicas: number[]; @@ -463,7 +472,6 @@ const databaseDetailQueries: DatabaseDetailsQuery[] = [ getDatabaseReplicasAndRegions, getDatabaseIndexUsageStats, getDatabaseZoneConfig, - getDatabaseSpanStats, ]; export function createDatabaseDetailsReq( @@ -480,6 +488,35 @@ export function createDatabaseDetailsReq( }; } +export function createDatabaseDetailsSpanStatsReq( + params: DatabaseDetailsSpanStatsReqParams, +): SqlExecutionRequest { + const statement = { + sql: `SELECT + sum(range_count) as range_count, + sum(approximate_disk_bytes) as approximate_disk_bytes, + sum(live_bytes) as live_bytes, + sum(total_bytes) as total_bytes + FROM crdb_internal.tenant_span_stats((SELECT crdb_internal.get_database_id($1)))`, + arguments: [params.database], + }; + return createSqlExecutionRequest(params.database, [statement]); +} + +export async function getDatabaseDetailsSpanStats( + params: DatabaseDetailsSpanStatsReqParams, +) { + const req: SqlExecutionRequest = createDatabaseDetailsSpanStatsReq(params); + const sqlResp = await executeInternalSql(req); + const res = formatSpanStatsExecutionResult(sqlResp); + return formatApiResult( + res, + res.error, + "retrieving database span stats", + false, + ); +} + export async function getDatabaseDetails( params: DatabaseDetailsReqParams, timeout?: moment.Duration, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts index 8cdeb925e326..685ad0e29aee 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts @@ -14,7 +14,8 @@ import { Dispatch } from "redux"; import { databaseNameCCAttr } from "src/util/constants"; import { getMatchParamByName } from "src/util/query"; import { AppState } from "../store"; -import { actions as databaseDetailsActions } from "../store/databaseDetails"; +import { databaseDetailsReducer } from "../store/databaseDetails"; +const databaseDetailsActions = databaseDetailsReducer.actions; import { actions as localStorageActions, LocalStorageKeys, diff --git a/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts b/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts index 117cff6fa0af..0d9efc204d7b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts +++ b/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts @@ -16,7 +16,10 @@ import { normalizePrivileges, normalizeRoles, } from "./util"; -import { DatabaseDetailsState } from "../store/databaseDetails"; +import { + DatabaseDetailsSpanStatsState, + DatabaseDetailsState, +} from "../store/databaseDetails"; import { createSelector } from "@reduxjs/toolkit"; import { TableDetailsState } from "../store/databaseTableDetails"; import { generateTableID, longToInt, TimestampToMoment } from "../util"; @@ -32,6 +35,7 @@ const { RecommendationType } = cockroach.sql.IndexRecommendation; interface DerivedDatabaseDetailsParams { dbListResp: DatabasesListResponse; databaseDetails: Record; + spanStats: Record; nodeRegions: Record; isTenant: boolean; } @@ -39,20 +43,24 @@ interface DerivedDatabaseDetailsParams { export const deriveDatabaseDetailsMemoized = createSelector( (params: DerivedDatabaseDetailsParams) => params.dbListResp, (params: DerivedDatabaseDetailsParams) => params.databaseDetails, + (params: DerivedDatabaseDetailsParams) => params.spanStats, (params: DerivedDatabaseDetailsParams) => params.nodeRegions, (params: DerivedDatabaseDetailsParams) => params.isTenant, ( dbListResp, databaseDetails, + spanStats, nodeRegions, isTenant, ): DatabasesPageDataDatabase[] => { const databases = dbListResp?.databases ?? []; return databases.map(dbName => { const dbDetails = databaseDetails[dbName]; + const spanStatsForDB = spanStats[dbName]; return deriveDatabaseDetails( dbName, dbDetails, + spanStatsForDB, dbListResp.error, nodeRegions, isTenant, @@ -64,6 +72,7 @@ export const deriveDatabaseDetailsMemoized = createSelector( const deriveDatabaseDetails = ( database: string, dbDetails: DatabaseDetailsState, + spanStats: DatabaseDetailsSpanStatsState, dbListError: SqlExecutionErrorMessage, nodeRegionsByID: Record, isTenant: boolean, @@ -79,12 +88,16 @@ const deriveDatabaseDetails = ( dbStats?.indexStats.num_index_recommendations || 0; return { - loading: !!dbDetails?.inFlight, - loaded: !!dbDetails?.valid, - requestError: dbDetails?.lastError, - queryError: dbDetails?.data?.results?.error, + detailsLoading: !!dbDetails?.inFlight, + detailsLoaded: !!dbDetails?.valid, + spanStatsLoading: !!spanStats?.inFlight, + spanStatsLoaded: !!spanStats?.valid, + detailsRequestError: dbDetails?.lastError, // http request error. + spanStatsRequestError: spanStats?.lastError, // http request error. + detailsQueryError: dbDetails?.data?.results?.error, + spanStatsQueryError: spanStats?.data?.results?.error, name: database, - spanStats: dbStats?.spanStats, + spanStats: spanStats?.data?.results.spanStats, tables: dbDetails?.data?.results.tablesResp, nodes: nodes, nodesByRegionString, diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databaseTableCells.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databaseTableCells.tsx index 8dc1d19e748e..e66c1a5a14ca 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databaseTableCells.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databaseTableCells.tsx @@ -19,7 +19,11 @@ import { EncodeDatabaseUri } from "../util"; import { Link } from "react-router-dom"; import { StackIcon } from "../icon/stackIcon"; import { CockroachCloudContext } from "../contexts"; -import { checkInfoAvailable, getNetworkErrorMessage } from "../databases"; +import { + checkInfoAvailable, + getNetworkErrorMessage, + getQueryErrorMessage, +} from "../databases"; import * as format from "../util/format"; import { Caution } from "@cockroachlabs/icons"; @@ -33,7 +37,7 @@ export const DiskSizeCell = ({ database }: CellProps): JSX.Element => { return ( <> {checkInfoAvailable( - database.requestError, + database.spanStatsRequestError, database.spanStats?.error, database.spanStats?.approximate_disk_bytes ? format.Bytes(database.spanStats?.approximate_disk_bytes) @@ -66,16 +70,33 @@ export const DatabaseNameCell = ({ database }: CellProps): JSX.Element => { ? `${location.pathname}/${database.name}` : EncodeDatabaseUri(database.name); let icon = ; - if (database.requestError || database.queryError) { + + const needsWarning = + database.detailsRequestError || + database.spanStatsRequestError || + database.detailsQueryError || + database.spanStatsQueryError; + + if (needsWarning) { + const titleList = []; + if (database.detailsRequestError) { + titleList.push(getNetworkErrorMessage(database.detailsRequestError)); + } + if (database.spanStatsRequestError) { + titleList.push(getNetworkErrorMessage(database.spanStatsRequestError)); + } + if (database.detailsQueryError) { + titleList.push(database.detailsQueryError.message); + } + if (database.spanStatsQueryError) { + titleList.push(getQueryErrorMessage(database.spanStatsQueryError)); + } + icon = ( diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx index cfe7cb9e47e1..884a70e1b2e0 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx @@ -25,7 +25,8 @@ const withLoadingIndicator: DatabasesPageProps = { loading: true, loaded: false, requestError: undefined, - queryError: null, + queryError: undefined, + automaticStatsCollectionEnabled: true, indexRecommendationsEnabled: false, csIndexUnusedDuration: indexUnusedDuration, databases: [], @@ -39,6 +40,7 @@ const withLoadingIndicator: DatabasesPageProps = { refreshDatabases: () => {}, refreshSettings: () => {}, refreshDatabaseDetails: () => {}, + refreshDatabaseSpanStats: () => {}, location: history.location, history, match: { @@ -52,8 +54,8 @@ const withLoadingIndicator: DatabasesPageProps = { const withoutData: DatabasesPageProps = { loading: false, loaded: true, - requestError: null, - queryError: null, + requestError: undefined, + queryError: undefined, automaticStatsCollectionEnabled: true, indexRecommendationsEnabled: false, csIndexUnusedDuration: indexUnusedDuration, @@ -69,6 +71,7 @@ const withoutData: DatabasesPageProps = { refreshDatabases: () => {}, refreshSettings: () => {}, refreshDatabaseDetails: () => {}, + refreshDatabaseSpanStats: () => {}, location: history.location, history, match: { @@ -82,8 +85,8 @@ const withoutData: DatabasesPageProps = { const withData: DatabasesPageProps = { loading: false, loaded: true, - requestError: null, - queryError: null, + requestError: undefined, + queryError: undefined, showNodeRegionsColumn: true, automaticStatsCollectionEnabled: true, indexRecommendationsEnabled: true, @@ -101,10 +104,14 @@ const withData: DatabasesPageProps = { }, databases: Array(42).map(() => { return { - loading: false, - loaded: true, - requestError: undefined, - queryError: null, + detailsLoading: false, + detailsLoaded: false, + spanStatsLoading: false, + spanStatsLoaded: false, + detailsRequestError: undefined, + detailsQueryError: undefined, + spanStatsRequestError: undefined, + spanStatsQueryError: undefined, name: randomName(), sizeInBytes: _.random(1000.0) * 1024 ** _.random(1, 2), tableCount: _.random(5, 100), @@ -118,6 +125,7 @@ const withData: DatabasesPageProps = { refreshDatabases: () => {}, refreshSettings: () => {}, refreshDatabaseDetails: () => {}, + refreshDatabaseSpanStats: () => {}, location: history.location, history, match: { diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx index 2cf95516e8bc..e2262e636f5c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx @@ -70,29 +70,6 @@ const booleanSettingCx = classnames.bind(booleanSettingStyles); // // The loading and loaded flags help us know when to dispatch the appropriate // refresh actions. -// -// The overall structure is: -// -// interface DatabasesPageData { -// loading: boolean; -// loaded: boolean; -// lastError: Error; -// sortSetting: SortSetting; -// search: string; -// filters: Filters; -// nodeRegions: { [nodeId: string]: string }; -// isTenant: boolean; -// databases: { // DatabasesPageDataDatabase[] -// loading: boolean; -// loaded: boolean; -// name: string; -// sizeInBytes: number; -// tableCount: number; -// rangeCount: number; -// nodes: number[]; -// nodesByRegionString: string; -// }[]; -// } export interface DatabasesPageData { loading: boolean; loaded: boolean; @@ -113,12 +90,18 @@ export interface DatabasesPageData { } export interface DatabasesPageDataDatabase { - loading: boolean; - loaded: boolean; + detailsLoading: boolean; + detailsLoaded: boolean; + + spanStatsLoading: boolean; + spanStatsLoaded: boolean; + // Request error when getting database details. - requestError: Error; + detailsRequestError: Error; + spanStatsRequestError: Error; // Query error when getting database details. - queryError: SqlExecutionErrorMessage; + detailsQueryError: SqlExecutionErrorMessage; + spanStatsQueryError: SqlExecutionErrorMessage; name: string; spanStats?: SqlApiQueryResponse; tables?: SqlApiQueryResponse; @@ -137,6 +120,7 @@ export interface DatabasesPageActions { database: string, csIndexUnusedDuration: string, ) => void; + refreshDatabaseSpanStats: (database: string) => void; refreshSettings: () => void; refreshNodes?: () => void; onFilterChange?: (value: Filters) => void; @@ -338,15 +322,22 @@ export class DatabasesPage extends React.Component< filteredDbs.forEach(database => { if ( - !database.loaded && - !database.loading && - database.requestError === undefined + !database.detailsLoaded && + !database.detailsLoading && + database.detailsRequestError == null ) { this.props.refreshDatabaseDetails( database.name, this.props.csIndexUnusedDuration, ); } + if ( + !database.spanStatsLoaded && + !database.spanStatsLoading && + database.spanStatsRequestError == null + ) { + this.props.refreshDatabaseSpanStats(database.name); + } }); } @@ -488,7 +479,13 @@ export class DatabasesPage extends React.Component< if ( !this.props.databases || this.props.databases.length === 0 || - this.props.databases.every(x => x.loaded || x.loading) + this.props.databases.every( + x => + x.detailsLoading || + x.detailsLoaded || + x.spanStatsLoaded || + x.spanStatsLoading, + ) ) { return false; } @@ -508,7 +505,18 @@ export class DatabasesPage extends React.Component< i++ ) { const db = filteredDatabases[i]; - if (db.loaded || db.loading || db.requestError != null) { + if ( + db.detailsLoaded || + db.detailsLoading || + db.detailsRequestError != null + ) { + continue; + } + if ( + db.spanStatsLoading || + db.spanStatsLoaded || + db.spanStatsRequestError != null + ) { continue; } // Info is not loaded for a visible database. @@ -556,7 +564,7 @@ export class DatabasesPage extends React.Component< ), cell: database => checkInfoAvailable( - database.requestError, + database.detailsRequestError, database.tables?.error, database.tables?.tables?.length, ), @@ -575,7 +583,7 @@ export class DatabasesPage extends React.Component< ), cell: database => checkInfoAvailable( - database.requestError, + database.spanStatsRequestError, database.spanStats?.error, database.spanStats?.range_count, ), @@ -594,7 +602,7 @@ export class DatabasesPage extends React.Component< ), cell: database => checkInfoAvailable( - database.requestError, + database.detailsRequestError, null, database.nodesByRegionString ? database.nodesByRegionString : null, ), diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPageConnected.ts b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPageConnected.ts index c77ee380c8c9..c28526efe2c0 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPageConnected.ts +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPageConnected.ts @@ -30,7 +30,13 @@ import { selectIsTenant } from "../store/uiConfig"; import { Dispatch } from "redux"; import { actions as clusterSettingsActions } from "../store/clusterSettings"; import { actions as databasesListActions } from "../store/databasesList"; -import { actions as databaseDetailsActions } from "../store/databaseDetails"; +import { + databaseDetailsReducer, + databaseDetailsSpanStatsReducer, +} from "../store/databaseDetails"; +const databaseDetailsActions = databaseDetailsReducer.actions; +const databaseDetailsSpanStatsActions = databaseDetailsSpanStatsReducer.actions; + import { actions as localStorageActions, LocalStorageKeys, @@ -56,6 +62,7 @@ const mapStateToProps = (state: AppState): DatabasesPageData => { databases: deriveDatabaseDetailsMemoized({ dbListResp: databasesListState?.data, databaseDetails: state.adminUI?.databaseDetails, + spanStats: state.adminUI?.databaseDetailsSpanStats, nodeRegions, isTenant, }), @@ -82,6 +89,9 @@ const mapDispatchToProps = (dispatch: Dispatch): DatabasesPageActions => ({ databaseDetailsActions.refresh({ database, csIndexUnusedDuration }), ); }, + refreshDatabaseSpanStats: (database: string) => { + dispatch(databaseDetailsSpanStatsActions.refresh({ database })); + }, refreshSettings: () => { dispatch(clusterSettingsActions.refresh()); }, diff --git a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.reducer.ts b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.reducer.ts index 38e0816ab471..0e85e3fe0f5f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.reducer.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.reducer.ts @@ -11,6 +11,8 @@ import { DatabaseDetailsReqParams, DatabaseDetailsResponse, + DatabaseDetailsSpanStatsReqParams, + DatabaseDetailsSpanStatsResponse, ErrorWithKey, SqlApiResponse, } from "../../api"; @@ -19,7 +21,12 @@ import { DOMAIN_NAME } from "../utils"; type DatabaseDetailsWithKey = { databaseDetailsResponse: SqlApiResponse; - key: string; + key: string; // Database name. +}; + +type DatabaseDetailsSpanStatsWithKey = { + response: SqlApiResponse; + key: string; // Database name. }; export type DatabaseDetailsState = { @@ -30,15 +37,26 @@ export type DatabaseDetailsState = { inFlight: boolean; }; +export type DatabaseDetailsSpanStatsState = { + data?: SqlApiResponse; + lastError?: Error; + valid: boolean; + inFlight: boolean; +}; + export type KeyedDatabaseDetailsState = { [dbName: string]: DatabaseDetailsState; }; -const initialState: KeyedDatabaseDetailsState = {}; +export type KeyedDatabaseDetailsSpanStatsState = { + [dbName: string]: DatabaseDetailsSpanStatsState; +}; -const databaseDetailsReducer = createSlice({ +// const initialState: KeyedDatabaseDetailsState = {}; + +export const databaseDetailsReducer = createSlice({ name: `${DOMAIN_NAME}/databaseDetails`, - initialState, + initialState: {} as KeyedDatabaseDetailsState, reducers: { received: (state, action: PayloadAction) => { state[action.payload.key] = { @@ -75,4 +93,50 @@ const databaseDetailsReducer = createSlice({ }, }); -export const { reducer, actions } = databaseDetailsReducer; +export const databaseDetailsSpanStatsReducer = createSlice({ + name: `${DOMAIN_NAME}/databaseDetailsSpanStats`, + initialState: {} as KeyedDatabaseDetailsSpanStatsState, + reducers: { + received: ( + state, + action: PayloadAction, + ) => { + state[action.payload.key] = { + valid: true, + inFlight: false, + data: action.payload.response, + lastError: null, + }; + }, + failed: (state, action: PayloadAction) => { + state[action.payload.key] = { + valid: false, + inFlight: false, + data: null, + lastError: action.payload.err, + }; + }, + refresh: ( + state, + action: PayloadAction, + ) => { + state[action.payload.database] = { + valid: false, + inFlight: true, + data: null, + lastError: null, + }; + }, + request: ( + state, + action: PayloadAction, + ) => { + state[action.payload.database] = { + valid: false, + inFlight: true, + data: null, + lastError: null, + }; + }, + }, +}); diff --git a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.spec.ts b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.spec.ts index 37660d9f7cf3..d0982c474ce7 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.spec.ts @@ -30,11 +30,11 @@ import { requestDatabaseDetailsSaga, } from "./databaseDetails.saga"; import { - actions, KeyedDatabaseDetailsState, - reducer, + databaseDetailsReducer, } from "./databaseDetails.reducer"; import { indexUnusedDuration } from "src/util/constants"; +const { actions, reducer } = databaseDetailsReducer; describe("DatabaseDetails sagas", () => { const database = "test_db"; @@ -68,12 +68,6 @@ describe("DatabaseDetails sagas", () => { zone_config_level: ZoneConfigurationLevel.CLUSTER, }, stats: { - spanStats: { - approximate_disk_bytes: 1000, - live_bytes: 100, - total_bytes: 500, - range_count: 20, - }, replicaData: { replicas: [1, 2, 3], regions: ["this", "is", "a", "region"], diff --git a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.ts b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.ts index e6fd1cbf9a7e..661665320d28 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetails.saga.ts @@ -10,7 +10,7 @@ import { all, call, put, takeEvery } from "redux-saga/effects"; -import { actions } from "./databaseDetails.reducer"; +import { databaseDetailsReducer } from "./databaseDetails.reducer"; import { DatabaseDetailsReqParams, ErrorWithKey, @@ -19,6 +19,7 @@ import { import moment from "moment"; import { PayloadAction } from "@reduxjs/toolkit"; +const actions = databaseDetailsReducer.actions; export function* refreshDatabaseDetailsSaga( action: PayloadAction, ) { diff --git a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.spec.ts b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.spec.ts new file mode 100644 index 000000000000..7607a2753bef --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.spec.ts @@ -0,0 +1,111 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import { PayloadAction } from "@reduxjs/toolkit"; +import { + EffectProviders, + StaticProvider, + throwError, +} from "redux-saga-test-plan/providers"; +import * as matchers from "redux-saga-test-plan/matchers"; +import { expectSaga } from "redux-saga-test-plan"; +import { + DatabaseDetailsSpanStatsReqParams, + DatabaseDetailsSpanStatsResponse, + getDatabaseDetailsSpanStats, + SqlApiResponse, +} from "../../api"; +import { + databaseDetailsSpanStatsReducer, + KeyedDatabaseDetailsSpanStatsState, +} from "./databaseDetails.reducer"; +import { + refreshDatabaseDetailsSpanStatsSaga, + requestDatabaseDetailsSpanStatsSaga, +} from "./databaseDetailsSpanStats.saga"; +const { actions, reducer } = databaseDetailsSpanStatsReducer; + +describe("DatabaseDetails sagas", () => { + const database = "test_db"; + const requestAction: PayloadAction = { + payload: { database }, + type: "request", + }; + const spanStatsResponse: SqlApiResponse = { + maxSizeReached: false, + results: { + spanStats: { + approximate_disk_bytes: 100, + live_bytes: 200, + range_count: 300, + total_bytes: 400, + error: undefined, + }, + }, + }; + const provider: (EffectProviders | StaticProvider)[] = [ + [matchers.call.fn(getDatabaseDetailsSpanStats), spanStatsResponse], + ]; + + describe("refreshSpanStatsSaga", () => { + it("dispatches request span stats action", () => { + return expectSaga(refreshDatabaseDetailsSpanStatsSaga, requestAction) + .put(actions.request(requestAction.payload)) + .run(); + }); + }); + + describe("request span stats saga", () => { + it("successfully requests span stats", () => { + return expectSaga(requestDatabaseDetailsSpanStatsSaga, requestAction) + .provide(provider) + .put( + actions.received({ + response: spanStatsResponse, + key: database, + }), + ) + .withReducer(reducer) + .hasFinalState({ + [database]: { + data: spanStatsResponse, + lastError: null, + valid: true, + inFlight: false, + }, + }) + .run(); + }); + + it("returns error on failed request", () => { + const error = new Error("Failed request"); + return expectSaga(requestDatabaseDetailsSpanStatsSaga, requestAction) + .provide([ + [matchers.call.fn(getDatabaseDetailsSpanStats), throwError(error)], + ]) + .put( + actions.failed({ + err: error, + key: database, + }), + ) + .withReducer(reducer) + .hasFinalState({ + [database]: { + data: null, + lastError: error, + valid: false, + inFlight: false, + }, + }) + .run(); + }); + }); +}); diff --git a/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.ts b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.ts new file mode 100644 index 000000000000..93e304880a10 --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/store/databaseDetails/databaseDetailsSpanStats.saga.ts @@ -0,0 +1,53 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import { all, call, put, takeEvery } from "redux-saga/effects"; + +import { databaseDetailsSpanStatsReducer } from "./databaseDetails.reducer"; +import { + DatabaseDetailsSpanStatsReqParams, + ErrorWithKey, + getDatabaseDetailsSpanStats, +} from "src/api"; +import { PayloadAction } from "@reduxjs/toolkit"; + +const actions = databaseDetailsSpanStatsReducer.actions; +export function* refreshDatabaseDetailsSpanStatsSaga( + action: PayloadAction, +) { + yield put(actions.request(action.payload)); +} + +export function* requestDatabaseDetailsSpanStatsSaga( + action: PayloadAction, +): any { + try { + const result = yield call(getDatabaseDetailsSpanStats, action.payload); + yield put( + actions.received({ + key: action.payload.database, + response: result, + }), + ); + } catch (e) { + const err: ErrorWithKey = { + err: e, + key: action.payload.database, + }; + yield put(actions.failed(err)); + } +} + +export function* databaseDetailsSpanStatsSaga() { + yield all([ + takeEvery(actions.refresh, refreshDatabaseDetailsSpanStatsSaga), + takeEvery(actions.request, requestDatabaseDetailsSpanStatsSaga), + ]); +} diff --git a/pkg/ui/workspaces/cluster-ui/src/store/reducers.ts b/pkg/ui/workspaces/cluster-ui/src/store/reducers.ts index 86a5bf3fed8c..2134a0ed1753 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/reducers.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/reducers.ts @@ -69,7 +69,9 @@ import { } from "./clusterSettings/clusterSettings.reducer"; import { KeyedDatabaseDetailsState, - reducer as databaseDetails, + KeyedDatabaseDetailsSpanStatsState, + databaseDetailsReducer, + databaseDetailsSpanStatsReducer, } from "./databaseDetails"; import { KeyedTableDetailsState, @@ -99,6 +101,7 @@ export type AdminUiState = { clusterLocks: ClusterLocksReqState; databasesList: DatabasesListState; databaseDetails: KeyedDatabaseDetailsState; + databaseDetailsSpanStats: KeyedDatabaseDetailsSpanStatsState; tableDetails: KeyedTableDetailsState; stmtInsights: StmtInsightsState; txnInsightDetails: TxnInsightDetailsCachedState; @@ -132,7 +135,8 @@ export const reducers = combineReducers({ executionDetailFiles, clusterLocks, databasesList, - databaseDetails, + databaseDetails: databaseDetailsReducer.reducer, + databaseDetailsSpanStats: databaseDetailsSpanStatsReducer.reducer, tableDetails, schemaInsights, statementFingerprintInsights, diff --git a/pkg/ui/workspaces/cluster-ui/src/store/sagas.ts b/pkg/ui/workspaces/cluster-ui/src/store/sagas.ts index b964bd34f45a..50f2b008bc4f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/sagas.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/sagas.ts @@ -35,6 +35,7 @@ import { txnStatsSaga } from "./transactionStats"; import { clusterSettingsSaga } from "./clusterSettings/clusterSettings.saga"; import { databaseDetailsSaga } from "./databaseDetails"; import { tableDetailsSaga } from "./databaseTableDetails"; +import { databaseDetailsSpanStatsSaga } from "./databaseDetails/databaseDetailsSpanStats.saga"; export function* sagas(cacheInvalidationPeriod?: number): SagaIterator { yield all([ @@ -49,6 +50,7 @@ export function* sagas(cacheInvalidationPeriod?: number): SagaIterator { fork(jobSaga), fork(databasesListSaga), fork(databaseDetailsSaga), + fork(databaseDetailsSpanStatsSaga), fork(tableDetailsSaga), fork(sessionsSaga), fork(terminateSaga), diff --git a/pkg/ui/workspaces/db-console/src/redux/apiReducers.ts b/pkg/ui/workspaces/db-console/src/redux/apiReducers.ts index 3d5a07ce3578..5fca3fbe18e0 100644 --- a/pkg/ui/workspaces/db-console/src/redux/apiReducers.ts +++ b/pkg/ui/workspaces/db-console/src/redux/apiReducers.ts @@ -121,6 +121,17 @@ const databaseDetailsReducerObj = new KeyedCachedDataReducer( moment.duration(10, "m"), ); +const databaseDetailsSpanStatsReducerObj = new KeyedCachedDataReducer( + clusterUiApi.getDatabaseDetailsSpanStats, + "databaseDetailsSpanStats", + databaseRequestPayloadToID, + null, + moment.duration(10, "m"), +); + +export const refreshDatabaseDetailsSpanStats = + databaseDetailsSpanStatsReducerObj.refresh; + const hotRangesRequestToID = (req: api.HotRangesRequestMessage) => req.page_token; @@ -572,6 +583,9 @@ export interface APIReducersState { databaseDetails: KeyedCachedDataReducerState< clusterUiApi.SqlApiResponse >; + databaseDetailsSpanStats: KeyedCachedDataReducerState< + clusterUiApi.SqlApiResponse + >; tableDetails: KeyedCachedDataReducerState< clusterUiApi.SqlApiResponse >; @@ -637,6 +651,8 @@ export const apiReducersReducer = combineReducers({ [databasesReducerObj.actionNamespace]: databasesReducerObj.reducer, [databaseDetailsReducerObj.actionNamespace]: databaseDetailsReducerObj.reducer, + [databaseDetailsSpanStatsReducerObj.actionNamespace]: + databaseDetailsSpanStatsReducerObj.reducer, [tableDetailsReducerObj.actionNamespace]: tableDetailsReducerObj.reducer, [indexStatsReducerObj.actionNamespace]: indexStatsReducerObj.reducer, [nonTableStatsReducerObj.actionNamespace]: nonTableStatsReducerObj.reducer, diff --git a/pkg/ui/workspaces/db-console/src/util/api.spec.ts b/pkg/ui/workspaces/db-console/src/util/api.spec.ts index 98c44f362a0a..46b60e3305c6 100644 --- a/pkg/ui/workspaces/db-console/src/util/api.spec.ts +++ b/pkg/ui/workspaces/db-console/src/util/api.spec.ts @@ -105,6 +105,39 @@ describe("rest api", function () { }); }); + describe("database span stats request", () => { + const database = "test"; + afterEach(fetchMock.restore); + beforeEach(fetchMock.restore); + it("correctly requests span stats", () => { + // Mock out the fetch query + stubSqlApiCall( + clusterUiApi.createDatabaseDetailsSpanStatsReq({ + database, + }), + [ + { + rows: [ + { + approximate_disk_bytes: 100, + live_bytes: 200, + total_bytes: 300, + range_count: 400, + }, + ], + }, + ], + ); + + clusterUiApi.getDatabaseDetailsSpanStats({ database }).then(res => { + expect(res.results.spanStats.approximate_disk_bytes).toEqual(100); + expect(res.results.spanStats.live_bytes).toEqual(200); + expect(res.results.spanStats.total_bytes).toEqual(300); + expect(res.results.spanStats.range_count).toEqual(400); + }); + }); + }); + describe("database details request", function () { const database = "test"; const mockOldDate = new Date(2023, 2, 3); @@ -125,7 +158,7 @@ describe("rest api", function () { afterEach(fetchMock.restore); - it("correctly requests info about a specific database", function () { + it("correctly requests details for a specific database", function () { // Mock out the fetch query stubSqlApiCall( clusterUiApi.createDatabaseDetailsReq({ @@ -179,17 +212,6 @@ describe("rest api", function () { }, ], }, - // Database span stats query - { - rows: [ - { - approximate_disk_bytes: 100, - live_bytes: 200, - total_bytes: 300, - range_count: 400, - }, - ], - }, ], ); @@ -212,12 +234,6 @@ describe("rest api", function () { expect(result.results.zoneConfigResp.zone_config_level).toBe( ZoneConfigurationLevel.DATABASE, ); - expect(result.results.stats.spanStats.approximate_disk_bytes).toBe( - 100, - ); - expect(result.results.stats.spanStats.live_bytes).toBe(200); - expect(result.results.stats.spanStats.total_bytes).toBe(300); - expect(result.results.stats.spanStats.range_count).toBe(400); }); }); @@ -260,6 +276,7 @@ describe("rest api", function () { database, csIndexUnusedDuration: indexUnusedDuration, }); + fetchMock.reset(); fetchMock.mock({ matcher: clusterUiApi.SQL_API_PATH, method: "POST", diff --git a/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.spec.ts b/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.spec.ts index 55b601396831..dadab5eb3acd 100644 --- a/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.spec.ts +++ b/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.spec.ts @@ -144,10 +144,14 @@ describe("Databases Page", function () { queryError: undefined, databases: [ { - loading: false, - loaded: false, - requestError: undefined, - queryError: undefined, + detailsLoading: false, + detailsLoaded: false, + spanStatsLoading: false, + spanStatsLoaded: false, + detailsRequestError: undefined, + spanStatsRequestError: undefined, + detailsQueryError: undefined, + spanStatsQueryError: undefined, name: "system", nodes: [], spanStats: undefined, @@ -156,10 +160,14 @@ describe("Databases Page", function () { numIndexRecommendations: 0, }, { - loading: false, - loaded: false, - requestError: undefined, - queryError: undefined, + detailsLoading: false, + detailsLoaded: false, + spanStatsLoading: false, + spanStatsLoaded: false, + detailsRequestError: undefined, + spanStatsRequestError: undefined, + detailsQueryError: undefined, + spanStatsQueryError: undefined, name: "test", nodes: [], spanStats: undefined, @@ -274,17 +282,6 @@ describe("Databases Page", function () { { rows: [], }, - // Span Stats - { - rows: [ - { - approximate_disk_bytes: 100, - live_bytes: 200, - total_bytes: 300, - range_count: 400, - }, - ], - }, ], ); @@ -293,18 +290,17 @@ describe("Databases Page", function () { await driver.refreshDatabaseDetails("test", indexUnusedDuration); driver.assertDatabaseProperties("test", { - loading: false, - loaded: true, - requestError: null, - queryError: undefined, + detailsLoading: false, + detailsLoaded: true, + spanStatsLoading: false, + spanStatsLoaded: false, + detailsRequestError: null, + spanStatsRequestError: undefined, + detailsQueryError: undefined, + spanStatsQueryError: undefined, name: "test", nodes: [1, 2, 3], - spanStats: { - approximate_disk_bytes: 100, - live_bytes: 200, - total_bytes: 300, - range_count: 400, - }, + spanStats: undefined, tables: { tables: [`"public"."foo"`, `"public"."bar"`], }, diff --git a/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts b/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts index d617922fc537..72e912572215 100644 --- a/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts +++ b/pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts @@ -22,6 +22,7 @@ import { refreshDatabaseDetails, refreshNodes, refreshSettings, + refreshDatabaseDetailsSpanStats, } from "src/redux/apiReducers"; import { AdminUIState } from "src/redux/state"; import { @@ -73,6 +74,7 @@ const searchLocalSetting = new LocalSetting( export const mapStateToProps = (state: AdminUIState): DatabasesPageData => { const dbListResp = state?.cachedData.databases.data; const databaseDetails = state?.cachedData.databaseDetails; + const spanStats = state?.cachedData.databaseDetailsSpanStats; const nodeRegions = nodeRegionsByIDSelector(state); return { loading: selectLoading(state), @@ -82,6 +84,7 @@ export const mapStateToProps = (state: AdminUIState): DatabasesPageData => { databases: deriveDatabaseDetailsMemoized({ dbListResp, databaseDetails, + spanStats, nodeRegions, isTenant, }), @@ -107,6 +110,9 @@ export const mapDispatchToProps = { csIndexUnusedDuration, }); }, + refreshDatabaseSpanStats: (database: string) => { + return refreshDatabaseDetailsSpanStats({ database }); + }, refreshNodes, onSortingChange: ( _tableName: string, From 1c2d81301b6487a9b47d045788853a816f1b9d35 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 9 Oct 2023 14:53:32 -0600 Subject: [PATCH 2/2] sql: allow refcursor comparison with NULL values Most comparisons are disallowed for the REFCURSOR data type. For example, the following is not allowed in postgres: ``` SELECT 'foo'::REFCURSOR = 'bar'::REFCURSOR; ``` However, postgres does allow using `IS [NOT] DISTINCT FROM NULL` and `IS [NOT] NULL` with a refcursor-typed expression os the left operand. There are a few places where CRDB internally expects this comparison to be valid as well, so disallowing these comparisons has caused test failures. This patch removes the refcursor-specific check for invalid comparisons during type checking, and relies on execution-time checks as is done for other comparisons. This means that `IsNull` and `IsNotNull` expressions can now be executed. In addition, the `IS DISTINCT FROM NULL` comparison is now supported only for the case of type `REFCURSOR` as the left operand, and `UNKNOWN` (NULL) as the right operand. Fixes #112010 Fixes #112011 Release note: None --- docs/generated/sql/operators.md | 1 + .../logictest/testdata/logic_test/refcursor | 106 ++++++++++++++++-- pkg/sql/sem/tree/eval.go | 4 + pkg/sql/sem/tree/type_check.go | 20 ---- 4 files changed, 102 insertions(+), 29 deletions(-) diff --git a/docs/generated/sql/operators.md b/docs/generated/sql/operators.md index 21d27ad861c6..9f01f3508286 100644 --- a/docs/generated/sql/operators.md +++ b/docs/generated/sql/operators.md @@ -447,6 +447,7 @@ oid IS NOT DISTINCT FROM intbool oid IS NOT DISTINCT FROM oidbool pg_lsn IS NOT DISTINCT FROM pg_lsnbool +refcursor IS NOT DISTINCT FROM unknownbool string IS NOT DISTINCT FROM stringbool string[] IS NOT DISTINCT FROM string[]bool time IS NOT DISTINCT FROM timebool diff --git a/pkg/sql/logictest/testdata/logic_test/refcursor b/pkg/sql/logictest/testdata/logic_test/refcursor index 56fd0750201c..8558b231337c 100644 --- a/pkg/sql/logictest/testdata/logic_test/refcursor +++ b/pkg/sql/logictest/testdata/logic_test/refcursor @@ -440,29 +440,117 @@ SELECT 'foo'::REFCURSOR >= 'foo'::TEXT; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= NULL; -# TODO(drewk): Postgres allows this case. -statement error pgcode 42883 pq: unsupported comparison operator +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::TEXT; + +# Regression test for #112010 - REFCURSOR should allow IS NULL and +# IS NOT NULL comparisons. +subtest is_null + +query B SELECT 'foo'::REFCURSOR IS NULL; +---- +false -statement error pgcode 42883 pq: unsupported comparison operator +query B SELECT 'foo'::REFCURSOR IS NOT NULL; +---- +true + +query B +SELECT 'foo'::REFCURSOR IS DISTINCT FROM NULL; +---- +true + +query B +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM NULL; +---- +false + +# REFCURSOR columns are allowed. +query B rowsort +SELECT l IS NULL FROM implicit_types; +---- +true +true +true +true +true + +query B rowsort +SELECT l IS NOT NULL FROM implicit_types; +---- +false +false +false +false +false + +query B rowsort +SELECT l IS DISTINCT FROM NULL FROM implicit_types; +---- +false +false +false +false +false + +query B rowsort +SELECT l IS NOT DISTINCT FROM NULL FROM implicit_types; +---- +true +true +true +true +true +# Comparison with column is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM l FROM implicit_types; statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM l FROM implicit_types; +# Comparison with typed NULL is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM NULL; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (NULL::REFCURSOR); statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (NULL::REFCURSOR); +# Comparison with CASE expression is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::TEXT; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM NULL; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); + +# The CASE operator is folded into an untyped NULL, so it's allowed. +# TODO(drewk): Postgres doesn't allow this case. +query B +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); +---- +true + +query B +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); +---- +false + +# The CASE operator is folded into a typed NULL, so it's not allowed. +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); subtest end diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 6748453f51ed..e9784271e94f 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -1775,6 +1775,10 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Void, types.Unknown, volatility.Leakproof), makeIsFn(types.Unknown, types.Void, volatility.Leakproof), + // REFCURSOR cannot be compared with itself, but as a special case, it can + // be compared with NULL using IS NOT DISTINCT FROM. + makeIsFn(types.RefCursor, types.Unknown, volatility.Leakproof), + // Tuple comparison. { LeftType: types.AnyTuple, diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index abd896057bdf..cbef0101ba1b 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -941,14 +941,6 @@ func (expr *ComparisonExpr) TypeCheck( return nil, err } - // REFCURSOR does not support comparisons. - leftType, rightType := leftTyped.ResolvedType(), rightTyped.ResolvedType() - if leftType.Family() == types.RefCursorFamily || rightType.Family() == types.RefCursorFamily { - return nil, pgerror.Newf(pgcode.UndefinedFunction, - "unsupported comparison operator: <%s> %s <%s>", leftType, cmpOp, rightType, - ) - } - if alwaysNull { return DNull, nil } @@ -1471,12 +1463,6 @@ func (expr *IsNullExpr) TypeCheck( if err != nil { return nil, err } - // REFCURSOR does not support comparisons. - if exprTyped.ResolvedType().Family() == types.RefCursorFamily { - return nil, pgerror.New(pgcode.UndefinedFunction, - "unsupported comparison operator: refcursor IS unknown", - ) - } expr.Expr = exprTyped expr.typ = types.Bool return expr, nil @@ -1490,12 +1476,6 @@ func (expr *IsNotNullExpr) TypeCheck( if err != nil { return nil, err } - // REFCURSOR does not support comparisons. - if exprTyped.ResolvedType().Family() == types.RefCursorFamily { - return nil, pgerror.New(pgcode.UndefinedFunction, - "unsupported comparison operator: refcursor IS NOT unknown", - ) - } expr.Expr = exprTyped expr.typ = types.Bool return expr, nil