Add more columns/dimensions to pg_wait_sampling#97
Add more columns/dimensions to pg_wait_sampling#97
Conversation
|
@Medvecrab Performance penalty is exact risk that I would like to avoid while still want to have some extra dimensions. So my suggestion to add fields from PGPROC to the "main" views and fields from |
The idea of *_extended views is to add ALL additional fields to them (also any that could be added in the future) so the original views remain the same for backwards compatibility. I agree that if we were rewriting pg_wait_sampling anew, it would make some sense to distribute columns differently, but alas. This is also the reason that with my patch we still set profiling per profile/query_id with only the old GUC variables, not with the new ones When all fields from |
|
agree with your point, just one more comment, can we add isBackgroundWorker from PGPROC? I understand that backend_type is more detailed, but it we can get it without performance penalty while we can distinct regular backends in performance analysis already |
|
Seems like a fair request, will probably add after/during review |
…sions Sometimes it can be useful to have additional info collected with wait_events, so we add two new views/functions that include more information. The structure of those views could be changed in new versions of pg_wait_sampling extension
Also fix some typos/reword some sentences
Medvecrab
force-pushed
the
new_dimensions
branch
from
1597439 to
01c45a4
Compare
- Rename some columns in code and in GUC allowed values - Change client_addr type to inet as in pg_stat_activity - event value for dimension GUC now turn both wait_event and wait_event_type on and off; event_type not supported - README formatting - Hide leader_pid for leader process - Reset profile and history after reloading configuration, but only if dimensions have changed - Disallow passing empty string as dimensions - pid and queryid are NULL now if are not in dimenstions - empty client_hostname is now NULL like in pg_stat_activity - empty appname is now shown as empty string as in pg_stat_activity
- Remove unneded palloc - Move initialization of possibly-null fields behind mask-checking (in fill_values_and_nulls function) - Switch api_version and parameter amount in *_internal functions, this makes safety check look better - Remove pgstat_clear_*** from pg_wait_sampling_get_current, it could mess with other functions unexpectedly - Use correct bitmask in get_profile/history_internal - Add dimensions_mask to Profile and History to save it through multiple calls to get_profile/history - Use statically allocated varialbes ts and count in deserialize_array - Take care of padding in deserialize_array - Leave only common_dimensions in probe_waits - Remove unneded +1 in serialize_item - Remove serialized_key in probe_waits, it's not needed - Add pfree to stop leaking serialized_item - Always copy only count to hash table in probe_waits
|
This pull request is obsoleted by #104 |
2 participants
Following the discussion in #94, I decided to add more columns to pg_wait_sampling_current/history/profile
I've added most of the columns from request in #94, but some fields from "pg_stat_activity" (=
localBackendStatusTable) like query_text, query_start, query_id (if we actually took it from "pg_stat_activity") work in the following way: they save values at the start of the query and DO NOT clear those fields when the query has ended. So after executing "select 1;" in some client backend we will always sample its query_text and query_start until we execute other query. We specifically avoided this kind of sampling when we were making our way of tracking query_id with Executor hooks.All new columns are added to new views with suffix "_extended". Those views COULD be changed between extension versions, while existing views without those suffixes SHOULD NOT be changed.
One more thing to highlight - since we have to look into
localBackendStatusTablewhen we sample some columns, we have reduced perfomance. This is unavoidable and is noted in updated README. BUT, in PostgreSQL 13-16 we can't reliably linkProcGlobalandlocalBackendStatusTablearrays and from this was bornget_beentry_by_procpid. For each interesting process fromProcGlobalwe have to iterate throughlocalBackendStatusTable. This has O(n^2) time complexity, where n is a number of all backends. Very inefficient. I probably could remake the collector code to iterate throughlocalBackendStatusTablefirst and then find corresponding entries inProcGlobalbut I have not investigated it and am not sure it would be faster (it may be faster, depending onProcGlobalaccess/iteration)There could some sloppy code, including possible "you should copy the struct here, not use pointer" moments
Everyone is welcome to review the code and share their thoughts.