Introduce a per-query memory limit

[coffeemug]

This came out of the team discussion. We should introduce a per-query-per-node memory limit configurable in run in MBs. If the query goes over the limit, it should be interrupted with an error.

[coffeemug]

This is somewhat related to #971.

[pixelspark]

A time limit would also be nice (see #1344)

[coffeemug]

@pixelspark -- cool idea. I think we already have a time limit, but it's hardcoded. It should be relatively easy to expose it. ("I think" is a key phrase here :))

[timmaxw]

Copying @mlucy's comment:

I think we should just bite the bullet and do this. (We need it, or something similar but simpler, for #2721.)

[timmaxw]

I think this is a good idea, but I think we should think carefully about the implementation or else it will end up being unmaintainable. This proposal would require making each datum_t be "owned" by a specific query. This is a big change; I suspect it's bigger than the configured_limits_t changes. We have to deal with all the same awkward cases, plus some new ones. For example, when we deserialize a datum through a mailbox_t, we want to apply the cost of that datum to the per-query memory limit for the query that initiated the request. It has the potential to get complicated quickly.

Here are some thoughts:

1. We should look at how other projects have implemented memory limits, and see if there are good ideas that we can copy.
2. We should consider making the memory limit system generic instead of tying it to the concept of a ReQL queries. For example, we arguably want per-backfill memory limits. The other advantage of making the memory limit system generic is that we might want to thread it through weird places (such as mailboxes), and threading a ReQL-specific memory limit system through mailboxes is way uglier than threading a generic memory limit system through mailboxes.
3. We should see if we can get away with implementing a datum_t size limit first. I'm pretty sure we'll want a per-query memory limit eventually, but implementing a datum_t size limit might buy us some time before it becomes critical, and we can use that time to do a better job.

I also have some vague ideas for how a per-query memory limit system could work. I suggest that we create a base class for objects subject to the memory limit; this way, we can apply the memory limit gradually to one data type at a time in a controlled fashion. Subclasses of this base class would be constructed with a "memory account" object that they would use for allocations, and they would remember this object. We would overload "placement operator new" to use the special allocator. It would be possible to transfer ownership of a memory-managed object between two different accounts, or to construct it with no owner. (For example, mailboxes might construct objects received over with no owner, but then the query would take ownership once it received the object.) We would also provide a std allocator implementation that allocates from a memory account.

Obviously this deserves a lot more thought than this brief paragraph, but these are just some ideas to get the juices flowing. Also, we shouldn't forget to check how other projects do it and see if there are existing best practices.

[mlucy]

I would be OK with just having a per-{object/array} cumulative size limit and doing a generic memory-limiting system later. I think it depends on how hard it would be to limit memory on a per-query basis.

(To be clear, when I say "cumulative size limit" I mean "An array of ten objects each with 5 fields counts as 50.")

@gchpaco -- any opinion?

[wojons]

@coffeemug @pixelspark

I think @danielmewes or @neumino told me about timeout's i have not used them yet but i thought there was a limit in the driver for that.

[gchpaco]

The best way to do this is probably to attach a custom pool allocator onto env_ts and use it, which is another one of those 'propogate this through every part of the codebase, plz' tasks.

[mlucy]

After thinking about this more, I think the per-object/per-array cumulative size limit would be pretty easy to do and would be good enough. I think we should do that. (It will also avoid the propagation problems @gchpaco refers to.)

[neumino]

If we introduce such limit, are we adding an option in the query language to increase it? It would be a poor experience to be stuck during a one time analytic operation because of this limit.

Also is there a reason we ship this in 1.16? It seems to be purely for safety -- and no one really ran into that right? I would rather fix things that people run into like backtraces, compound indexes, and strings operations for example.

[mlucy]

It would be configurable the same way the array size limit is now.

I want to do this sooner rather than later because I think we need this for https://github.com/rethinkdb/rethinkdb/issues/2721. (If we don't have this, then it's way too easy for someone with large rows in a table to accidentally try to load the whole table into memory. For instance, if someone had 1-MB rows and wrote r([r.table('test'), r.table('test2')]), the array size limit wouldn't kick in until we'd loaded 100GB into memory.)

[gchpaco]

In theory, this could be done with the preexisting configurable limits spec, and by adding an allocator to the env parameter. That allocator would then need to be threaded into every allocation made; this probably means at least feeding it to the datum builders, which isn't that big a problem.

It would be highly obnoxious and difficult to try to force the total allocated memory of the query across all shards to fit some threshold; much easier to enforce it on a per shard basis. I can look into this if you want me to.

[neumino]

Are we removing the array limit if we implement a per query memory limit?

[mlucy]

There are three different things here:

• The array size limit.
• A per-query memory limit.
• A cumulative array/object size limit.

We currently only have the array size limit. I think we need either an array size limit or a cumulative array/object size limit (Slava thinks that if we only had a per-query memory limit it would feel developer-unfriendly). I think we need either a per-query memory limit or a cumulative array/object size limit to do #2721, which is important. Long-term, I think we need a per-query memory limit for devops reasons.

So, I think short-term we should implement a cumulative array/object size limit so that we can do #2721, and long-term we should add a per-query memory limit in addition to the cumulative array/object size limit. I'm agnostic on whether we should get rid of the existing array size limit when we implement the cumulative array/object size limit.

[gchpaco]

@timmaxw You seem to be trending toward the same way I am, but I don't especially understand why we need to have special base classes etc. at all. We could use the same allocator design the std lib does; http://en.cppreference.com/w/cpp/memory/allocator these things. Our STL containers are supposed to already support it, and if we hack make_counted to use the allocator rather than new we should be able to slot it in. Now threading those allocators through everywhere they need to go is a huge deal, I will not deny that. I think it should be sufficient to attach an allocator to the env object we already thread through everything, and we can follow the lead of the array limits changes to figure out where that threading is no longer sufficient. This also may give us the ability to use pool based memory allocation eventually, which could be a big win (we currently spend a lot of time incrementing and decrementing reference counts on short lived garbage). We don't need to write our own—Boost provides one—but our previous experience with Boost has not been universally positive, to be certain.

I should note that I loathe the way allocator is designed, but I can't put my finger on concrete reasons and it's integrated into the rest of the C++ std lib so it's probably not worth fighting over.

One detriment is due to the type system, the types of these things will change slightly; wikipedia notes "Like all C++ class templates, instantiations of standard library containers with different allocator arguments are distinct types. A function expecting an std::vector argument will therefore only accept a vector instantiated with the default allocator."

[gchpaco]

After speaking with @mlucy about this, he asked me to put down my thoughts here, which is as follows:

• I think that array/datum size limits and the memory allocation limits are solving in some sense the same problem--please do not detonate my database because I accidentally wrote a dumb query.
• With my developer hat on array size limits are very understandable and datum limits are a bit weird. Particularly given that the way we perform operations in the database make a lot of junk datums that will be cleaned up almost immediately, it is possible to violate the datum limit by accident without knowing why.
• A limit on a number of datums doesn't really help us if someone accidentally does a file read from /dev/zero or an HTTP GET on something stupidly enormous.
• Memory limits are a bit opaque but at least it's a comprehensible situation: "query exceeded allocated memory at point X" should be enough to start debugging.
• With my sysadmin hat on array size limits and datum limits don't solve any of the problems I want, which is that I have a server with X GB of memory that I know needs to be able to serve such and such query requests per second.
• Even if our memory limits are imperfect (for example, we may not be able to easily track mailbox allocations) they are still useful; after all, some shard had to build that thing that we are reading.
• Accordingly I think that if we add query limits, even if they are not perfectly precise, we don't really need array limits or datum limits.

[mlucy]

@gchpaco -- how long do you think this will take? If it's more than a few days (or turns into more than a few days), I think we should do cumulative datum size limits for now because there's lots of other important stuff to work on.

[gchpaco]

Minimum a week.

[mlucy]

@coffeemug, how important is a memory size limit as opposed to a cumulative datum size limit?

[coffeemug]

Here are my thoughts:

• We need to solve the problem (more on the specific solution below) sooner rather than later. I talked to a user on Tue who wrote an analytics-style query, ran it on a large dataset, and had RethinkDB crash. The problem was the OOM killer because he was on a small-RAM VM, but he perceived it as RethinkDB crashing. This is a real problem that needs to get solved. Note that a datum limit wouldn't solve this specific problem (it was a non-indexed join that returned a selection; no huge datums involved).
• I agree with @gchpaco that array limits make sense, per-query memory limits make sense, but per-datum memory limit are weird and hard to understand. I also agree that a partial solution might get us 90% of the way there, and we may not necessarily need to thread things through the mailboxes and such.
• I also think that we should have an array limit and a per-query memory limit. Both make sense in different contexts. I don't think we should have a datum size limit if at all possible.

[neumino]

What I expect to happen if someone runs a query without index is:

• User runs r.table("foo").orderBy("id").run() -- hits the array limit
• User runs r.table("foo").orderBy("id").run({arrayLimit: Infinity}) -- hit the memory limit
• User runs r.table("foo").orderBy("id").run({arrayLimit: Infinity, memoryLimit: Infinity}) -- OOM killed

The limits don't solve anything, and just prevent them from shooting themselves in the foot.

To me, this is still a not low hanging fruit that isn't tasty and doesn't fill much. Having the drivers automatically coerce cursors to arrays would solve more problem that this limit -- at least people won't run things like r.table("foo").coerceTo("ARRAY").

[mlucy]

Note that a datum limit wouldn't solve this specific problem (it was a non-indexed join that returned a selection; no huge datums involved).

I don't think that's true. A non-indexed join creates an array, so a cumulative datum size limit would solve this.

I don't think we should have a datum size limit if at all possible.

Why's that? The array size limit feels sort of arbitrary because you can have a 90,000 element array of integers or a 90,000 element array of nested 90,000 element arrays, and neither trips the limit.

(Note that when we're talking about "datum size limit" in this issue, we mean "number of elements", not "bytes of memory", so [{a: 1, b: 2}, {c: 3}] would have a datum size of 3 because it has one two-element object and one one-element object.)

[mlucy]

The limits don't solve anything, and just prevent them from shooting themselves in the foot.

I think that's the whole problem -- people should be able to accidentally run expensive queries without crashing their server. It isn't reasonable to expect people to be able to exactly predict the memory efficiency of a query.

[mlucy]

Having the drivers automatically coerce cursors to arrays would solve more problem that this limit -- at least people won't run things like r.table("foo").coerceTo("ARRAY").

I don't follow. In fact, people would run out of memory more often because someone running r.table('foo') would then run out of memory on the client.

[coffeemug]

@danielmewes -- I noticed your comment on the mailing list that this is intended to replace the array limit. Is that right? I don't think it should -- I think we should have the memory limit and the array limit, and always stop whenever we hit the most conservative limit.

[danielmewes]

@coffeemug Sorry, I assumed this was the plan, but I saw now that we hadn't actually reached a consensus on the question.

The statements I could find in this thread are that @mlucy was "agnostic" to that question and @gchpaco said "[...] I think that if we add query limits [...] we don't really need array limits or datum limits." Do you think there's a reason to keep the array size limit around? Does it solve a problem that the memory size limit doesn't?

[coffeemug]

Does it solve a problem that the memory size limit doesn't?

That's a good point -- I'm not sure. To me these arguments solve slightly different problems. The array limit lets me design an app in a way where I can have very well-defined and consistent assurances about when things will and will not work. So if I start getting into large array territory, I can temporarily increase the limit and redesign my app.

The memory limit is more of an operational thing. I might set it to different values on different machines for my analytics-type queries, for example, but because I can't easily map in my mind from the memory limit to the possible number of elements in the array, just having it would be very unpredictable for the use case above.

I think we should keep both for now, see what happens in real usecases as people start taking advantage of this functionality, and then decide whether we want to merge the two optargs later.

[danielmewes]

My concern is that once we add the memory limit, the array size limit is going to be difficult to explain to users. If a user asks us "I'm running into an array size limit. Why is RethinkDB limited in this way?" We can currently answer "This is a protection to make sure queries don't kill the server by consuming too much memory. You can increase it by running ... if you are sure your query doesn't actually use that much." Once we have the memory size limit, this answer doesn't work anymore.

The other use case that you mention I think is much more difficult to justify. The basic motivation here is to warn users if they're using queries that should be using streaming and an index instead of generating big intermediate results if I see this correctly (please let me know if there's something else to it that I don't realize). However I think the array size limit is not actually great in doing that. The first problem is that it will often not show up in small-scale tests, but only later when users roll out their application to larger data sets. In this respect it kicks in at a somewhat arbitrary boundary. This is not much unlike what the memory size limit would do. The memory size limit wouldn't actually be better, but I don't think it would be much worse in warning people about these conditions in their queries either. The other problem is that - as we have seen on the mailing list - there isn't always a way to avoid generating the intermediate result. Here the memory size limit does a better job because it better differentiates between cases where the large intermediate result is actually large and those where the only issue is that we think that generally intermediate results with many entries should be avoided.

(An alternative for the second motivation could be @neumino's idea of having warnings in the driver https://github.com/rethinkdb/rethinkdb/issues/3247 , though I think that has some problems too.)

I still think that we should remove the array size limit when we introduce the memory limit. But if you feel strongly about this or more arguments in its favor come up, I would certainly be ok with leaving it in at least for now.

[coffeemug]

I'd like to think about it for a little bit. You bring up good points, I need a day or so to process them.

[mlucy]

The only thing I would say on this is that if we automatically convert streams to arrays, which I think we're still planning to do, then lots and lots of people will start running into situations where they're accidentally trying to load whole tables into memory. Having some way to catch this case and tell people specifically what part of their query is at fault would be great.

An array size limit catches this better than a memory size limit because a memory size limit will be tripped at some random place in the query, whereas the array size limit will be tripped at the point where the table is converted to an array. So the user will get back an error message saying the actual problem (they're trying to generate a huge array) and with the appropriate part of their query underlined.

[danielmewes]

Having some way to catch this case and tell people specifically what part of their query is at fault would be great.

I see, that makes sense. The situation is still not quite satisfactory because there's no guarantee in such cases that the array size limit is going to be tripped first. Consider me undecided.