This commit fix some issues when the query buffer overflows and
pg_stat_monitor attempts to dump its contents to a file.
The dump process is now as follows:
1. The dump will always be a full dump of the current query buffer,
meaning pg_stat_monitor will dump MAX_QUERY_BUFFER_BUCKET bytes to
the dump file.
2. When scanning the dump file, read chunks of size
MAX_QUERY_BUFFER_BUCKET, then look for the query ID using that chunk
and the query position metadata, this allows pg_stat_monitor to avoid
scanning the whole chunk when looking for a query ID.
The code in charge to read from/write to the dump file now takes into
account that read() and write() may return less bytes than what it was
asked for, the code now ensures that we actually read or write the
amount of bytes required (MAX_QUERY_BUFFER_BUCKET), also it handles
rare but posssible interrupts when doing those operations.
In SaveQueryText() we check for a possible overflow in the query buffer,
but if overflow would happen and pgsm_overflow_target value is 1 (the
default), then we dump the query buffer to a temporary file and reset
the buffer (start saving queries from the start of the buffer). The
problem is that after resetting the buffer we don't check if the current
query length would exceed the buffer size of MAX_QUERY_BUFFER_BUCKET, if
that is the case the buffer would overflow and probably crash the
process or in the worst case become an attack vector for exploitation.
This commit fix the problem by adding an additional check for overflow
after resetting the query buffer.
The GUC variable pgsm_overflow_target was pointing to the wrong index
(12, pgsm_track_planning) in the "GucVariable conf[MAX_SETTINGS]" array.
Adjusted it to the right index, 11.
The regular expression was updated to properly capture comments in SQL
in the form /* */.
The previous regex was capturing everything from /* until the last */
because regex are greedy, this was presenting problems if a input query
has something like:
SELECT /* comment 1 */ field /* comment 2 */ from FOO;
As such, the previous regex would capture anytying between /* comment 1
and comment 2 */, the result would be:
/* comment 1 field comment 2*/.
Multiline comments are also captured.
Multiple comments in one query are now stored in the pg_stat_monitor
comments field in the form: /* Comment 1 */, ... , /* Comment N */.
The code added in pgss_store() to handle an assertion failure when
GetUserId() was being called introduced a problem with regression tests
in some builds, specifically our PG11 and PG12 distributions for Ubuntu.
The problem was detected when calling some json functions that would
trigger parallel workers, to solve the problem now we check if our hooks
are being called by parallel workers, in this case we can avoid doing
work, it also fixes the issue mentioned above as we won't call
GetUserId() anymore in an invalid context.
The regression tests required some adjustmentes as they were based on a
wrong behavior in pg_stat_monitor that was fixed in the last commits.
The problem was that pg_stat_monitor_reset() was not properly clearing
the query buffers, as such, some garbage queries were residing in the
buffers after calling pg_stat_monitor_reset().
One example of a problem, a query such as "SELECT 1 AS num" and the
same query with comments such as:
SELECT $1 AS num /* { "application", psql_app, "real_ip", 192.168.1.3)
*/
Are evaluated to the same query ID, if a test issue the first query, call
pg_stat_monitor_reset() to clear query buffer, then issue the second
query with comments, the result in pg_stat_monitor view would still contain
the first query without comments, this was leading to tests expecting
the wrong output, which is now fixed.
Don't display queries in PARSE or PLAN state, to keep it consistent
with previous behavior, this avoids showing intermediate scripts like
part of a procedure, such as:
$1
$1 := $3
As codecov is not approved by IT due to some known vulnerabilities in the past, reverting back to coverall.io to make sure that at least code coverage is available.
Whenever a new query is added to a query buffer, record the position
in which the query was inserted, we can then use this information to
locate the query in a faster way later on when required.
This allowed to simplify the logic in hash_entry_dealloc(), after
creating the list of pending queries, as the list is scanned we can copy
the query from the previous query buffer to the new one by using the
query position (query_pos), this avoids scanning the whole query buffer
when looking up for the queryid.
Also, when moving a query to a new buffer (copy_query), we update
the query_pos for the hash table entry, so it points to the right place
in the new query buffer.
read_query() function was updated to allow query position to be passed
as argument, if pos != 0 use it to locate the query directly, otherwise
fallback to the previous mode of scanning the whole buffer.
SaveQueryText() was updated to pass a reference to the query position as
argument, this value is updated after the function finishes with the
position that the query was stored into the buffer.
There was no necessity for using a separate hash table to keep track
of queries as all the necessary information is already available in
the pgss_hash table.
The code that moves pending queries' text in hash_query_entry_dealloc
was merged into hash_entry_dealloc.
Couple functions were not necessary anymore, thus were removed:
- hash_create_query_entry
- pgss_store_query_info
- pgss_get_entry (this logic was added directly into pgss_store).
We simplified the logic in pgss_store, it basically works as follows:
1. Create a key (bucketid, queryid, etc...) for the query event.
2. Lookup the key in pgss_hash, if no entry exists for the key, create a
new one, save the query text into the current query buffer.
3. If jstate == NULL, then update stats counters for the entry.
There were many variables being initialized in pgss_store() before
checking if the module was actually active, this would waste cpu
processor if the module is disabled.
To fix that, declare variables and initialize them only after check
that pg_stat_monitor is active.
Added code to move all pending queries text from an expired bucket's query
buffer to the next, active query buffer.
The previous implementation was not very efficient, it worked like this,
as soon as a query is processed and a bucket expires:
1. Allocate memory to save the contents of the next query buffer.
2. Clear the next query buffer.
3. Iterate over pgss_query_hash, then, for each entry:
- If the entry's bucket id is equal to the next bucket then:
-- If the query for this entry has finished or ended in error, then
remove it from the hash table.
-- Else, if the query is not yet finished, copy the query from the
backup query buffer to the new query buffer.
Now, this copy was really expensive, because it was implemented
using read_query() / SaveQueryText(), and read_query() scans the
whole query buffer looking for some query ID, since we do this
extra lookup loop for each pending query we end up with a O(n^2)
algorithm.
4. Release the backup query buffer.
Since now we always move pending queries from an expired bucket to the
next one, there is no need to scan the next query buffer for pending
queries (the pending queries are always in the current bucket, and when
it expires we move them to the next one).
Taking that into consideration, the new implementation works as follows,
whenever a bucket expires:
1. Clear the next query buffer (all entries).
2. Define an array to store pending query ids from the expired bucket,
we use this array later on in the algorithm.
3. Iterate over pgss_query_hash, then, for each entry:
- If the entry's bucket id is equal to the next bucket then:
-- If the query for this entry has finished or ended in error, then
remove it from the hash table. This is equal to the previous
implementation.
- Else, if the entry's bucket id is equal to the just expired bucket
id (old bucket id) and the query state is pending (not yet finished),
then add this query ID to the array of pending query IDs.
Note: We define the array to hold up to 128 pending entries, if there
are more entries than this we try to allocate memory in the heap to
store them, then, if the allocation fails we manually copy every
pending query past the 128th to the next query buffer, using the
previous algorithm (read_query() / SaveQueryText), this should be a
very rare situation.
4. Finally, if there are pending queries, copy them from the previous
query buffer to the next one using copy_queries.
Now, copy_queries() is better than looping through each query entry and
calling read_query() / SaveQueryText() to copy each of them to the new
buffer, as explained, read_query() scans the whole query buffer for
every call.
copy_queries(), instead, scans the query buffer only once, and for every
element it checks if the current query id is in the list of queries to
be copied, this is an array of uint64 that is small enough to fit in L1
cache.
Another important fix in this commit is the addition of the line 1548 in
pg_stat_monitor.c / pgss_store():
query_entry->state = kind;
Before the addition of this line, all entries in the pgss_query_hash
hash table were not having their status updated when the query entered
execution / finished or ended in error, effectively leaving all entries
as pending, thus whenever a bucket expired all entries were being copied
from the expired bucket to the next one.
The if condition bellow in geta_next_wbucket() was subject to a race
condition:
if ((current_usec - pgss->prev_bucket_usec) > (PGSM_BUCKET_TIME * 1000 *
1000))
Two or more threads/processes could easily evaluate this condition to
true, thus executing more than once the block that would calculate a
new bucket id, clear/move old entries in the pgss_query_hash and
pgss_hash hash tables.
To avoid this problem, we define prev_bucket_usec and current_wbucket
variables as atomic and execute a loop to check if another thread has
updated prev_bucket_usec before the current one.
Ibrar Ahmed
Anastasia Alexadrova
Roma Novikov
EvgeniyPatlan
Denys Kondratenko
Diego Fronza
Naeem Akhter