Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

PSQL and SQL profiler #7086

Merged
merged 30 commits into from
Aug 11, 2022
Merged

PSQL and SQL profiler #7086

merged 30 commits into from
Aug 11, 2022

Conversation

asfernandes
Copy link
Member

No description provided.

@asfernandes
Copy link
Member Author

Are there any objections to integrate this in master, or anything yet to discuss?

@romansimakov
Copy link
Contributor

In firebird-devel we talked about an ability to profile other connections. Has it been added or are you going to add it later?

@asfernandes
Copy link
Member Author

In firebird-devel we talked about an ability to profile other connections. Has it been added or are you going to add it later?

I think this change would not invalidate the current design, so I think it does not make much sense to continue adding features before an initial integration and lack of feedback on what we have now.

@romansimakov
Copy link
Contributor

In firebird-devel we talked about an ability to profile other connections. Has it been added or are you going to add it later?

I think this change would not invalidate the current design, so I think it does not make much sense to continue adding features before an initial integration and lack of feedback on what we have now.

I don't mind if so

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

I'd prefer the default plugin to be used without naming in START_SESSION (for example, declare NULL as a default value). It could be done via DefaultProfilerPlugin option in firebird.conf, for example. Alternatively, we may have ProfilerPlugin option in firebird.conf (similar to TracePlugin = fbtrace) that substitutes the missing plugin name in START_SESSION to any specified plugin (not only the default one). One may still specify the plugin name explicitly in START_SESSION, if necessary.

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

Once we support profiling other connections (I agree this is a top priority feature), should we extend START_SESSION with a new parameter (ATTACHMENT_ID DEFAULT CURRENT_ATTACHMENT) or some new function will be introduced?

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

While I don't mind CREATE_SESSION and FINISH_SESSION routine names, I'd suggest to rename SESSION_ID columns inside tables to PROFILE_ID. Having SESSION_ID and ATTACHMENT_ID side to side looks somewhat ambiguous, considering we have SESSION_ID and USER_SESSION meaning attachments in RDB$GET_CONTEXT, and also considering the SQL specification having SESSION_USER where session also means an attachment.

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

Why FBPROF$ prefix for tables/views instead of just PROF$? We're Firebird anyway, so what's the point.

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

Given the default profiler plugin is supposedly to be really usable (not just a draft example of how things should be coded), why its tables/views are created dynamically rather than being part of ODS? Are they expected to change significantly with time?

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

I really miss timings (count/min/max/total) for statements as a whole. It would be inconvenient to measure them with some external tools and then look into PROF$ tables/views for details, it would be more handy to have everything in a single place. Especially if we speak about multiple executions of a single prepared/cached statement.

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

I've attempted to collect execution times in the past, but wanted to split them into total/cpu/wait parts, with the wait part being also detailed (I/O, lock, latch, pause, etc). Given that extra measurements are not always dirty cheap, I had a doubt they should be presented via MON$ tables unconditionally. Now it looks like it could be integrated with your profiler design after it's committed and thus measured on demand. But the question is how deep we need to dive into the CPU time. Is it OK to calculate it as (total - wait) (or leave this calculation up to users), or do we want to see real CPU time and possibly deal with some delta (total - wait - cpu) that remains unknown. And if we need real CPU time, do we want user/kernel times? All these things complicate the implementation and make it somewhat platform-dependent.

@dyemanov
Copy link
Member

dyemanov commented May 1, 2022

I feel the profiler package also needs a routine SET_FLUSH_INTERVAL that does flushing automagically, based on timer. Zero (default) means manual flushing, as designed currently.

@aafemt
Copy link
Contributor

aafemt commented May 1, 2022

It could be done via DefaultProfilerPlugin option in firebird.conf, for example. Alternatively, we may have ProfilerPlugin option in firebird.conf (similar to TracePlugin = fbtrace)

Talking about configuration: because profiling is entirely engine-related, I'd say that this setting should belong to Engine14.conf, not firebird.conf.

@asfernandes
Copy link
Member Author

Given the default profiler plugin is supposedly to be really usable (not just a draft example of how things should be coded), why its tables/views are created dynamically rather than being part of ODS? Are they expected to change significantly with time?

  1. Then it's not really plugin design
  2. It would require many code to make backup and restore of them work
  3. It would not support primary and foreign keys

@asfernandes
Copy link
Member Author

Why FBPROF$ prefix for tables/views instead of just PROF$? We're Firebird anyway, so what's the point.

My idea was to have a FB namespace to differentiate things from users objects, as they can also use the dollar sign.

But since it's new name convention, I would not have a problem to change it.

We also have PLG$* tables, so maybe we should name them PLG$PROF*?

@asfernandes
Copy link
Member Author

I really miss timings (count/min/max/total) for statements as a whole. It would be inconvenient to measure them with some external tools and then look into PROF$ tables/views for details, it would be more handy to have everything in a single place. Especially if we speak about multiple executions of a single prepared/cached statement.

How these stored timings would be different than aggregate the request-based timings per STATEMENT_ID?

@asfernandes
Copy link
Member Author

