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

use boolean for span value on plan gist #87138

Closed
maryliag opened this issue Aug 30, 2022 · 0 comments · Fixed by #87152
Closed

use boolean for span value on plan gist #87138

maryliag opened this issue Aug 30, 2022 · 0 comments · Fixed by #87152
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team

Comments

@maryliag
Copy link
Contributor

maryliag commented Aug 30, 2022

Currently when creating the plan hash/plan gist, it uses the literal value on the span, and different values of span creates different plan gists. This is rapidly increasing the cardinality of rows on statement_statistics since a new row is created for each unique plan.

E.g.

root@localhost:26257/defaultdb> SELECT crdb_internal.decode_plan_gist('AgHIAQIAfg4AAAYM');
  crdb_internal.decode_plan_gist
----------------------------------
  • scan
    table: ?@?
    spans: 7 spans
(3 rows)

root@localhost:26257/defaultdb> SELECT crdb_internal.decode_plan_gist('AgHIAQIAfgIAAAYM');
  crdb_internal.decode_plan_gist
----------------------------------
  • scan
    table: ?@?
    spans: 1 span
(3 rows)

We would like the span value to be changed to a boolean instead, to follow the example of the limit, this way the same plan with different span values would all have the same hash/gist.

Context https://cockroachlabs.slack.com/archives/C01RX2G8LT1/p1661804175738409

Jira issue: CRDB-19170

@maryliag maryliag added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team labels Aug 30, 2022
@mgartner mgartner self-assigned this Aug 30, 2022
craig bot pushed a commit that referenced this issue Aug 31, 2022
85405: kv: retry AdminSplit on LeaseTransferRejectedBecauseTargetMayNeedSnapshot r=shralex a=nvanbenschoten

Informs #84635.
Informs #84162.
Fixes #85449.
Fixes #83174.

This commit considers the `LeaseTransferRejectedBecauseTargetMayNeedSnapshot`
status to be a form of a retriable replication change error. It then hooks
`Replica.executeAdminCommandWithDescriptor` up to consult this status in its
retry loop.

This avoids spurious errors when a split gets blocked behind a lateral replica
move like we see in the following situation:
1. issue AdminSplit
2. range in joint config, first needs to leave (maybeLeaveAtomicChangeReplicas)
3. to leave, needs to transfer lease from voter_outgoing to voter_incoming
4. can’t transfer lease because doing so is deemed to be potentially unsafe

Release note: None

Release justification: Low risk, resolves flaky test.

87137: storage: default to TableFormatPebblev1 in backups r=itsbilal,dt a=jbowens

If the v22.2 upgrade has not yet been finalized, so we're not permitted
to use the new TableFormatPebblev2 sstable format, default to
TableFormatPebblev1 which is the format used by v22.1 internally.

This change is intended to allow us to remove code for understanding the
old RocksDB table format version sooner (eg, v23.1).

Release justification: low-risk updates to existing functionality
Release note: None

87152: sql: encode either 0 or 1 spans in scan gists r=mgartner a=mgartner

#### dev: add rewritable paths for pkg/sql/opt/exec/explain tests

This commit adds fixtures in
`pkg/sql/opt/testutils/opttester/testfixtures` as rewritable paths for
tests in `pkg/sql/opt/exec/explain`. This prevents
`dev test pkg/sql/opt/exec/explain` from erring when the `--rewrite`
flag is used.

Release justification: This is a test-only change.

Release note: None

#### sql: encode either 0 or 1 spans in scan gists

In plan gists, we no longer encode the exact number of spans for scans
so that two queries with the same plan but a different number of spans
have the same gist.

In addition, plan gists are now decoded with the `OnlyShape` flag which
prints any non-zero number of spans as "1+ spans" and removes attributes
like "missing stats" from scans.

Fixes #87138

Release justification: This is a minor change that makes plan gist
instrumentation more scalable.

Release note (bug fix): The Explain Tab inside the Statement Details
page now groups plans that have the same shape but a different number of
spans in corresponding scans.


87154: roachtest: stop cockroach gracefully when upgrading nodes r=yuzefovich a=yuzefovich

This commit makes it so that we stop cockroach nodes gracefully when
upgrading them. Previous abrupt behavior of stopping the nodes during
the upgrade could lead to test flakes because the nodes were not
being properly drained.

Here is one scenario for how one of the flakes (`pq: version mismatch in
flow request: 65; this node accepts 69 through 69`, which means that
a gateway running an older version asks another node running a newer
version to do DistSQL computation, but the versions are not DistSQL
compatible):
- we are in a state when node 1 is running a newer version when node
2 is running an older version. Importantly, node 1 was upgraded
"abruptly" meaning that it wasn't properly drained; in particular, it
didn't send DistSQL draining notification through gossip.
- newer node has already been started but its DistSQL server hasn't been
started yet (although it already can accept incoming RPCs - see comments
on `distsql.ServerImpl.Start` for more details). This means that newer
node has **not** sent through gossip an update about its DistSQL version.
- node 2 acts as the gateway for a query that reads some data that node
1 is the leaseholder for. During the physical planning, older node
2 checks whether newer node 1 is "healthy and compatible", and node 1 is
deemed both healthy (because it can accept incoming RPCs) and is
compatible (because node 2 hasn't received updated DistSQL version of
node 1 since it hasn't been sent yet). As a result, node 2 plans a read
on node 1.
- when node 1 receives that request, it errors out with "version
mismatch" error.

This whole problem is solved if we stop nodes gracefully when upgrading
them. In particular, this will mean that node 1 would first dissipate its
draining notification across the cluster, so during the physical
planning it will only be considered IFF node 1 has already communicated
its updated DistSQL version, and then it would be deemed
DistSQL-incompatible.

I verified that this scenario is possible (with manual adjustments of the
version upgrade test and cockroach binary to insert a delay) and that
it's fixed by this commit. I believe it is likely that other flake types
have the same root cause, but I haven't verified it.

Fixes: #87104.

Release justification: test-only change.

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig craig bot closed this as completed in 5db559d Aug 31, 2022
mgartner added a commit to mgartner/cockroach that referenced this issue Aug 31, 2022
In plan gists, we no longer encode the exact number of spans for scans
so that two queries with the same plan but a different number of spans
have the same gist.

In addition, plan gists are now decoded with the `OnlyShape` flag which
prints any non-zero number of spans as "1+ spans" and removes attributes
like "missing stats" from scans.

Fixes cockroachdb#87138

Release justification: This is a minor change that makes plan gist
instrumentation more scalable.

Release note (bug fix): The Explain Tab inside the Statement Details
page now groups plans that have the same shape but a different number of
spans in corresponding scans.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants