From c03a4c585476673051e0cc124b343bc6445bd9fe Mon Sep 17 00:00:00 2001 From: maryliag Date: Tue, 16 May 2023 18:26:29 -0400 Subject: [PATCH 1/3] ui: fix errors from undefined usage The fix done on #98177 was reverted on #98815, so this commit is adding the check back, which is necessary to not cause an undefined error. The value for AdminUI could take some time to be initialized, making calls using localStorage fail. This commit adds a check and return an empty object instead of undefined for localStorage. Epic: None Release note (bug fix): Fixes calls to undefined objects. --- .../cluster-ui/src/sessions/sessionsPage.tsx | 4 +++- .../cluster-ui/src/statementsPage/statementsPage.tsx | 4 +++- .../cluster-ui/src/store/utils/selectors.ts | 12 ++++++++++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx index 794192582267..1a82403c258c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx @@ -235,7 +235,9 @@ export class SessionsPage extends React.Component< onChangePage = (current: number): void => { const { pagination } = this.state; this.setState({ pagination: { ...pagination, current } }); - this.props.onPageChanged(current); + if (this.props.onPageChanged) { + this.props.onPageChanged(current); + } }; onSubmitFilters = (filters: Filters): void => { diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx index e9099e08693b..bf5aea4424c6 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx @@ -408,7 +408,9 @@ export class StatementsPage extends React.Component< ...prevState, pagination: { ...pagination, current }, })); - this.props.onPageChanged(current); + if (this.props.onPageChanged) { + this.props.onPageChanged(current); + } }; onSubmitSearchField = (search: string): void => { diff --git a/pkg/ui/workspaces/cluster-ui/src/store/utils/selectors.ts b/pkg/ui/workspaces/cluster-ui/src/store/utils/selectors.ts index add19012a7b7..d652c964bf65 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/utils/selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/utils/selectors.ts @@ -9,7 +9,10 @@ // licenses/APL.txt. import { createSelector } from "reselect"; -import { LocalStorageKeys } from "src/store/localStorage/localStorage.reducer"; +import { + LocalStorageKeys, + LocalStorageState, +} from "src/store/localStorage/localStorage.reducer"; import { AppState } from "../reducers"; export const adminUISelector = createSelector( @@ -19,7 +22,12 @@ export const adminUISelector = createSelector( export const localStorageSelector = createSelector( adminUISelector, - adminUiState => adminUiState?.localStorage, + adminUiState => { + if (adminUiState) { + return adminUiState.localStorage; + } + return {} as LocalStorageState; + }, ); export const selectTimeScale = createSelector( From c0a20cdaea33b207daa29d1451a09e38c9b6b8f9 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 17 May 2023 06:35:06 -0700 Subject: [PATCH 2/3] sql: fix a couple of comments This commit fixes a typo as well as removes the now-stale TODO (I believe it was fixed in 0316935492e26a58741b4c3d0e924c007bd8c541, but we forgot to remove the TODO in that commit). Release note: None --- pkg/sql/internal.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/sql/internal.go b/pkg/sql/internal.go index 9e93823a3a1b..3eb171de966a 100644 --- a/pkg/sql/internal.go +++ b/pkg/sql/internal.go @@ -884,8 +884,6 @@ var rowsAffectedResultColumns = colinfo.ResultColumns{ // goroutines. In particular, this blocking allows us to avoid invalid // concurrent txn access when during the stmt evaluation the internal executor // needs to run "nested" internally-executed stmt (see #62415 for an example). -// TODO(yuzefovich): currently, this statement is not entirely true if the retry -// occurs. // // An additional responsibility of the internalClientComm is handling the retry // errors. If a retry error is encountered with an implicit txn (i.e. nil txn @@ -1161,7 +1159,7 @@ func (ie *InternalExecutor) execInternal( // Note that if a context cancellation error has occurred, we still return // the iterator and nil retErr so that the iterator is properly closed by - // the caller which will cleanup the connExecutor goroutine. + // the caller which will clean up the connExecutor goroutine. return r, nil } From 0f365924f90b5781f8d56d51d2be4469ed849d49 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 17 May 2023 06:38:14 -0700 Subject: [PATCH 3/3] sql: fix recently introduced bug around the internal rowsIterator This commit fixes a minor bug introduced in c7eb1bfbf667e88945080bcf294a5743f9d2e773 that could lead to a nil pointer panic. In particular, that commit introduced `HasResults` method to the `rowsIterator`, and it assumed that `first` field is always non-nil when the iterator was returned on `QueryIteratorEx` call. However, that is not true - in case an error was returned from the connExecutor goroutine, then `rowsIterator.lastErr` is set while `first` is left nil. The expectation is that in such a case the user of the iterator will receive that error either on `Next` or `Close` call, properly cleaning up the iterator. We might want to rethink this and return the error explicitly, but in the spirit of making the least amount of changes, this commit simply added the non-nil check for the `first` field. Release note (bug fix): CockroachDB could previously encounter a nil pointer crash when populating data for SQL Activity page in some rare cases, and this is now fixed. The bug is present in 22.2.9 and 23.1.1 releases. --- pkg/sql/internal.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/sql/internal.go b/pkg/sql/internal.go index 3eb171de966a..c2f0f77bd2c4 100644 --- a/pkg/sql/internal.go +++ b/pkg/sql/internal.go @@ -553,7 +553,7 @@ func (r *rowsIterator) Types() colinfo.ResultColumns { } func (r *rowsIterator) HasResults() bool { - return r.first.row != nil + return r.first != nil && r.first.row != nil } // QueryBuffered executes the supplied SQL statement and returns the resulting @@ -1160,6 +1160,8 @@ func (ie *InternalExecutor) execInternal( // Note that if a context cancellation error has occurred, we still return // the iterator and nil retErr so that the iterator is properly closed by // the caller which will clean up the connExecutor goroutine. + // TODO(yuzefovich): reconsider this and return an error explicitly if + // r.lastErr is non-nil. return r, nil }