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: stats being different leads to different plans when using a placeholder #99389

Closed
cucaroach opened this issue Mar 23, 2023 · 6 comments · Fixed by #99433
Closed

sql: stats being different leads to different plans when using a placeholder #99389

cucaroach opened this issue Mar 23, 2023 · 6 comments · Fixed by #99433
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-sql-queries SQL Queries Team

Comments

@cucaroach
Copy link
Contributor

cucaroach commented Mar 23, 2023

This originated from https://github.com/cockroachlabs/support/issues/2179. Basically we a get a RowCount of 1 w/o placeholders and a RowCount of 1/3 the columns distinct count w/ a placeholder. Here's a really straightforward distilled repro courtesy @DrewKimball :

exec-ddl
CREATE TABLE xy (x INT, y INT, INDEX (x) STORING (y));
----
exec-ddl
ALTER TABLE xy INJECT STATISTICS '[
  {
    "columns": ["x"],
    "created_at": "2018-01-01 1:00:00.00000+00:00",
    "row_count": 100000,
    "distinct_count": 100000
  },
  {
    "columns": ["y"],
    "created_at": "2018-01-01 1:00:00.00000+00:00",
    "row_count": 100000,
    "distinct_count": 2
  }
]';
----
opt format=show-all
WITH RECURSIVE cte(a,b) AS (
  (SELECT * FROM xy WHERE x = 1)
  UNION ALL
  (SELECT * FROM cte WHERE False)
)
SELECT * FROM cte;
----

If you replace 1 with $1 the problem becomes apparent. Doesn't matter if you use this simple approach or assign-placeholders-opt query-args=(1) format=show-all.

I've narrowed it down to (sb *statisticsBuilder) applyConstraintSet but haven't completed the investigation. In the customer issue the result is a hash join vs a lookup join which is believed to be causing poor performance.

Jira issue: CRDB-25906

@cucaroach cucaroach added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs labels Mar 23, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Mar 23, 2023
@DrewKimball
Copy link
Collaborator

I don't think the statistics builder is necessarily doing anything wrong itself, but we're not updating the properties that are used by the statistics builder after placeholder assignment happens. The property calculation takes place here when the original query plan is built, and we don't update it during the re-optimization that happens after placeholder assignment.

@cucaroach
Copy link
Contributor Author

But it doesn't make sense to me that if the distinct_count and row_count are both N we have a selectivity of 1 on a equality filter but if we know the value the selectivity drops to .0001. We shouldn't need a particular value in order to calculate that the selectivity is much smaller than 1 right? Or am I missing something?

cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 24, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
a normalization step to do this so it happens on the assignplaceholder
path.

Fixes: cockroachdb#99389
Epic: none
Release note: none
@cucaroach cucaroach self-assigned this Mar 24, 2023
@cucaroach cucaroach added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-22.2.x and removed backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Mar 24, 2023
@DrewKimball
Copy link
Collaborator

But it doesn't make sense to me that if the distinct_count and row_count are both N we have a selectivity of 1 on a equality filter but if we know the value the selectivity drops to .0001. We shouldn't need a particular value in order to calculate that the selectivity is much smaller than 1 right? Or am I missing something?

You make a good point that we should be able to see that that filter should produce at most one row, but for now I think we don't estimate for a filter with a placeholder at all, and instead use the "unknown" 1/3 selectivity.

@cucaroach
Copy link
Contributor Author

You make a good point that we should be able to see that that filter should produce at most one row, but for now I think we don't estimate for a filter with a placeholder at all, and instead use the "unknown" 1/3 selectivity.

Yeah I'm just not sure it matters, like do we need good estimates for filters with a placeholder if assign placeholders process will revisit everything? I guess the real question is whether there are any other bits of work we do at build time that should be redone at assign-placeholders time that isn't?

cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 27, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
a normalization step to do this so it happens on the assignplaceholder
path.

Fixes: cockroachdb#99389
Epic: none
Release note: none
@mgartner
Copy link
Collaborator

The output of the tests in the description:

exec-ddl
CREATE TABLE xy (x INT, y INT, INDEX (x) STORING (y));
----

