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
v44-0001-Add-function-to-report-memory-context-statistics.patch
Description: Binary data
v44-0002-Test-module-to-test-memory-context-reporting-wit.patch
Description: Binary data
