Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: insert-stmt-stats is not version gated and fails in mixed-version state #88140

Closed
stevendanna opened this issue Sep 19, 2022 · 2 comments · Fixed by #88165
Closed

sql: insert-stmt-stats is not version gated and fails in mixed-version state #88140

stevendanna opened this issue Sep 19, 2022 · 2 comments · Fixed by #88165
Assignees
Labels
branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker

Comments

@stevendanna
Copy link
Collaborator

stevendanna commented Sep 19, 2022

Describe the problem

In a mixed-version state (with nodes running the 22.2 binary but still on a 22.1 cluster version), I see a number of log lines such as:

W220912 15:21:54.889174 398 sql/sqlstats/persistedsqlstats/flush.go:117 ⋮ [n1] 2118  ‹failed to flush statement statistics›: flush statement ‹738159740542152745›'s statistics: ‹insert-stmt-stats›: ‹INSERT› has more ‹expressions› than ‹target columns›, 11 expressions for 10 targets

This is the result of an insert statement that requires a newly added column but which doesn't use a version gate to ensure that the column was added.

insertStmt := `
INSERT INTO system.statement_statistics
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
ON CONFLICT (crdb_internal_aggregated_ts_app_name_fingerprint_id_node_id_plan_hash_transaction_fingerprint_id_shard_8,
aggregated_ts, fingerprint_id, transaction_fingerprint_id, app_name, plan_hash, node_id)
DO NOTHING
`

To avoid this, we likely need to conditionally insert the index recommendations based on a version gate on

AlterSystemStatementStatisticsAddIndexRecommendations

We simply log this failure, so the only impact is that the stats we had hoped to persist are not persisted and some log spam.

To Reproduce

This can be seen in the import/mixed-versions roachtest.

Jira issue: CRDB-19707

@stevendanna stevendanna added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-observability labels Sep 19, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 19, 2022

Hi @ajwerner, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@ajwerner ajwerner added the branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 label Sep 19, 2022
@ajwerner
Copy link
Contributor

@maryliag I don't know if this rises to the level of release-blocker, but it very well might

craig bot pushed a commit that referenced this issue Sep 20, 2022
86334: *: add restriction to running DDL with internal executors r=ajwerner,rafiss a=ZhouXing19

The current internal executor has its lifecycle, which makes it erroneous
when being used to execute DDL statements if under an outer txn. 

In this commit, we 
1. Migrated the existing DDLs with internal executor with not-nil txn to either 
`descs.CollectionFactory.TxnWithExecutor()` or `planner.WithInternalExecutor()`.
Only internal executors inited via these 2 interfaces are bounded with txn-related
metadata, and hence are allowed to run DDLs in a transactional manner.
2. Added a restriction for running DDLs with internal executor only if it's bound with
txn-related metadata.

fixes #87281

Release justification: bug fix for the internal executor
Release note: none

87304: roachtest: refactor github issue posting and expose actual vm args for reporting r=renatolabs a=smg260

The change is split into 2 commits. 
1. More strictly addresses [81846](#81846).  Also addresses [this](https://github.com/cockroachdb/cockroach/pull/87304/files#diff-1e3ec1fc5bade2df4b8681c44959be2a390f703839f30a979da98f963769c9fcR1046) existing TODO
2. Refactors functions related to posting an issue to github into a separate source file and adds tests.

These commits can be squashed but are separate at the moment for convenience. 

Resolves: #81846

Release justification: test-only change
Release note: none

87896: ci: fix validation for mixed version schema change corpus r=fqazi a=fqazi

Informs: #86381

Previously, the command for validating the mixed
version corpus was invalid. This patch fixes the validation 
command to address this, so the mixed version corpus is 
automatically uploaded.

Release note: None

88138: server: return all completed stmt diag requests r=xinhaoz a=xinhaoz

Fixes #80104

Previously, we only return statement diagnostics requests that have not yet expired. Since we use the results of this request to populate completed statement diagnostics bundles in addition to outstanding requests, completed statement diag bundles would disappear from the UI after the request expired.

This commit ensures that `StatementDiagnosticsRequests` returns all completed stmt diag requests so that we can display the complete history of completed bundles in the UI.

Release note (bug fix): completed stmt diagnostics bundles now persist in the UI in stmt diag bundle pages

88159: ui/cluster-ui: fix jobs page polling r=xinhaoz a=xinhaoz

Fixes #68109

Previously, the jobs page would not poll for new data until a re-render was triggered. This commit updates the jobs page polling to every 10s regardless of
whether or not the rest of the page has changed.

Release note (bug fix): jobs page refreshes page data at an interval of 10s

88165: sql: version gate idx recommendations in insert-stmt-stats r=ericharmeling a=ericharmeling

This commit version gates index recommendation insert in insert-stmt-stats.

Fixes #88140.

Release note: None

88168: api: increase timeout of sql api r=maryliag a=maryliag

Previously, some request using the sql-over-http
api were hitting a timeout, with the default value of 5s. This commit increases to 300s (5min) on the calls made from SQL Activity and Insights pages.

Fixes #88094

Release note: None

88173: cli/sql: put a limit on history size r=DrewKimball a=knz

Fixes #54679.

Previously, there was no limit. Some users managed to make their history run into megabyte-size, despite de-duplication, which was causing slowness.

This patch fixes it by adding a limit of 1000 entries. Sufficiently large to not be inconvenient, but sufficiently small that it prevents the history file from growing abnormally large.

Release note (cli change): The interactive SQL shell now retains a maximum of 1000 entries. There was no limit previously.

88193: kv: hoist log.ExpensiveLogEnabled outside loop in appendRefreshSpans r=arulajmani a=nvanbenschoten

This commit hoists the call to `log.ExpensiveLogEnabled` outside of the per-span loop in `txnSpanRefresher.appendRefreshSpans`. The function is too expensive to call once per span.

Release justification: low risk change that may close perf gap.

Release note: None.

Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Miral Gadani <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig craig bot closed this as completed in 7457a70 Sep 20, 2022
blathers-crl bot pushed a commit that referenced this issue Sep 20, 2022
This commit version gates index recommendation insert in insert-stmt-stats.

Fixes #88140.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants