Thread

Re: Enhancing Memory Context Statistics Reporting

Rahila Syed <rahilasyed90@gmail.com> — 2025-12-12T11:04:37Z

Hi Daniel,


>
> Thanks for the patch, below are a few comments and suggestions.  As I was
> reviewing I tweaked the below and have attached the comments as changes in
> 0003.
>

Thank you for the improvements.
All your changes look good to me. I have incorporated those in the v44
patch.


> +    * Entry has been deleted due to client process exit. Make sure that
> the
> +    * client always deletes the entry after taking required lock or this
> +    * function may end up writing to unallocated memory.
> Can you explain this a bit further, I'm not sure I get it.  The code goes
> on to
> release a lock immediately and then destroys the hash.  Who is responsible
> for
> destroying the entry?
>

This just points to the general requirements of taking a lock before
writing to a shared variable.
This serves as a warning to other processes not to delete the entry without
taking a lock, since
we are about to write to the entry.


> == In ProcessGetMemoryContextInterrupt()
> I'm not a fan of having to exit's from the function doing duplicative
> cleanups,
> in the attached I've wrapped them in a conditional to just have one exit
> path.
> What do you think about that?
>

I agree with your approach.  It certainly makes the code more concise and
easier to read.


> == In PublishMemoryContext()
> +   /* Allocate DSA memory for storing path information */
> This comment is no longer accurate is it?  The DSA has already been
> allocated
> at this point.
>
>
Yes, it is not valid anymore. Fixed accordingly.

Apart from this, I cleaned up the test module by removing unnecessary sql
functions, added some more injection points based tests and a few
minor tweaks.

Please find attached updated and rebased patches.

Thank you,
Rahila Syed