Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
103484: ui: fix errors from undefined usage r=maryliag a=maryliag

The fix done on cockroachdb#98177 was reverted on cockroachdb#98815, so this commit is adding the check back, which is necessary to not cause an undefined error.

Example of error saw on Datadog:

<img width="998" alt="Screenshot 2023-05-16 at 6 36 07 PM" src="https://github.com/cockroachdb/cockroach/assets/1017486/0e979df8-e66e-4b3c-9cb6-24ace5dd26fd">


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.

Examples of this error saw on Datadog:
<img width="1092" alt="Screenshot 2023-05-16 at 3 59 27 PM" src="https://github.com/cockroachdb/cockroach/assets/1017486/2553fdfc-54f5-4ec1-addf-1af597180e7a">

<img width="1180" alt="Screenshot 2023-05-16 at 4 00 59 PM" src="https://github.com/cockroachdb/cockroach/assets/1017486/f93e7c12-5fdd-490a-b072-298b7b2158ae">

Epic: None

Release note (bug fix): Fixes calls to undefined objects.

103514: sql: fix recently introduced bug around the internal rowsIterator r=yuzefovich a=yuzefovich

This commit fixes a minor bug introduced in cockroachdb@c7eb1bf
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.

Fixes: cockroachdb#103508.

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.

Co-authored-by: maryliag <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
3 people committed May 17, 2023
3 parents f16976e + c03a4c5 + 0f36592 commit 9ae0d28
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 8 deletions.
8 changes: 4 additions & 4 deletions pkg/sql/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1161,7 +1159,9 @@ 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.
// TODO(yuzefovich): reconsider this and return an error explicitly if
// r.lastErr is non-nil.
return r, nil
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
12 changes: 10 additions & 2 deletions pkg/ui/workspaces/cluster-ui/src/store/utils/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down

0 comments on commit 9ae0d28

Please sign in to comment.