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: turn plan sampling back off by default #89847

Merged
merged 1 commit into from
Oct 13, 2022
Merged

sql: turn plan sampling back off by default #89847

merged 1 commit into from
Oct 13, 2022

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Oct 12, 2022

The value was originally disabled by default in: #88343. The value was enabled by default: #89020 because of a bug that was fixed in #89418. This PR is disabling it by default again now that the bug has been fixed.

Part Of #89847

Release note (sql change): Turn the default value of sql.metrics.statement_details.plan_collection.enabled to false.

@j82w j82w requested a review from a team October 12, 2022 18:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

LGTM. Build failure looks like whitespace differences in docs/generated/settings/settings.html:

diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html
index 8d28ec9f38..6620036f6b 100644
--- a/docs/generated/settings/settings.html
+++ b/docs/generated/settings/settings.html
@@ -187,12 +187,7 @@
 <tr><td><code>sql.metrics.statement_details.gateway_node.enabled</code></td><td>boolean</td><td><code>true</code></td><td>save the gateway node for each statement fingerprint. If false, the value will be stored as 0.</td></tr>
 <tr><td><code>sql.metrics.statement_details.index_recommendation_collection.enabled</code></td><td>boolean</td><td><code>true</code></td><td>generate an index recommendation for each fingerprint ID</td></tr>
 <tr><td><code>sql.metrics.statement_details.max_mem_reported_idx_recommendations</code></td><td>integer</td><td><code>5000</code></td><td>the maximum number of reported index recommendation info stored in memory</td></tr>
-<tr>
-  <td><code>sql.metrics.statement_details.plan_collection.enabled</code></td>
-  <td>boolean</td>
-  <td><code>false</code></td>
-  <td>periodically save a logical plan for each fingerprint</td>
-</tr>
+<tr><td><code>sql.metrics.statement_details.plan_collection.enabled</code></td><td>boolean</td><td><code>false</code></td><td>periodically save a logical plan for each fingerprint</td></tr>

@j82w
Copy link
Contributor Author

j82w commented Oct 13, 2022

Fixed, I didn't realize that GoLand reformatted the line.

This value was enabled by default again: #89020. The reason it was
enabled again was fixed in #89418.

Part of #89847

Release note (sql change): Turn the default value of
sql.metrics.statement_details.plan_collection.enabled to false.
@j82w
Copy link
Contributor Author

j82w commented Oct 13, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 13, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Oct 13, 2022

Build succeeded:

@craig craig bot merged commit 3bdca7d into cockroachdb:master Oct 13, 2022
blathers-crl bot pushed a commit that referenced this pull request Oct 13, 2022
This value was enabled by default again: #89020. The reason it was
enabled again was fixed in #89418.

Part of #89847

Release note (sql change): Turn the default value of
sql.metrics.statement_details.plan_collection.enabled to false.
j82w pushed a commit that referenced this pull request Oct 18, 2022
This value was enabled by default again: #89020. The reason it was
enabled again was fixed in #89418.

Part of #89847

Release justification: Bug fixes and low-risk updates to new functionality.

Release note (sql change): Turn the default value of
sql.metrics.statement_details.plan_collection.enabled to false.
j82w pushed a commit that referenced this pull request Nov 2, 2022
This value was enabled by default again: #89020. The reason it was
enabled again was fixed in #89418.

Part of #89847

Release justification: Bug fixes and low-risk updates to new functionality.

Release note (sql change): Turn the default value of
sql.metrics.statement_details.plan_collection.enabled to false.
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