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

Thanks for the rebase, a few mostly superficial comments from a first
read-through.

The documentation refers to attributes in the return row but the format of that
row isn't displayed which makes following along hard.  I think we should
include a table or a programlisting showing the return data before this
paragraph.

+const char *
+AssignContextType(NodeTag type)
This function doesn't actually assign anything so the name is a bit misleading,
it would be better with ContextTypeToString or something similar.

+ * By default, only superusers or users with PG_READ_ALL_STATS are allowed to
This sentence is really long and should probably be broken up.

+ * The shared memory buffer has a limited size - it the process has too many
s/it/if/

+ * If the publishing backend does not respond before the condition variable
+ * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry for max_tries
+ * number of times, which is defined by user, before giving up and
+ * returning previously published statistics, if any.
This comment should mention what happens if the process gives up and there is
no previously published stats.

+   int         i;
    ...
+   for (i = 0; i < memCtxState[procNumber].total_stats; i++)
This can be rewritten as "for (int i = 0; .." since we allow C99.

+    * process running and consuming lots of memory, that it might end on its
+    * own first and its memory contexts are not logged is not a problem.
This comment is copy/pasted from pg_log_backend_memory_contexts and while it
mostly still apply it should at least be rewritten to not refer to logging as
this function doesn't do that.

+   ereport(WARNING,
+           (errmsg("PID %d is not a PostgreSQL server process",
No need to add the extra parenthesis around errmsg anymore, so I think new code
should omit those.

+  errhint("Use pg_backend_memory_contexts view instead")));
Super nitpick, but errhints should be complete sentences ending with a period.

+    * statitics have previously been published by the backend. In which case,
s/statitics/statistics/

+    * statitics have previously been published by the backend. In which case,
+    * check if statistics are not older than curr_timestamp, if they are wait
I think the sentence around the time check is needlessly confusing, could it be
rewritten into something like:
    "A valid DSA pointer isn't proof that statistics are available, it can be
    valid due to previously published stats.  Check if the stats are updated by
    comparing the timestamp, if the stats are newer than our previously
    recorded timestamp from before sending the procsignal they must by
    definition be updated."

+   /* Assert for dsa_handle to be valid */
Was this intended to be turned into an Assert call? Else it seems better to remove. 

+   if (print_location != PRINT_STATS_NONE)
This warrants a comment stating why it makes sense. 

+    * Do not print the statistics if print_to_stderr is PRINT_STATS_NONE,
s/print_to_stderr/print_location/.  Also, do we really need print_to_stderr in
this function at all?  It seems a bit awkward to combine a boolean and a
paramter for a tri-state value when the parameter holds the tri_state already.
For readability I think just checking print_location will be better since the
value will be clear, where print_to_stderr=false is less clear in a tri-state
scenario.

+ * its ancestors to a list, inorder to compute a path.
s/inorder/in order/

+   elog(LOG, "hash table corrupted, can't construct path value");
+   break;
This will return either a NIL list or a partial path, but PublishMemoryContext
doesn't really take into consideration that it might be so.  Is this really
benign to the point that we can blindly go on?  Also, elog(LOG..) is mostly for
tracing or debugging as elog() isn't intended for user facing errors.

+static void
+compute_num_of_contexts(List *contexts, HTAB *context_id_lookup,
+                       int *stats_count, bool get_summary)
This function does a lot than compute the number of contexts so the name seems
a bit misleading.  Perhaps a rename to compute_contexts() or something similar?

+   memctx_info[curr_id].name = dsa_allocate0(area,
+                                             strlen(clipped_ident) + 1);
These calls can use idlen instead of more strlen() calls no?  While there is no
performance benefit, it would increase readability IMHO if the code first
calculates a value, and then use it.

+   strncpy(name,
+           clipped_ident, strlen(clipped_ident));
Since clipped_ident has been nul terminated earlier there is no need to use
strncpy, we can instead use strlcpy and take the target buffer size into
consideration rather than the input string length.

    PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
+   PROCSIG_GET_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
This comment should be different from the LOG_MEMORY_xx one.

+#define MEM_CONTEXT_SHMEM_STATS_SIZE   30
+#define MAX_TYPE_STRING_LENGTH 64
These are unused, from an earlier version of the patch perhaps?

+ * Singe DSA area is created and used by all the processes,
s/Singe/Since/

+typedef struct MemoryContextBackendState
This is only used in mcxtfuncs.c and can be moved there rather than being
exported in the header.

+}          MemoryContextId;
This lacks an entry in the typedefs.list file.