Yes, without this code, the dsa memory was being freed in the timeout path without acquiring a lock.
Since you seem to make a next version patch, I understand v35 is an interim patch, so this isn’t a major concern, but I encountered trailing whitespace warnings when applying the patches.
The updated patch contains the following changes. These changes are addressing some review comments discussed off list and a couple of bugs found while doing injection points tests.
1.
All the changes made to MemoryContextStatsInternal and MemoryContextStatsDetail are removed. Instead of modifying these functions, I have written a separate function MemoryContextStatsCounter that takes care of counting statistics. This approach ensures that the existing functions remain unchanged.
2. Changes to ensure that the wait loop does not exceed the prescribed wait time. Additional exit condition has been added to the infinite loop that waits for request completion. This allows the pg_get_memoy_context_statistics function to return if the elapsed time goes beyond a set limit i.e the following timeout.
3. The user facing timeout is removed as that would complicate the user interface. CFIs are called frequently and the requests are likely to be addressed promptly. A predefined macro MEMORY_CONTEXT_STATS_TIMEOUT 5 (secs) is used for timeout
instead. This would also remove the possibility of a user setting very low timeouts, which could cause requests to be incomplete and result in NULL outputs.
4. Miscellaneous cleanups to improve comments and remove left over comments from older versions. Also, removed an unnecessary argument from the PublishMemoryContext function.
5. Addressed Torikoshias suggestion to change the order of columns to match pg_backend_memory_contexts.
6. Attached is a test module that tests error handling by introducing errors using injection points. I have resolved a few bugs, so the memory monitoring function
now runs correctly after the previous request ended with an error.
