sql: attempt to account for memory in crdb_internal.jobs #45758
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)
pkg/sql/crdb_internal.go, line 490 at r1 (raw file):
for field, d := range r { // NB: Add the datum size twice to count the fact that we already allocated // it once and we're about to
This comment seems to end before it should
pkg/sql/crdb_internal.go, line 493 at r1 (raw file):
datumSize := int64(d.Size()) if field == payloadFieldN || field == progressFieldN { datumSize *= 2
I don't understand this... why? Also if you insist on doing this, can we just do it after the loop rather than this comparison stuff.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
pkg/sql/crdb_internal.go, line 490 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
This comment seems to end before it should
... unmarshal it and keep that around.?
pkg/sql/crdb_internal.go, line 493 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I don't understand this... why? Also if you insist on doing this, can we just do it after the loop rather than this comparison stuff.
I did this. I did it because we unmarshal these fields and keep both the marshaled and unmarshaled bits alive. If we zero'd the row datums after unmarshaling then I'd feel better about not counting it but that seems like too much effort. I'm cool with just doubling the row count.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go, line 493 at r1 (raw file):
Previously, ajwerner wrote…
I did this. I did it because we unmarshal these fields and keep both the marshaled and unmarshaled bits alive. If we zero'd the row datums after unmarshaling then I'd feel better about not counting it but that seems like too much effort. I'm cool with just doubling the row count.
Oh I see. Yeah I'd just get rid of the "precise" stuff here with the column integer index constants and double the whole size.
This is a stop-gap PR to do _some_ memory accounting during the execution of `crdb_internal.jobs`. It turns out that with hundreds of databases and hours backups per database, one can easily get to 100k jobs which are roughly 2-3Kb each. Given the total lack of memory monitoring here, this situation can lead to OOMs. Ideally we'd track the memory usage by the internal executor. That's a bigger change left for later. Also ideally we'd paginate the output. Touches cockroachdb#44166. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go, line 490 at r1 (raw file):
Previously, ajwerner wrote…
... unmarshal it and keep that around.?
Done.
pkg/sql/crdb_internal.go, line 493 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
Oh I see. Yeah I'd just get rid of the "precise" stuff here with the column integer index constants and double the whole size.
Done.
This is a stop-gap PR to do some memory accounting during the
execution of
crdb_internal.jobs. It turns out that with hundreds ofdatabases and hours backups per database, one can easily get to 100k jobs
which are roughly 2-3Kb each. Given the total lack of memory monitoring here,
this situation can lead to OOMs.
Ideally we'd track the memory usage by the internal executor. That's a
bigger change left for later. Also ideally we'd paginate the output.
Touches #44166.
Release note: None