I feel the profiler package also needs a routine SET_FLUSH_INTERVAL that does flushing automagically, based on timer. Zero (default) means manual flushing, as designed currently.

Data (even when flushed) is stored as part of user transaction, that may be rolled back later.

I do not see a way that automatic flush would work with this or would be less confusing than manually flush the data before read.

But this could be useful in the case of profiling others connections.

@dyemanov
Copy link
Member

dyemanov commented May 4, 2022

We also have PLG$* tables, so maybe we should name them PLG$PROF*?

This looks good to me.

@dyemanov
Copy link
Member

dyemanov commented May 4, 2022

Data (even when flushed) is stored as part of user transaction, that may be rolled back later.

Automatic flushing could behave like it's executed in autonomous transaction. Rollback will surely not be possible, but one may always delete the rows manually. This should be well-documented, of course.

@dyemanov
Copy link
Member

dyemanov commented May 4, 2022

How these stored timings would be different than aggregate the request-based timings per STATEMENT_ID?

They cannot be aggregated from RECORD_SOURCE_STATS, as time may also be spent between cursor operations. Perhaps it could be aggregated from PSQL_STATS, but only for PSQL routines. Imagine I execute INSERT FROM SELECT and 90% of time is spent inside VIO_store(), how could I get the total execution time for my statement? What about non-PSQL procedures/functions?

@AlexPeshkoff
Copy link
Member

AlexPeshkoff commented May 4, 2022 via email

@asfernandes
Copy link
Member Author

I've implemented profiling of others attachments with this commit set. The semantics of it is documented in the readme.

To make things not confusing, user's transaction is not passed to plugins anymore and flush always starts its own transaction.

hvlad and others added 5 commits June 4, 2022 15:45
…of MSVC.

Note, it is not documented so far for newly released  MSVC 17.1.
Assume _MSC_VER will be increased to be >= 2000 when\if VC CRT library get new version number in suffix.
Add parameter FLUSH_INTERVAL to START_SESSION.
@asfernandes
Copy link
Member Author

How these stored timings would be different than aggregate the request-based timings per STATEMENT_ID?

They cannot be aggregated from RECORD_SOURCE_STATS, as time may also be spent between cursor operations. Perhaps it could be aggregated from PSQL_STATS, but only for PSQL routines. Imagine I execute INSERT FROM SELECT and 90% of time is spent inside VIO_store(), how could I get the total execution time for my statement? What about non-PSQL procedures/functions?

I think we need TOTAL time of requests (as stored in PLG$PROF_REQUESTS) which is valid for SQL and PSQL.

Then it's possible to calculate MIN/MAX per STATEMENT (as stored in PLG$PROF_STATEMENTS).

@asfernandes
Copy link
Member Author

I've attempted to collect execution times in the past, but wanted to split them into total/cpu/wait parts, with the wait part being also detailed (I/O, lock, latch, pause, etc). Given that extra measurements are not always dirty cheap, I had a doubt they should be presented via MON$ tables unconditionally. Now it looks like it could be integrated with your profiler design after it's committed and thus measured on demand. But the question is how deep we need to dive into the CPU time. Is it OK to calculate it as (total - wait) (or leave this calculation up to users), or do we want to see real CPU time and possibly deal with some delta (total - wait - cpu) that remains unknown. And if we need real CPU time, do we want user/kernel times? All these things complicate the implementation and make it somewhat platform-dependent.

AFAIK there is no way to directly get the "wait time" from the OS. It's the elapsed time minus the thread's CPU time (user + kernel).

I think we can pass the elapsed and total thread's CPU time. The helper views could also calculate the total wait time.

We should decide if the APIs that currently receives only uint64 runTime would get a new uint64 cpuTime parameter, hence will not be extensible.

Or if we would need to add another interface which would be extensible and plugin writers would call its methods to get timings. The downside of this is that it will be slower.

@dyemanov
Copy link
Member

Yes, no way to get wait time from the system. But we can measure "logical waits" ourselves in most points where we checkout from the engine. Some of them won't be honest (IO wait time will actually be IO CPU time when reads are performed from filesystem cache, for example), but this is OK as they're expected to be short and unlikely to be noticeable as "top" waits. If the CPU time is also measured, then we could calculate "wait" as "total time - CPU time" and it should be more or less real, but I doubt it gonna be useful. So I'm somewhat skeptical in whether we really need CPU time...

Add ProfilerStats interface and pass it to plugin instead of runTime parameter.

Rename *_TIME columns to *_ELAPSED_TIME.
@asfernandes
Copy link
Member Author

Anyone see a blocking point to have this merged in master?

@AlexPeshkoff
Copy link
Member

AlexPeshkoff commented Oct 11, 2022 via email

@asfernandes
Copy link
Member Author

asfernandes commented Oct 11, 2022 via email

@pavel-zotov
Copy link

@@@ QA issue @@@
Test verifies only example from doc/sql.extensions/README.profiler.md
More complex checks will be implementer later.

@asfernandes asfernandes deleted the work/profiler-plugin branch April 9, 2024 00:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants