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

consolidate guidance on avoiding full scans; update session vars #18338

Merged
merged 4 commits into from
Mar 29, 2024

Conversation

taroface
Copy link
Contributor

@taroface taroface commented Feb 29, 2024

DOC-9629

  • Added a section to SQL Performance Best Practices that summarizes how to prevent the optimizer from planning full scans. Preview
  • This also consolidates existing guidance across the docs into several includes.
  • Updated descriptions of the disallow_full_table_scans and large_full_scan_rows settings (for the past 3 docs versions) as it looked like the behavior of these settings was updated in sql, opt: support hint to disallow full scan, update coster to avoid full scans cockroach#71317 but this wasn't documented.

@taroface taroface requested a review from rytaft February 29, 2024 23:15
Copy link

github-actions bot commented Feb 29, 2024

Files changed:

Copy link

netlify bot commented Feb 29, 2024

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit b4eaf59
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/66071aff8c0e9500089da17e

Copy link

netlify bot commented Feb 29, 2024

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit b4eaf59
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/66071affb172b50008f4df9c

Copy link

netlify bot commented Feb 29, 2024

Netlify Preview

Name Link
🔨 Latest commit b4eaf59
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/66071aff04a490000936fa7d
😎 Deploy Preview https://deploy-preview-18338--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! Sorry for the delay.

There was another support issue where the customer was tuning their queries and was concerned that the EXPLAIN output showed some full table scans. However, this was expected since the customer was testing with empty tables, and therefore the stats did not represent the production environment.

Do you think it makes sense to update this section of the docs to mention that most of the time the optimizer makes the right decision, and if you see a full scan you should (a) make sure that it's actually causing a performance problem, and (b) confirm that the optimizer would actually choose a full scan in the production environment (with correct stats, cluster topology, etc)? Only then should a customer try to use one of these tools to override the optimizer's decision.

If you think that should be a different PR, I'm happy to file a separate docs issue. Let me know what you think. Thanks!

src/current/v23.2/transactions.md Outdated Show resolved Hide resolved
src/current/_includes/v23.2/sql/transactions-limit-rows.md Outdated Show resolved Hide resolved
@taroface
Copy link
Contributor Author

This looks great, thanks! Sorry for the delay.

There was another support issue where the customer was tuning their queries and was concerned that the EXPLAIN output showed some full table scans. However, this was expected since the customer was testing with empty tables, and therefore the stats did not represent the production environment.

Do you think it makes sense to update this section of the docs to mention that most of the time the optimizer makes the right decision, and if you see a full scan you should (a) make sure that it's actually causing a performance problem, and (b) confirm that the optimizer would actually choose a full scan in the production environment (with correct stats, cluster topology, etc)? Only then should a customer try to use one of these tools to override the optimizer's decision.

If you think that should be a different PR, I'm happy to file a separate docs issue. Let me know what you think. Thanks!

TFTR @rytaft! I had a follow-up question (above).

I do think that a separate docs issue with some more details would make sense for the other recommendations you describe.

@rytaft
Copy link
Contributor

rytaft commented Mar 28, 2024

This looks great, thanks! Sorry for the delay.
There was another support issue where the customer was tuning their queries and was concerned that the EXPLAIN output showed some full table scans. However, this was expected since the customer was testing with empty tables, and therefore the stats did not represent the production environment.
Do you think it makes sense to update this section of the docs to mention that most of the time the optimizer makes the right decision, and if you see a full scan you should (a) make sure that it's actually causing a performance problem, and (b) confirm that the optimizer would actually choose a full scan in the production environment (with correct stats, cluster topology, etc)? Only then should a customer try to use one of these tools to override the optimizer's decision.
If you think that should be a different PR, I'm happy to file a separate docs issue. Let me know what you think. Thanks!

TFTR @rytaft! I had a follow-up question (above).

I do think that a separate docs issue with some more details would make sense for the other recommendations you describe.

Done: #18428

Copy link
Contributor

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you! And sorry for the delays!

Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good refactor. I'll go ahead and approve because I don't feel super strongly about any of my feedback.

src/current/_includes/v23.2/misc/force-index-selection.md Outdated Show resolved Hide resolved
src/current/v23.2/transactions.md Show resolved Hide resolved
@taroface taroface force-pushed the full-scan-guardrails branch 2 times, most recently from 0f27f7e to 16e9a05 Compare March 29, 2024 19:37
@taroface taroface merged commit 6ee9ceb into main Mar 29, 2024
6 checks passed
@taroface taroface deleted the full-scan-guardrails branch March 29, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants