-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
cli: add --max-sql-memory flag to cockroach mt start-sql
#71011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that SQL disk spilling uses pebble, we'll want to enable --cache
as well.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @darinpp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
Given that SQL disk spilling uses pebble, we'll want to enable --cache as well.
Correct me if I'm wrong, but I think that won't work for a few reasons:
-
The cache size used for DistSQL's temp engine is hardcoded to 128MB:
cockroach/pkg/storage/temp_engine.go
Line 72 in 8badea4
CacheSize(128<<20),
--cache
is only used by server.CreateEngines, which isn't used when starting the tenant at all. Looking at old PRs, it looks like the cache size doesn't matter for the temp engine:cockroach/pkg/storage/engine/temp_engine.go
Lines 105 to 107 in 161c2f3
// Pebble doesn't currently support 0-size caches, so use a 128MB cache for // now. Cache: cache.New(128 << 20), -
SQL disk spilling currently does not allow the option of using pebble since we hardcoded the temp storage to use in-memory:
cockroach/pkg/cli/mt_start_sql.go
Lines 104 to 110 in 8badea4
serverCfg.SQLConfig.TempStorageConfig = base.TempStorageConfigFromEnv( ctx, st, base.StoreSpec{InMemory: true}, "", // parentDir tempStorageMaxSizeBytes, )
If we want to enable SQL disk spilling on tenants, we have more work to do, and that should be another PR.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @darinpp)
All right, thanks for explaining. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @darinpp)
Previously the `--max-sql-memory` flag wasn't available to the multi-tenancy start-sql command, even thought the feature was already there for other `start`-related commands. Release note (cli change): `cockroach mt start-sql` will now support the `--max-sql-memory` flag to configure maximum SQL memory capacity to store temporary data. Release justification: The upcoming Serverless MVP release plans to use a different value for --max-sql-memory instead of the default value of 25% of container memory. This commit is only a flag change that will only be used in multi-tenant scenarios, and should have no impact on dedicated customers.
1589d4c
to
e93430e
Compare
TFTR! bors r=knz |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from e93430e to blathers/backport-release-21.2-71011: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Previously the
--max-sql-memory
flag wasn't available to the multi-tenancystart-sql command, even though the feature was already there for other
start
-related commands.Release note (cli change):
cockroach mt start-sql
will now support the--max-sql-memory
flag to configure maximum SQL memory capacity to storetemporary data.
Release justification: The upcoming Serverless MVP release plans to use a
different value for
--max-sql-memory
instead of the default value of 25%of container memory. This commit is only a flag change that will only be used
in multi-tenant scenarios, and should have no impact on dedicated customers.