Re: Enhancing Memory Context Statistics Reporting

Hi,

I have attached a version 40 patch that has been rebased onto the
latest master branch, as CFbot indicated a rebase was needed.
The test module patch is unchanged.

Thank you,
Rahila Syed

On Tue, Oct 28, 2025 at 11:06 AM Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

> 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
>