Re: Enhancing Memory Context Statistics Reporting - Mailing list pgsql-hackers

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.

+       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".

+       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?

+               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.