Re: Enhancing Memory Context Statistics Reporting

Hi,

PFA an updated v39 patch which is ready for review in the upcoming
commitfest.

> v35 works fine on my environment.
> I ran the same test and haven’t encountered the crash anymore.
>

Thank you for testing and confirming the fix.

> The addition of the following code appears to have resolved the issue:
>
> +memstats_dsa_cleanup(char *key)
> +{
> + MemoryStatsDSHashEntry *entry;
> +
> + entry = dshash_find(MemoryStatsDsHash, key, true);
>

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.
>
$ git apply
> 0001-v35-0001-Add-pg_get_process_memory_context-function.patch
> 0001-v35-0001-Add-pg_get_process_memory_context-function.patch:705:
> trailing whitespace.
> 0001-v35-0001-Add-pg_get_process_memory_context-function.patch:1066:
> trailing whitespace.
>
>
Thanks, should be fixed now.

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.

Thank you,
Rahila Syed