Re: Enhancing Memory Context Statistics Reporting

Hi Daniel,

Thank you for your comments. Please find attached v41 with all the comments
addressed.

>
> +#include "access/twophase.h"
> +#include "catalog/pg_authid_d.h"
> ...
> +#include "utils/acl.h"
> Are these actually required to be included?
>
>
Removed these.

> - MemoryContextId *entry;
> + MemoryStatsContextId *entry;
> Why is this needed? MemoryStatsContextId is identical to MemoryContextId
> and
> is too only used in mcxtfuncs.c so there is no need to expose it in
> memutils.h.
> Can't you just use MemoryContextId everywhere or am I missing something?
>
>
MemoryContextId has been renamed to MemoryStatsContextId for better
code readability. I removed the leftover MemoryContextId definition.
Also, I moved it out of memutils.h. Did the same with some other structures
and definitions which were only used in mcxtfuncs.c

> +#define CLIENT_KEY_SIZE 64
> +
> +static LWLock *client_keys_lock = NULL;
> +static int *client_keys = NULL;
> +static dshash_table *MemoryStatsDsHash = NULL;
> +static dsa_area *MemoryStatsDsaArea = NULL;
> These new additions have in some cases too generic names (client_keys etc)
> and
> they all lack comments explaining why they're needed. Maybe a leading
> block
> comment explaining they are used for process memory context reporting, and
> then
> inline comments on each with their use?
>
>
Added comments.

> +#define CLIENT_KEY_SIZE 64
> ...
> + char key[CLIENT_KEY_SIZE];
> ...
> + snprintf(key, sizeof(key), "%d", MyProcNumber);
> Given that MyProcNumber is an index into the proc array, it seems
> excessive to
> use 64 bytes to store it, can't we get away with a small stack allocation?
>

I agree. Defined it as 32 bytes as MyProcNumber is of size uint32. Kindly
let me know if you think it can be reduced further.

+ * Retreive the client key for publishing statistics and reset it to -1,
> s/Retreive/Retrieve/
>

Fixed.

> + ProcNumber procNumber = INVALID_PROC_NUMBER;
> This variable is never accessed before getting re-assigned, so this
> assignment
> in the variable definition can be removed per project style.
>
>
>
Fixed too.

> + InitMaterializedSRF(fcinfo, 0);
> Can this initialization be postponed till when we know the ResultSetInfo is
> needed? While a micro optimization, it seems we can avoid that overhead in
> case the query errors out?
>
>
Good point. Added this just before the result set is getting populated.

> + if (MemoryStatsDsHash == NULL)
> + MemoryStatsDsHash =
> GetNamedDSHash("memory_context_statistics_dshash", &memctx_dsh_params,
> &found);
> Nitpick, but there are a few oversize lines, like this one, which need to
> be
> wrapped to match project style.
>
>
I have edited this accordingly.

> + /*
> + * XXX. If the process exits without cleaning up its slot, i.e in case
> of
> + * an abrupt crash the client_keys slot won't be reset thus resulting
> in
> + * false negative and WARNING would be thrown in case another process
> with
> + * same slot index is queried for statistics.
> + */
> + if (client_keys[procNumber] == -1)
> + client_keys[procNumber] = MyProcNumber;
> + else
> + {
> + LWLockRelease(client_keys_lock);
> + ereport(WARNING,
> + errmsg("server process %d is processing previous request",
> pid));
> + PG_RETURN_NULL();
> + }
> AFAICT this mean that a failure to clean up (through a crash for example)
> can
> block a future backend from reporting which isn't entirely ideal. Is there
> anything we can do to mitigate this?
>
>
Yes, we can reset it when the client times out, as long as we verify that
the value corresponds
to our ProcNumber and not another client's request. Fixed accordingly.

>
> + bool summary = false;
> In ProcessGetMemoryContextInterrupt(), can't we just read entry->summary
> rather
> than define a local variable and assign it? We already read lots of other
> fields from entry directly so it seems more readable to be consistent.
>
>
Fixed.

>
> + /*
> + * Add the count of children contexts which are traversed
> + */
> + *num_contexts = *num_contexts + ichild;
> Isn't this really the number of children + the parent context? ichild
> starts
> at one to (AIUI) include the parent context. Also,
> MemoryContextStatsCounter
> should also make sure to set num_contexts to zero before adding to it.
>
>
Yes. Adjusted the comment to match this and set num_contexts to zero.

>
> +#define MAX_MEMORY_CONTEXT_STATS_SIZE (sizeof(MemoryStatsEntry))
> +#define MAX_MEMORY_CONTEXT_STATS_NUM
> MEMORY_CONTEXT_REPORT_MAX_PER_BACKEND / MAX_MEMORY_CONTEXT_STATS_SIZE
> I don't think MAX_MEMORY_CONTEXT_STATS_SIZE adds any value as it's only
> used
> once, on the line directly after its definition. We can just use the
> expansion
> of ((sizeof(MemoryStatsEntry)) when defining MAX_MEMORY_CONTEXT_STATS_NUM.
>
>
Fixed.

I've attached the test patch as is, I will clean it up and do further
improvements to it.

Thank you,
Rahila Syed