opt format=show-all
WITH RECURSIVE cte(a,b) AS (
  (SELECT * FROM xy WHERE x = 1)
  UNION ALL
  (SELECT * FROM cte WHERE False)
)
SELECT * FROM cte;
----
project
 ├── columns: a:10(int) b:11(int)
 ├── stats: [rows=10]
 ├── cost: 28.8900001
 ├── prune: (10,11)
 ├── recursive-c-t-e
 │    ├── columns: a:6(int) b:7(int)
 │    ├── working table binding: &1
 │    ├── initial columns: t.public.xy.x:1(int) t.public.xy.y:2(int)
 │    ├── recursive columns: a:8(int) b:9(int)
 │    ├── stats: [rows=10]
 │    ├── cost: 28.5700001
 │    ├── fake-rel
 │    │    ├── columns: a:6(int) b:7(int)
 │    │    ├── cardinality: [1 - ]
 │    │    ├── stats: [rows=10]
 │    │    └── cost: 0.02
 │    ├── scan t.public.xy@xy_x_idx
 │    │    ├── columns: t.public.xy.x:1(int!null) t.public.xy.y:2(int)
 │    │    ├── constraint: /1/3: [/1 - /1]
 │    │    ├── stats: [rows=10, distinct(1)=1, null(1)=0]
 │    │    ├── cost: 28.5200001
 │    │    ├── fd: ()-->(1)
 │    │    └── prune: (2)
 │    └── values
 │         ├── columns: a:8(int!null) b:9(int!null)
 │         ├── cardinality: [0 - 0]
 │         ├── stats: [rows=0]
 │         ├── cost: 0.01
 │         ├── key: ()
 │         ├── fd: ()-->(8,9)
 │         └── prune: (8,9)
 └── projections
      ├── variable: a:6 [as=a:10, type=int, outer=(6)]
      └── variable: b:7 [as=b:11, type=int, outer=(7)]

assign-placeholders-opt query-args=(1) format=show-all
WITH RECURSIVE cte(a,b) AS (
  (SELECT * FROM xy WHERE x = $1)
  UNION ALL
  (SELECT * FROM cte WHERE False)
)
SELECT * FROM cte;
----
project
 ├── columns: a:10(int) b:11(int)
 ├── stats: [rows=10]
 ├── cost: 28.8900001
 ├── prune: (10,11)
 ├── recursive-c-t-e
 │    ├── columns: a:6(int) b:7(int)
 │    ├── working table binding: &1
 │    ├── initial columns: t.public.xy.x:1(int) t.public.xy.y:2(int)
 │    ├── recursive columns: a:8(int) b:9(int)
 │    ├── stats: [rows=10]
 │    ├── cost: 28.5700001
 │    ├── fake-rel
 │    │    ├── columns: a:6(int) b:7(int)
 │    │    ├── cardinality: [1 - ]
 │    │    ├── stats: [rows=330]
 │    │    └── cost: 0.02
 │    ├── scan t.public.xy@xy_x_idx
 │    │    ├── columns: t.public.xy.x:1(int!null) t.public.xy.y:2(int)
 │    │    ├── constraint: /1/3: [/1 - /1]
 │    │    ├── stats: [rows=10, distinct(1)=1, null(1)=0]
 │    │    ├── cost: 28.5200001
 │    │    ├── fd: ()-->(1)
 │    │    └── prune: (2)
 │    └── values
 │         ├── columns: a:8(int!null) b:9(int!null)
 │         ├── cardinality: [0 - 0]
 │         ├── stats: [rows=0]
 │         ├── cost: 0.01
 │         ├── key: ()
 │         ├── fd: ()-->(8,9)
 │         └── prune: (8,9)
 └── projections
      ├── variable: a:6 [as=a:10, type=int, outer=(6)]
      └── variable: b:7 [as=b:11, type=int, outer=(7)]

@cucaroach in the real-world case where you saw this problem, was the bad row count affecting the query plan that the optimizer chose?

cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 27, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
a normalization step to do this so it happens on the assignplaceholder
path.

Fixes: cockroachdb#99389
Epic: none
Release note: none
@cucaroach
Copy link
Contributor Author

@cucaroach in the real-world case where you saw this problem, was the bad row count affecting the query plan that the optimizer chose?

Yes it was causing us to use a hash join instead of a lookup join.

cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 27, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
a normalization step to do this so it happens on the assignplaceholder
path.

Fixes: cockroachdb#99389
Epic: none
Release note: none
cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 27, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
a normalization step to do this so it happens on the assignplaceholder
path.

Fixes: cockroachdb#99389
Epic: none
Release note: none
cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 27, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: cockroachdb#99389
Epic: none
Release note: none
craig bot pushed a commit that referenced this issue Mar 28, 2023
…99752 #99774

99433: opt: fixup CTE stats on placeholder queries r=cucaroach a=cucaroach

During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: #99389
Epic: none
Release note: none

99516: metrics: improve ux around _status/vars output r=aadityasondhi a=dhartunian

Previously, the addition of the `tenant` metric label was applied uniformly and could result in confusion for customers who never enable multi-tenancy or c2c. The `tenant="system"` label carries little meaning when there's no tenancy in use.

This change modifies the system tenant label application to only happen when a non- sytem in-process tenant is created.

Additionally, an environment variable:
`COCKROACH_DISABLE_NODE_AND_TENANT_METRIC_LABELS` can be set to `false` to disable the new `tenant` and `node_id` labels. This can be used on single-process tenants to disable the `tenant` label.

Resolves: #94668

Epic: CRDB-18798

Release note (ops change): The
`COCKROACH_DISABLE_NODE_AND_TENANT_METRIC_LABELS` env var can be used to disable the newly introduced metric labels in the `_status/vars` output if they conflict with a customer's scrape configuration.

99522: jobsprofiler: store DistSQL diagram of jobs in job info r=dt a=adityamaru

