-
-
Notifications
You must be signed in to change notification settings - Fork 262
Per pagespace I/O statistics and new trace API interfaces to allow extendable statistics #8808
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
base: master
Are you sure you want to change the base?
Conversation
dyemanov
commented
Nov 22, 2025
- Deprecate non-extendable PerformanceInfo struct in favor of the new PerformanceCounters/PerformanceStats interfaces
- Adjust the trace implementation to the new API
- Add internal support for per-pagespace I/O statistics. It's not used now, but will be used by PR Support for tablespaces #8314. This part is moved here to simplify review of the tablespaces.
…e PerformanceInfo struct in favor of the new PerformanceCounters/PerformanceStats interfaces. Adjust the trace implementation to the new API.
|
Documentation is missing. |
| uint64 getElapsedTime(); // in milliseconds | ||
| uint64 getFetchedRecords(); | ||
|
|
||
| PerformanceCounters getGlobalCounters(); |
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.
If you claim expandability, this must be one function with enum parameter: cGlobal, cPage, cRelation.
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.
We can easily add more functions to extend the interface. But I may agree a single function would be simpler.
|
|
||
| uint getId(uint index); | ||
| const string getName(uint index); | ||
| const int64* getCounterVector(uint index); |
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.
TTL of returned value is...?
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.
TTL of returned value is...?
It corresponds to the object TTL.
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.
Well... Returning of a plain array is simple and effective, but doesn't quite fit the ideology of OO API. How about returning of another object with methods getRead, getWrites, getFetches, etc? In this case getVectorCapacity is not needed (I guess).
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.
Valid point that I don't have a strong opinion about. Given the performance stats being somewhat low-level thing, I tried to follow the old-style interface with struct and counter vector, just made it extendable for new counter groups. Also, when we add new counters into the engine they automatically become available in the API, even being undocumented as enum members they're still usable without the need to recompile your trace plugin. With the pure OO approach the trace plugin should be recompiled to utilize new interface methods (and they should be added every time we add new counters).
@AlexPeshkoff @hvlad @asfernandes What do you think about this?
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.
I do not think that adding separate method for each counter is good idea. An additional argument not to follow OO rules too strict here is that calling virtual function once and working with array elements after it should be faster than calling N virtual functions.
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.
Also, when we add new counters into the engine they automatically become available in the API, even being undocumented as enum
How do a v7 caller of this interface can know the returned array from a v6 implementation does not have its expected items?
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.
That's NOT the reason.
In the beginning I voted for pure virtual interfaces. That time non-virtual CLOOP functions was argument against them.
Looks like you missed the whole thing or are insisting on bad things 20 years later. Virtual functions has no standard ABI layout between different compilers.
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.
Virtual functions has no standard ABI layout between different compilers.
You are dead wrong.
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.
I do not think that adding separate method for each counter is good idea. An additional argument not to follow OO rules too strict here is that calling virtual function once and working with array elements after it should be faster than calling N virtual functions.
+1
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.
Also, when we add new counters into the engine they automatically become available in the API, even being undocumented as enum
How do a v7 caller of this interface can know the returned array from a v6 implementation does not have its expected items?
If you want to access 9th counter, but getVectorCapacity() returns 7, then you're out of luck.
| uint getVectorCapacity(); | ||
|
|
||
| uint getId(uint index); | ||
| const string getName(uint index); |
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.
ID and name of... what?
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.
ID and name of... what?
Object (table, tablespace, whatever) counters belong to. Would you suggest getObjectId() / getObjectName()?
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.
No, I just wonder what these methods shall return for Global and Page counters.
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.
Nothing (zero) for global, valid id/name otherwise.
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.
Perhaps, it could be useful to return hardcoded "Global" and "Pages" in these cases...
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.
Would you suggest
getObjectId() / getObjectName()?
Yes, please. Now it is confusing as could be read as ObjectName or CounterName
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.
No, I just wonder what these methods shall return for Global and Page counters.
Thinking twice, I suppose getGlobalCounters() is not needed at all. Currently, it's used only to report page-level stats in the trace and this output will be modified anyway when tablespaces are merged (per-tablespace stats will be printed instead). And any global counter may be aggregated manually from the appropriate PerformanceCounters interface.
True, to be added. |
| interface PerformanceCounters : Versioned | ||
| { | ||
| uint getCount(); | ||
| uint getVectorCapacity(); |
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.
What it purpose of this method ? Why getCount() is not enough ?
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.
getCount() returns a number of objects (tables, tablespaces, etc), each of them has a vector of getVectorCapacity() counters. getVectorCapacity() is intended to ensure compatibility without interface versioning -- you can easily check whether the engine supports the counter you'd like to access.
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.
I suppose getObjectCount() would suit better here, given we discuss getObjectId() / getObjectName().
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.
I would suggest maxCounter because it is count of counters, not objects, and it is constant for every object/counter's set.
The trace interfaces seem being not documented entirely, see Using_OO_API.md:
I'm not in position to document the whole bunch of the trace interfaces right now, sorry. So documentation gonna be deferred. |
Limit to your new interfaces and methods. |
…e separate global counters. Misc renaming.