Re: Enhancing Memory Context Statistics Reporting

Hi,

Thank you for your feedback. Please find a few responses inline.

I am working on incorporating all the feedback in the patch and will share
it in a follow-up email.



> Some things I notice reading through:
>
> +       LWLockAcquire(client_keys_lock, LW_SHARED);
>
> LWLocks normally use NamesLikeThis not names_like_this and are
> generally created by just listing them in lwlocklist.h.
>
>
I will fix it, accordingly.


> +       else
> +       {
> +               clientProcNumber = client_keys[MyProcNumber];
> +               client_keys[MyProcNumber] = -1;
> +               LWLockRelease(client_keys_lock);
> +       }
>
> The "else" is not really necessary here because the "if" portion ends
> with "return".
>

Fixed this in the attached patch.


>
> +       memstats_ctx = AllocSetContextCreate((MemoryContext) NULL,
> +
>           "publish_memory_context_statistics",
> +
>           ALLOCSET_SMALL_SIZES);
>
> The comments do a good job justifying this, but as far as I know it
> would be the only instance of this pattern in the entire source tree.
> Are we really sure we want to deviate from the idea of having the
> memory context tree be a tree? And is it really so bad if the memory
> used to report memory contexts is included in the output?
>
>
This was implemented in response to a review suggestion [1].
If required, it can be updated to create one under CurrentMemoryContext.

+               MemoryStatsDsaArea =
> GetNamedDSA("memory_context_statistics_dsa",
> +
>           &found);
> ...
> +               MemoryStatsDsHash =
> GetNamedDSHash("memory_context_statistics_dshash",
> +
>             &memctx_dsh_params, &found);
> ...
> +       entry = dshash_find_or_insert(MemoryStatsDsHash, key, &found);
>
> Like LWLockAcquire, all of this code supposes that there's a
> transaction available to manage resource acquisition and release and
> to clean up after errors. I doubt that any of this is safe without a
> transaction.
>

Starting a transaction from CFI works for a client backend but
not for auxiliary processes. When I try to execute StartTransaction()
from a checkpointer process, the following assertion fails,
Assert(MyProc->vxid.procNumber == vxid.procNumber);
This is because MyProc->vxid.procNumber is not set for auxiliary
processes.

Please find attached a rebased patch, that fixes a build error reported
on the commit-fest app.

[1]. PostgreSQL: Re: Prevent an error on attaching/creating a DSM/DSA from
an interrupt handler.
<https://www.postgresql.org/message-id/594293.1747708165%40sss.pgh.pa.us>

Thank you,
Rahila Syed