From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Subject: Re: Creating a function for exposing memory usage of backend process
Date: Tue, 18 Aug 2020 18:41:47 +0900
Message-ID: <e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com> (raw)
In-Reply-To: <083781e2-339e-9844-a02c-53a0ee3beb82@oss.nttdata.com>
References: <805313b5f7dc2e6c19b209ff930a7a61@oss.nttdata.com>
<a9d68488-8343-5d70-4612-311d597f9962@oss.nttdata.com>
<cd0e961fd42e5708fdea70f7420bf214@oss.nttdata.com>
<9ee242b1-9e70-18f5-0033-6a086e71164d@oss.nttdata.com>
<3548926512ddfaedfb5eaf967bf2f196@oss.nttdata.com>
<20200708170348.rsqry4b4k5mrsu7y@alap3.anarazel.de>
<24ea05d7bbdb652feef50caee6fcfbf8@oss.nttdata.com>
<CAP0=ZVJ2JPS41CffcG-E2CnBkfOjFRa4YoKB=A8vbWGkjEtA1w@mail.gmail.com>
<e1b81c88dc750ecd2ef03924af751567@oss.nttdata.com>
<CA+TgmoZU6Tr2mEY2Cjf5-Cce5NF6B0eknCge1v7Uk3D5H2njOA@mail.gmail.com>
<20200808014409.GA1600@paquier.xyz>
<66c9d45297ad820cd092b125aa91ce2c@oss.nttdata.com>
<98e53274-253f-6bd1-b542-489ecbda8753@oss.nttdata.com>
<083781e2-339e-9844-a02c-53a0ee3beb82@oss.nttdata.com>
On 2020-08-17 21:19, Fujii Masao wrote:
> On 2020/08/17 21:14, Fujii Masao wrote:
>>> On 2020-08-07 16:38, Kasahara Tatsuhito wrote:
>>>> The following review has been posted through the commitfest
>>>> application:
>>>> make installcheck-world: tested, passed
>>>> Implements feature: tested, passed
>>>> Spec compliant: not tested
>>>> Documentation: tested, passed
>>>>
>>>> I tested the latest
>>>> patch(0007-Adding-a-function-exposing-memory-usage-of-local-backend.patch)
>>>> with the latest PG-version
>>>> (199cec9779504c08aaa8159c6308283156547409)
>>>> and test was passed.
>>>> It looks good to me.
>>>>
>>>> The new status of this patch is: Ready for Committer
>>>
>>> Thanks for your testing!
>>
>> Thanks for updating the patch! Here are the review comments.
Thanks for reviewing!
>>
>> + <row>
>> + <entry><link
>> linkend="view-pg-backend-memory-contexts"><structname>pg_backend_memory_contexts</structname></link></entry>
>> + <entry>backend memory contexts</entry>
>> + </row>
>>
>> The above is located just after pg_matviews entry. But it should be
>> located
>> just after pg_available_extension_versions entry. Because the rows in
>> the table
>> "System Views" should be located in alphabetical order.
>>
>>
>> + <sect1 id="view-pg-backend-memory-contexts">
>> + <title><structname>pg_backend_memory_contexts</structname></title>
>>
>> Same as above.
Modified both.
>>
>>
>> + The view <structname>pg_backend_memory_contexts</structname>
>> displays all
>> + the local backend memory contexts.
>>
>> This description seems a bit confusing because maybe we can interpret
>> this
>> as "... displays the memory contexts of all the local backends"
>> wrongly. Thought?
>> What about the following description, instead?
>> The view <structname>pg_backend_memory_contexts</structname>
>> displays all
>> the memory contexts of the server process attached to the current
>> session.
Thanks! it seems better.
>> + const char *name = context->name;
>> + const char *ident = context->ident;
>> +
>> + if (context == NULL)
>> + return;
>>
>> The above check "context == NULL" is useless? If "context" is actually
>> NULL,
>> "context->name" would cause segmentation fault, so ISTM that the check
>> will never be performed.
>>
>> If "context" can be NULL, the check should be performed before
>> accessing
>> to "contect". OTOH, if "context" must not be NULL per the
>> specification of
>> PutMemoryContextStatsTupleStore(), assertion test checking
>> "context != NULL" should be used here, instead?
Yeah, "context" cannot be NULL because "context" must be
TopMemoryContext
or it is already checked as not NULL as follows(child != NULL).
I added the assertion check.
| for (child = context->firstchild; child != NULL; child =
child->nextchild)
| {
| ...
| PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
| child,
parentname, level + 1);
| }
> Here is another comment.
>
> + if (parent == NULL)
> + nulls[2] = true;
> + else
> + /*
> + * We labeled dynahash contexts with just the hash table
> name.
> + * To make it possible to identify its parent, we also
> display
> + * parent's ident here.
> + */
> + if (parent->ident && strcmp(parent->name, "dynahash") ==
> 0)
> + values[2] = CStringGetTextDatum(parent->ident);
> + else
> + values[2] = CStringGetTextDatum(parent->name);
>
> PutMemoryContextsStatsTupleStore() doesn't need "parent" memory
> context,
> but uses only the name of "parent" memory context. So isn't it better
> to use
> "const char *parent" instead of "MemoryContext parent", as the argument
> of
> the function? If we do that, we can simplify the above code.
Thanks, the attached patch adopted the advice.
However, since PutMemoryContextsStatsTupleStore() used not only the name
but also the ident of the "parent", I could not help but adding similar
codes before calling the function.
The total amount of codes and complexity seem not to change so much.
Any thoughts? Am I misunderstanding something?
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:0008-Adding-a-function-exposing-memory-usage-of-local-backend.patch (text/x-diff, 12.5K) download
From 055af903a3dbf146d97dd3fb01a6a7d3d3bd2ae0 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 18 Aug 2020 18:17:42 +0900
Subject: [PATCH] Add a function exposing memory usage of local backend.
pg_get_backend_memory_contexts which exposes memory usage of the local
backend. It also adds a new view pg_backend_memory_contexts for exposing
local backend memory contexts.
---
doc/src/sgml/catalogs.sgml | 122 ++++++++++++++++++++++
src/backend/catalog/system_views.sql | 3 +
src/backend/utils/mmgr/mcxt.c | 147 +++++++++++++++++++++++++++
src/include/catalog/pg_proc.dat | 9 ++
src/test/regress/expected/rules.out | 10 ++
5 files changed, 291 insertions(+)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgmlindex fc329c5cff..1232b24e74 100644--- a/doc/src/sgml/catalogs.sgml+++ b/doc/src/sgml/catalogs.sgml@@ -9226,6 +9226,11 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
<entry>available versions of extensions</entry>
</row>
+ <row>+ <entry><link linkend="view-pg-backend-memory-contexts"><structname>pg_backend_memory_contexts</structname></link></entry>+ <entry>backend memory contexts</entry>+ </row>+
<row>
<entry><link linkend="view-pg-config"><structname>pg_config</structname></link></entry>
<entry>compile-time configuration parameters</entry>
@@ -9577,6 +9582,123 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</para>
</sect1>
+ <sect1 id="view-pg-backend-memory-contexts">+ <title><structname>pg_backend_memory_contexts</structname></title>++ <indexterm zone="view-pg-backend-memory-contexts">+ <primary>pg_backend_memory_contexts</primary>+ </indexterm>++ <para>+ The view <structname>pg_backend_memory_contexts</structname> displays all+ the memory contexts of the server process attached to the current session.+ </para>+ <para>+ <structname>pg_backend_memory_contexts</structname> contains one row+ for each memory context.+ </para>++ <table>+ <title><structname>pg_backend_memory_contexts</structname> Columns</title>+ <tgroup cols="1">+ <thead>+ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ Column Type+ </para>+ <para>+ Description+ </para></entry>+ </row>+ </thead>++ <tbody>+ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>name</structfield> <type>text</type>+ </para>+ <para>+ Name of the memory context+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>ident</structfield> <type>text</type>+ </para>+ <para>+ Identification information of the memory context. This field is truncated at 1024 bytes+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>parent</structfield> <type>text</type>+ </para>+ <para>+ Name of the parent of this memory context+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>level</structfield> <type>int4</type>+ </para>+ <para>+ Distance from TopMemoryContext in context tree+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>total_bytes</structfield> <type>int8</type>+ </para>+ <para>+ Total bytes allocated for this memory context+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>total_nblocks</structfield> <type>int8</type>+ </para>+ <para>+ Total number of blocks allocated for this memory context+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>free_bytes</structfield> <type>int8</type>+ </para>+ <para>+ Free space in bytes+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>free_chunks</structfield> <type>int8</type>+ </para>+ <para>+ Total number of free chunks+ </para></entry>+ </row>++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>used_bytes</structfield> <type>int8</type>+ </para>+ <para>+ Used space in bytes+ </para></entry>+ </row>+ </tbody>+ </tgroup>+ </table>++ </sect1>+
<sect1 id="view-pg-config">
<title><structname>pg_config</structname></title>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sqlindex 8625cbeab6..ba5a23ac25 100644--- a/src/backend/catalog/system_views.sql+++ b/src/backend/catalog/system_views.sql@@ -554,6 +554,9 @@ CREATE VIEW pg_shmem_allocations AS
REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
+CREATE VIEW pg_backend_memory_contexts AS+ SELECT * FROM pg_get_backend_memory_contexts();+
-- Statistics views
CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.cindex abda22fa57..d4fc2aed93 100644--- a/src/backend/utils/mmgr/mcxt.c+++ b/src/backend/utils/mmgr/mcxt.c@@ -21,8 +21,10 @@
#include "postgres.h"
+#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+#include "utils/builtins.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -67,6 +69,12 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
#define AssertNotInCriticalSection(context) \
Assert(CritSectionCount == 0 || (context)->allowInCritSection)
+/* ----------+ * The max bytes for showing identifiers of MemoryContext.+ * ----------+ */+#define MEMORY_CONTEXT_IDENT_DISPLAY_SIZE 1024+
/*****************************************************************************
* EXPORTED ROUTINES *
*****************************************************************************/
@@ -1220,3 +1228,142 @@ pchomp(const char *in)
n--;
return pnstrdup(in, n);
}
++/*+ * PutMemoryContextsStatsTupleStore+ * One recursion level for pg_get_backend_memory_contexts.+ */+static void+PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,+ TupleDesc tupdesc, MemoryContext context,+ const char *parent, int level)+{+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS 9++ Datum values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];+ bool nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];+ MemoryContextCounters stat;+ MemoryContext child;+ const char *name;+ const char *ident;++ Assert(context != NULL);++ name = context->name;+ ident = context->ident;++ /*+ * To be consistent with logging output, we label dynahash contexts+ * with just the hash table name as with MemoryContextStatsPrint().+ */+ if (ident && strcmp(name, "dynahash") == 0)+ {+ name = ident;+ ident = NULL;+ }++ /* Examine the context itself */+ memset(&stat, 0, sizeof(stat));+ (*context->methods->stats) (context, NULL, (void *) &level, &stat);++ memset(values, 0, sizeof(values));+ memset(nulls, 0, sizeof(nulls));++ values[0] = CStringGetTextDatum(name);++ if (ident)+ {+ int idlen = strlen(ident);+ char clipped_ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];++ /*+ * Some identifiers such as SQL query string can be very long,+ * truncate oversize identifiers.+ */+ if (idlen >= MEMORY_CONTEXT_IDENT_DISPLAY_SIZE)+ idlen = pg_mbcliplen(ident, idlen, MEMORY_CONTEXT_IDENT_DISPLAY_SIZE - 1);++ memcpy(clipped_ident, ident, idlen);+ clipped_ident[idlen] = '\0';+ values[1] = CStringGetTextDatum(clipped_ident);+ }+ else+ nulls[1] = true;++ if (parent == NULL)+ nulls[2] = true;+ else+ values[2] = CStringGetTextDatum(parent);++ values[3] = Int32GetDatum(level);+ values[4] = Int64GetDatum(stat.totalspace);+ values[5] = Int64GetDatum(stat.nblocks);+ values[6] = Int64GetDatum(stat.freespace);+ values[7] = Int64GetDatum(stat.freechunks);+ values[8] = Int64GetDatum(stat.totalspace - stat.freespace);+ tuplestore_putvalues(tupstore, tupdesc, values, nulls);++ for (child = context->firstchild; child != NULL; child = child->nextchild)+ {+ const char *parentname;++ /*+ * We labeled dynahash contexts with just the hash table name.+ * To make it possible to identify its parent, we also use+ * the hash table as its context name.+ */+ if (context->ident && strcmp(context->name, "dynahash") == 0)+ parentname = context->ident;+ else+ parentname = context->name;++ PutMemoryContextsStatsTupleStore(tupstore, tupdesc,+ child, parentname, level + 1);+ }+}++/*+ * pg_get_backend_memory_contexts+ * SQL SRF showing backend memory context.+ */+Datum+pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)+{+ ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;+ TupleDesc tupdesc;+ Tuplestorestate *tupstore;+ MemoryContext per_query_ctx;+ MemoryContext oldcontext;++ /* check to see if caller supports us returning a tuplestore */+ if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("set-valued function called in context that cannot accept a set")));+ if (!(rsinfo->allowedModes & SFRM_Materialize))+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("materialize mode required, but it is not allowed in this context")));++ /* Build a tuple descriptor for our result type */+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)+ elog(ERROR, "return type must be a row type");++ per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;+ oldcontext = MemoryContextSwitchTo(per_query_ctx);++ tupstore = tuplestore_begin_heap(true, false, work_mem);+ rsinfo->returnMode = SFRM_Materialize;+ rsinfo->setResult = tupstore;+ rsinfo->setDesc = tupdesc;++ MemoryContextSwitchTo(oldcontext);++ PutMemoryContextsStatsTupleStore(tupstore, tupdesc,+ TopMemoryContext, NULL, 0);++ /* clean up and return the tuplestore */+ tuplestore_donestoring(tupstore);++ return (Datum) 0;+}diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex 082a11f270..27989971db 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -7807,6 +7807,15 @@
proargnames => '{name,off,size,allocated_size}',
prosrc => 'pg_get_shmem_allocations' },
+# memory context of local backend+{ oid => '2282', descr => 'information about all memory contexts of local backend',+ proname => 'pg_get_backend_memory_contexts', prorows => '100', proretset => 't',+ provolatile => 'v', proparallel => 'r', prorettype => 'record', proargtypes => '',+ proallargtypes => '{text,text,text,int4,int8,int8,int8,int8,int8}',+ proargmodes => '{o,o,o,o,o,o,o,o,o}',+ proargnames => '{name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',+ prosrc => 'pg_get_backend_memory_contexts' },+
# non-persistent series generator
{ oid => '1066', descr => 'non-persistent series generator',
proname => 'generate_series', prorows => '1000',
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.outindex 601734a6f1..2a18dc423e 100644--- a/src/test/regress/expected/rules.out+++ b/src/test/regress/expected/rules.out@@ -1324,6 +1324,16 @@ pg_available_extensions| SELECT e.name,
e.comment
FROM (pg_available_extensions() e(name, default_version, comment)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
+pg_backend_memory_contexts| SELECT pg_get_backend_memory_contexts.name,+ pg_get_backend_memory_contexts.ident,+ pg_get_backend_memory_contexts.parent,+ pg_get_backend_memory_contexts.level,+ pg_get_backend_memory_contexts.total_bytes,+ pg_get_backend_memory_contexts.total_nblocks,+ pg_get_backend_memory_contexts.free_bytes,+ pg_get_backend_memory_contexts.free_chunks,+ pg_get_backend_memory_contexts.used_bytes+ FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
pg_config| SELECT pg_config.name,
pg_config.setting
FROM pg_config() pg_config(name, setting);
--
2.18.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: torikoshia@oss.nttdata.com, masao.fujii@oss.nttdata.com, robertmhaas@gmail.com, kasahara.tatsuhito@gmail.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: Creating a function for exposing memory usage of backend process
In-Reply-To: <e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox