I have incorporated all your changes in v20 patches and ensured that the review comments corresponding to 0001 patch are included in that patch and not in 0002.
+MEM_CTX_PUBLISH "Waiting for backend to publish memory information." I wonder if this should really be "process" and not backend?
Fixed.
+ default: + context_type = "???"; + break; In ContextTypeToString() I'm having second thoughts about this, there shouldn't be any legitimate use-case of passing a nodetag this function which would fail MemoryContextIsValid(). I wonder if we aren't helping callers more by erroring out rather than silently returning an unknown? I haven't changed this but maybe we should to set the API right from the start?
I cannot think of any legitimate scenario where the context type would be unknown. However, if we were to throw an error, it would prevent us from reporting any memory usage information when the context type is unidentified. Perhaps, it would be more informative and less restrictive to label it as "Unrecognized" or "Unknown." I wonder if this was the reasoning behind doing it when it was added with the pg_backend_memory_contexts() function.
+ /* + * Recheck the state of the backend before sleeping on the condition + * variable + */ + proc = BackendPidGetProc(pid); Here we are really rechecking that the process is still alive, but I wonder if we should take the opportunity to ensure that the type is what we expect it to be? If the pid has moved from being a backend to an aux proc or vice versa we really don't want to go on.
The reasoning makes sense to me. For periodic monitoring of all processes, any PID that reincarnates into a different type could be queried in subsequent function calls. Regarding targeted monitoring of a specific process, such a reincarnated process would exhibit a completely different memory consumption, likely not aligning with the user's original intent behind requesting the statistics.
+ ereport(WARNING, + errmsg("PID %d is not a PostgreSQL server process", + pid)); I wonder if we should differentiate between the warnings? When we hit this in the loop the errmsg is describing a slightly different case. I did leave it for now, but it's food for thought if we should perhaps reword this one.
Changed it to "PID %d is no longer the same PostgreSQL server process".
+ ereport(LOG, + errmsg("hash table corrupted, can't construct path value")); I know you switched from elog(LOG.. to ereport(LOG.. but I still think a LOG entry stating corruption isn't helpful, it's not actionable for the user. Given that it's a case that shouldn't happen I wonder if we should downgrade it to an Assert(false) and potentially a DEBUG1?
How about changing it to ERROR, in accordance with current occurrences of the same message? I did it in the attached version, however I am open to changing it to an Assert(false) and DEBUG1.
Apart from the above, I made the following improvements.
1. Eliminated the unnecessary creation of an extra memory context before calling hash_create. The hash_create function already generates a memory context containing the hash table, enabling easy memory deallocation by simply deleting the context via hash_destroy. Therefore, the patch relies on hash_destroy for memory management instead of manual freeing.
2. Optimized memory usage by storing the path as an array of integers rather than as an array of Datums. This approach conserves DSA memory allocated for storing this information.
3. Miscellaneous comment cleanups and introduced macros to simplify calculations.
