Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Per pagespace I/O statistics and new trace API interfaces to allow extendable statistics #8808

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9304339
Add support of grouping page-level I/O counters per pagespace
dyemanov Nov 18, 2025
e0817d0
Add support for per-pagespace I/O statistics. Deprecate non-extendabl…
dyemanov Nov 22, 2025
45ec316
Better names for interface methods. Add the basic docs. Get rid of th…
dyemanov Nov 26, 2025
089060f
Add the docs weirdly escaped from the last commit
dyemanov Nov 28, 2025
cfb27d1
Follow Dimitry Sibiryakov's suggestion to unify get*Counters methods.
dyemanov Nov 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/include/firebird/FirebirdInterface.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,9 @@ interface TraceTransaction : Versioned
version: // 3.0.4 -> 3.0.5
int64 getInitialID();
int64 getPreviousID();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceParams : Versioned
Expand All @@ -1379,6 +1382,9 @@ interface TraceStatement : Versioned
{
int64 getStmtID();
PerformanceInfo* getPerf();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceSQLStatement : TraceStatement
Expand Down Expand Up @@ -1421,6 +1427,9 @@ version: // 4.0 -> 5.0
int64 getStmtID();
const string getPlan();
const string getExplainedPlan();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceFunction : Versioned
Expand All @@ -1434,6 +1443,9 @@ version: // 4.0 -> 5.0
int64 getStmtID();
const string getPlan();
const string getExplainedPlan();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceTrigger : Versioned
Expand All @@ -1456,6 +1468,9 @@ version: // 4.0 -> 5.0
int64 getStmtID();
const string getPlan();
const string getExplainedPlan();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceServiceConnection : TraceConnection
Expand All @@ -1480,6 +1495,9 @@ interface TraceSweepInfo : Versioned
int64 getOAT();
int64 getNext();
PerformanceInfo* getPerf();

version: // 5.0 -> 6.0
PerformanceStats getPerfStats();
}

interface TraceLogWriter : ReferenceCounted
Expand Down Expand Up @@ -1876,3 +1894,26 @@ interface ProfilerStats : Versioned
{
uint64 getElapsedTicks();
}

// Extendable replacement for struct PerformanceInfo

interface PerformanceCounters : Versioned
{
uint getCount();
uint getVectorCapacity();
Copy link
Member

@hvlad hvlad Nov 24, 2025

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 ?

Copy link
Member Author

@dyemanov dyemanov Nov 24, 2025 •
edited
Loading

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.

Copy link
Member Author

@dyemanov dyemanov Nov 24, 2025

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().

Copy link
Contributor

@aafemt aafemt Nov 24, 2025

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.


uint getId(uint index);
const string getName(uint index);
Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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?

Copy link
Member Author

@dyemanov dyemanov Nov 22, 2025

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()?

Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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.

Copy link
Member Author

@dyemanov dyemanov Nov 22, 2025

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.

Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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...

Copy link
Member

@hvlad hvlad Nov 24, 2025

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

Copy link
Member Author

@dyemanov dyemanov Nov 26, 2025

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.

const int64* getCounterVector(uint index);
Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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...?

Copy link
Member Author

@dyemanov dyemanov Nov 22, 2025

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.

Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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).

Copy link
Member Author

@dyemanov dyemanov Nov 24, 2025

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?

Copy link
Member

@AlexPeshkoff AlexPeshkoff Nov 24, 2025

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.

Copy link
Member

@asfernandes asfernandes Nov 24, 2025

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?

Copy link
Member

@asfernandes asfernandes Nov 24, 2025

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.

Copy link
Contributor

@aafemt aafemt Nov 24, 2025

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.

Copy link
Member

@hvlad hvlad Nov 24, 2025

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

Copy link
Member Author

@dyemanov dyemanov Nov 24, 2025

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.

}

interface PerformanceStats : Versioned
{
uint64 getElapsedTime(); // in milliseconds
uint64 getFetchedRecords();

PerformanceCounters getGlobalCounters();
Copy link
Contributor

@aafemt aafemt Nov 22, 2025

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.

Copy link
Member Author

@dyemanov dyemanov Nov 22, 2025

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.

PerformanceCounters getPageCounters();
PerformanceCounters getRelationCounters();
}

Loading
Loading