On 2021/03/26 12:01, Kyotaro Horiguchi wrote:
At Thu, 25 Mar 2021 23:45:17 +0900, torikoshia <torikos...@oss.nttdata.com> 
wrote in
Attached new one.

Regarding the argument max_children, isn't it better to set its default value,
e.g., 100 like MemoryContextStats() uses?

+               ereport(WARNING,
+                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                                errmsg("must be a superuser to log memory 
contexts")));

IMO this type of error, i.e., permission error, should be treated as ERROR
rather than WARNING, like pg_terminate_backend() does.

+               ereport(WARNING,
+                               (errmsg("failed to send signal: %m")));

Per message style rule, "could not send signal to process %d: %m" is better?

Thanks!

-    SELECT * FROM pg_get_backend_memory_contexts();
+    SELECT * FROM pg_get_backend_memory_contexts(0, 0);

However we can freely change the signature since it's new in 14, but I
see the (void) signature as useful.  I vaguely thought of defining
pg_get_backend_memory_contexts(void) in pg_proc.dat then defining
(int, int) as a different function in system_views.sql.  That allows
the function of the second signature to output nothing. You can make a
distinction between the two signatures by using PG_NARGS() in the C
function.

That being said, it's somewhat uneasy that two functions with the same
name returns different values.  I'd like to hear others what they feel
like about the behavior.

I think that it's confusing to merge two different features into one function.
Isn't it better to leave pg_get_backend_memory_contexts() as it is, and
make the log-memory-contexts feature as separate function, e.g.,
pg_log_backend_memory_contexts()?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply via email to