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

docs: RFC on resource usage boundaries. #7657

Closed
wants to merge 1 commit into from

Conversation

knz
Copy link
Contributor

@knz knz commented Jul 6, 2016

(Work done on the way to #3292, because #7572 hits all over the place)


This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Jul 6, 2016

cc @dt @petermattis

@maddyblue
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


docs/RFCS/resource_usage.md, line 146 [r1] (raw file):

  - the set of aggregate functions being computed in groupNode;
  - the set of in-memory right-side rows in joinNode;
  - the copy of the result set held in `Executor.execStmt()` until

From my read of the protocol docs I think we can eliminate this as a possibility. Since all our sql nodes support the Next() call (right?), and the pgwire stuff sends each row as a separate thing but can also send an error at any time. Thus, we can return the sql node to the pgwire package instead of the enumerated results. The only problem I can think of is about retryable operations. We currently have code that retries things when it can. If we do this, we can no longer do that and have to pass that choice up to the user because we've already returned rows to them.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


docs/RFCS/resource_usage.md, line 10 [r1] (raw file):

# Summary

We need to bound the resources (time, memory) used by SQL

There are other resources: client connections, cpu, network. I don't think these need to be addressed here, but you might want to mention them as areas for future work. In addition to strict bounds (which this RFC is targeting), we may eventually want to have throttling (i.e. for cpu and network). This is definitely outside the scope of this RFC.


docs/RFCS/resource_usage.md, line 253 [r1] (raw file):

- take the total amount of physical memory available in the system M,
  excluding swap.

I'm anxious about using the physical memory for this. Note that the vast majority of memory used by cockroach is the RocksDB block cache which doesn't show up in the Go runtime stats. (We do know what that memory usage number is, though).

We want to use the available memory, but I think we want this to be explicitly limited. I'd prefer taking some fraction of the --cache-size to reserve for query execution.


docs/RFCS/resource_usage.md, line 291 [r1] (raw file):

not sure

Allow swap or not? Proposal is to compute M without swap. Swap yields

Not (IMO). Performance is horrible when you start swapping. Better to figure out how to explicitly limit memory and switch to disk-based algorithms. For example, it is much better to use a disk sort than to assume infinite memory and use a memory sort + swap. On the other hand, we could use a cache oblivious sort...


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Jul 14, 2016

PTAL to section "Alternatives" specifically.

cc @RaduBerinde @andreimatei @dt.

@knz knz force-pushed the resource-usage-rfc branch from 895c769 to 85028af Compare September 12, 2016 14:27
@knz knz closed this Jun 26, 2017
@knz knz deleted the resource-usage-rfc branch June 26, 2017 18:16
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