Re: Enhancing Memory Context Statistics Reporting

Hi Daniel,

Thank you for your review.

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.

Thank you,
Rahila Syed