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

Recommend disallow_full_table_scans session var #18265

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

rmloveland
Copy link
Contributor

Fixes DOC-7875

Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit b73b402
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/65cf80d36cdc6e0008330551

Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit b73b402
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/65cf80d3c1635e00086d79ba

Copy link

netlify bot commented Jan 30, 2024

Netlify Preview

Name Link
🔨 Latest commit b73b402
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/65cf80d3d3031c0007b81008
😎 Deploy Preview https://deploy-preview-18265--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.

@rafiss
Copy link
Contributor

rafiss commented Jan 30, 2024

@dikshant do you have thoughts on whether we should recommend passing it in the connection string, versus recommending to configure the default value with ALTER ROLE ALL SET disallow_full_table_scans = true?

@rmloveland rmloveland force-pushed the 20240130-DOC-7875-table-scan-best-practices branch from b41aa7c to 130b82c Compare February 13, 2024 16:08
@rmloveland
Copy link
Contributor Author

@dikshant do you have thoughts on whether we should recommend passing it in the connection string, versus recommending to configure the default value with ALTER ROLE ALL SET disallow_full_table_scans = true?

Thanks for looking @rafiss - I have pushed an update that makes this into a list of options instead:

  1. per-cluster setting for all roles using ALTER ROLE as you mentioned
  2. per-application setting on the connection string (less preferred since it's the number 2 option)

PTAL!

@rmloveland rmloveland force-pushed the 20240130-DOC-7875-table-scan-best-practices branch from 130b82c to 6a49bb9 Compare February 13, 2024 16:11
@rmloveland
Copy link
Contributor Author

@rafiss and/or @dikshant - any objection to these latest changes? sorry to ping, I know I was away from it a bit myself

has a couple linked support issues from DOC-7875 so wanted to get it in

@dikshant
Copy link

LGTM! Thanks for adding this!

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

i like describing both options. lgtm!

@dikshant
Copy link

Ah I missed this:

@dikshant do you have thoughts on whether we should recommend passing it in the connection string, versus recommending to configure the default value with ALTER ROLE ALL SET disallow_full_table_scans = true?

Yes let's have both and let customers pick whichever they prefer.

@rmloveland rmloveland enabled auto-merge (squash) February 16, 2024 15:35
@rmloveland rmloveland merged commit a733c1d into main Feb 16, 2024
6 checks passed
@rmloveland rmloveland deleted the 20240130-DOC-7875-table-scan-best-practices branch February 16, 2024 15:40
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.

4 participants