Re: Enhancing Memory Context Statistics Reporting

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