This change teaches import, cdc, backup and restore
to store their DistSQL plans in the job_info table
under a timestamped info key. The generation and writing
of the plan diagram is done asynchronously so as to not
slow down the execution of the job. A new plan will be
stored everytime the job sets up its DistSQL flow.

Release note: None
Epic: [CRDB-8964](https://cockroachlabs.atlassian.net/browse/CRDB-8964)
Informs: #99729

99574: streamingccl: skip acceptance/c2c on remote cluster setup r=stevendanna a=msbutler

acceptance/c2c currently fails when run on a remote cluster. This patch ensures the test gets skipped when run on a remote cluster. There's no need to run the test on a remote cluster because the other c2c roachtests provide sufficient coverage.

Fixes #99553

Release note: none

99691: codeowners: update sql obs to cluster obs r=maryliag a=maryliag

Update mentions of `sql-observability` to
`cluster-observability`.

Epic: none
Release note: None

99712: ui: connect metrics provider to metrics timescale object r=xinhaoz a=dhartunian

Previously, the `MetricsDataProvider` component queried the redux store for the `TimeScale` object which contained details of the currently active time window. This piece of state was assumed to update to account for the "live" moving window that metrics show when pre-set lookback time windows are selected.

A recent PR: #98331 removed the feature that polled new data from SQL pages, which also disabled polling on metrics pages due to the re-use of `TimeScale`.

This commit modifies the `MetricsDataProvider` to instead read the `metricsTime` field of the `TimeScaleState` object. This object was constructed for use by the `MetricsDataProvider` but was not wired up to the component.

Resolves #99524

Epic: None

Release note: None

99733: telemetry: add FIPS-specific channel r=knz a=rail

Previously, all official builds were reporting using the same telemetry channel.

This PR adds an new telemetry channel for the FIPS build target.

Fixes: CC-24110
Epic: DEVINF-478
Release note: None

99745: spanconfigsqlwatcher: deflake TestSQLWatcherOnEventError r=arulajmani a=arulajmani

Previously, this test was setting the no-op checkpoint duration to be
 every hour to effectively disable checkpoints. Doing so is integral to
what the test is testing. However, this was a lie, given how `util.Every` works -- A call to `ShouldProcess` returns true the very first time.

This patch achieves the original goal by introducing a new testing knob. Previously, the test would fail in < 40 runs locally.  Have this running strong for ~1000 runs.

Fixes #76765

Release note: None

99747: roachtest: use persistent disks for disk-stall tests r=jbowens a=nicktrav

Currently, the `disk-stall` tests use local SSDs. When run on GCE VMs, a higher test flake rate is observed due to known issues with fsync latency for local SSDs.

Switch the test to use persistent disks instead.

Touches: #99372.

Release note: None.

Epic: CRDB-20293

99752: kvserver: bump tolerance more r=ajwerner a=ajwerner

I'm not digging into this more, but the test is flakey.

Epic: none

https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests_BazelUnitTests/9161972?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

Release note: None

99774: *: identify remaining uses of TODOSQLCodec r=stevendanna a=knz

The `TODOSQLCodec` was a bug waiting to happen.

The only reasonable remaining purpose is for use in tests. As such, this change moves its definition to a test-only package (we have a linter that verifies that `testutils` is not included in non-test code).

This change then identifies the one non-reasonable remaining purposes and identifies it properly as a bug linked to #48123.

Release note: None
Epic: None

Co-authored-by: Tommy Reilly <treilly@cockroachlabs.com>
Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: adityamaru <adityamaru@gmail.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: maryliag <marylia@cockroachlabs.com>
Co-authored-by: Rail Aliiev <rail@iqchoice.com>
Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
Co-authored-by: Nick Travers <travers@cockroachlabs.com>
Co-authored-by: ajwerner <awerner32@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig craig bot closed this as completed in 2014cf2 Mar 28, 2023
blathers-crl bot pushed a commit that referenced this issue Mar 28, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: #99389
Epic: none
Release note (sql change): Prepared statements using placeholders in
recursive CTEs sometimes did not re-optimize correctly after plugging
in the parameters leading to poor plan choices, this has been fixed.
cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 31, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: cockroachdb#99389
Epic: none
Release note (sql change): Prepared statements using placeholders in
recursive CTEs sometimes did not re-optimize correctly after plugging
in the parameters leading to poor plan choices, this has been fixed.
cucaroach added a commit to cucaroach/cockroach that referenced this issue Mar 31, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: cockroachdb#99389
Epic: none
Release note (sql change): Prepared statements using placeholders in
recursive CTEs sometimes did not re-optimize correctly after plugging
in the parameters leading to poor plan choices, this has been fixed.
cucaroach added a commit to cucaroach/cockroach that referenced this issue Apr 3, 2023
During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: cockroachdb#99389
Epic: none
Release note (sql change): Prepared statements using placeholders in
recursive CTEs sometimes did not re-optimize correctly after plugging
in the parameters leading to poor plan choices, this has been fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants