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

storage: track iteration stats per engine.Iterator #59069

Closed
sumeerbhola opened this issue Jan 15, 2021 · 1 comment
Closed

storage: track iteration stats per engine.Iterator #59069

sumeerbhola opened this issue Jan 15, 2021 · 1 comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@sumeerbhola
Copy link
Collaborator

sumeerbhola commented Jan 15, 2021

For better observability into the behavior of a query, we should be tracking detailed stats on iterator behavior, and log them to the query trace. This will be helpful when diagnosing slow queries. Using node profiles, like we did in diagnosing slow queries due to garbage in the LSM here https://github.com/cockroachlabs/support/issues/710#issuecomment-737607669 is not in general an effective technique.
And as we increase the minimum size of a replica, we will potentially increase the variability in different parts of the key space within the replica, so replica level MVCCStats may not be a good enough indicator of MVCC garbage encountered by a query.

These stats can also be aggregated per-replica, and be an indicator of replica read load.

This will require changes in Pebble too, since we don't currently track stats in the iterator stack consisting of Iterator, mergingIter, levelIter.
Some examples of counts that would be useful:

  • Obsolete MVCC versions encountered (measured in the storage.pebbleIterator).
  • Failed nexts before a seek (in pebbleMVCCScanner)
  • Obsolete Pebble versions
  • key-value pairs iterated over by Pebble. The total bytes (key+value) iterated over (even if we didn't read the value).
  • ssblocks loaded.
  • pebble.Iterator seeks and file level seeks.
  • Number of nexts done by mergingIter. This can help find instances of perf: relative positioning through broad range tombstones is slow pebble#1070

Jira issue: CRDB-3336

@sumeerbhola sumeerbhola added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-storage Relating to our storage engine (Pebble) on-disk storage. labels Jan 15, 2021
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue May 5, 2021
The calls to internalIterator are tracked in Iterator
since we use both mergingIter and getIter in that role,
and we probably don't need these stats for background
work (compactions) involving mergingIter.

Informs cockroachdb/cockroach#59069

Intended to unblock
cockroachdb/cockroach#64503
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue May 5, 2021
The calls to internalIterator are tracked in Iterator
since we use both mergingIter and getIter in that role,
and we probably don't need these stats for background
work (compactions) involving mergingIter.

Informs cockroachdb/cockroach#59069

Intended to unblock
cockroachdb/cockroach#64503
sumeerbhola added a commit to cockroachdb/pebble that referenced this issue May 5, 2021
The calls to internalIterator are tracked in Iterator
since we use both mergingIter and getIter in that role,
and we probably don't need these stats for background
work (compactions) involving mergingIter.

Informs cockroachdb/cockroach#59069

Intended to unblock
cockroachdb/cockroach#64503
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Feb 25, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over by the mergingIter. The latter includes points
that were ignored because of range tombstones.

These stats can be used to make inferences about root causes
of slow scans.

The stats are introduced by adding an InternalIteratorWithStats
interface that extends InternalIterator. Not all iterators need
to implement this interface (block iter, memtable iter etc.).
Flexibility of plugging in InternalIterators in various places
(especially for testing) is preserved by adding a trivial wrapper,
internalIteratorWithEmptyStats. This wrapping introduces a bug
risk that an InternalIteratorWithStats could be wrapped with an
InternalIterator and then wrapped using this trivial wrapper,
thereby losing the real stats. To reduce this risk, mergingIter
and Iterator expect to be provided an InternalIteratorWithStats
for their typical initialization paths.

Informs cockroachdb#1342
Informs cockroachdb/cockroach#59069
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Feb 28, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over by the mergingIter. The latter includes points
that were ignored because of range tombstones.

These stats can be used to make inferences about root causes
of slow scans.

The stats are introduced by adding an InternalIteratorWithStats
interface that extends InternalIterator. Not all iterators need
to implement this interface (block iter, memtable iter etc.).
Flexibility of plugging in InternalIterators in various places
(especially for testing) is preserved by adding a trivial wrapper,
internalIteratorWithEmptyStats. This wrapping introduces a bug
risk that an InternalIteratorWithStats could be wrapped with an
InternalIterator and then wrapped using this trivial wrapper,
thereby losing the real stats. To reduce this risk, mergingIter
and Iterator expect to be provided an InternalIteratorWithStats
for their typical initialization paths.

Informs cockroachdb#1342
Informs cockroachdb/cockroach#59069
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Mar 1, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over by the mergingIter. The latter includes points
that were ignored because of range tombstones.

These stats can be used to make inferences about root causes
of slow scans.

The stats are introduced by adding an InternalIteratorWithStats
interface that extends InternalIterator. Not all iterators need
to implement this interface (block iter, memtable iter etc.).
Flexibility of plugging in InternalIterators in various places
(especially for testing) is preserved by adding a trivial wrapper,
internalIteratorWithEmptyStats. This wrapping introduces a bug
risk that an InternalIteratorWithStats could be wrapped with an
InternalIterator and then wrapped using this trivial wrapper,
thereby losing the real stats. To reduce this risk, mergingIter
and Iterator expect to be provided an InternalIteratorWithStats
for their typical initialization paths.

Informs cockroachdb#1342
Informs cockroachdb/cockroach#59069
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Mar 1, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over by the mergingIter. The latter includes points
that were ignored because of range tombstones.

These stats can be used to make inferences about root causes
of slow scans.

The stats are introduced by adding an InternalIteratorWithStats
interface that extends InternalIterator. Not all iterators need
to implement this interface (block iter, memtable iter etc.).
Flexibility of plugging in InternalIterators in various places
(especially for testing) is preserved by adding a trivial wrapper,
internalIteratorWithEmptyStats. This wrapping introduces a bug
risk that an InternalIteratorWithStats could be wrapped with an
InternalIterator and then wrapped using this trivial wrapper,
thereby losing the real stats. To reduce this risk, mergingIter
and Iterator expect to be provided an InternalIteratorWithStats
for their typical initialization paths.

Informs cockroachdb#1342
Informs cockroachdb/cockroach#59069
sumeerbhola added a commit to cockroachdb/pebble that referenced this issue Mar 1, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over by the mergingIter. The latter includes points
that were ignored because of range tombstones.

These stats can be used to make inferences about root causes
of slow scans.

The stats are introduced by adding an InternalIteratorWithStats
interface that extends InternalIterator. Not all iterators need
to implement this interface (block iter, memtable iter etc.).
Flexibility of plugging in InternalIterators in various places
(especially for testing) is preserved by adding a trivial wrapper,
internalIteratorWithEmptyStats. This wrapping introduces a bug
risk that an InternalIteratorWithStats could be wrapped with an
InternalIterator and then wrapped using this trivial wrapper,
thereby losing the real stats. To reduce this risk, mergingIter
and Iterator expect to be provided an InternalIteratorWithStats
for their typical initialization paths.

Informs #1342
Informs cockroachdb/cockroach#59069
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Mar 8, 2022
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over when merging across levels in the log-structured
merge tree. The latter includes points that were ignored because
of range tombstones, but could not be cheaply skipped.

These stats can be used to make inferences about root causes
of slow scans.

This change exposes these in roachpb.ScanStats and the trace
span recorded when doing a mvcc get or scan. There are todos for
plumbing all or part of it through the higher layers via
roachpb.ScanStats and execinfrapb.KVStats.

Informs cockroachdb#59069
Informs cockroachdb/pebble#1342

Release justification: low risk, and potentially high benefit
observability increase for existing functionality.

Release note: None
craig bot pushed a commit that referenced this issue Mar 9, 2022
77512: storage,roachpb: expose pebble.InternalIteratorStats for scans r=azhng,yuzefovich,nicktrav a=sumeerbhola

The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over when merging across levels in the log-structured
merge tree. The latter includes points that were ignored because
of range tombstones, but could not be cheaply skipped.

These stats can be used to make inferences about root causes
of slow scans.

This change exposes these in roachpb.ScanStats and the trace
span recorded when doing a mvcc get or scan. There are todos for
plumbing all or part of it through the higher layers via
roachpb.ScanStats and execinfrapb.KVStats.

Informs #59069
Informs cockroachdb/pebble#1342

Release justification: low risk, and potentially high benefit
observability increase for existing functionality.

Release note: None

77519: parser: adjust ALTER TENANT ALL syntax r=knz,ajstorm a=rafiss

This change brings the syntax inline to what was proposed in the RFC.
This is worthwhile since it matches with the syntax we use in other
"ALL" cases -- notably `ALTER ROLE ALL SET ...``.

Release justification: low risk update to new functionality
Release note: None

Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@sumeerbhola
Copy link
Collaborator Author

Marking this done since pebble.InternalIteratorStats are exposed in #77512, and the remaining plumbing of these stats through SQL execution is covered by #77580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

1 participant