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