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: Wed, 19 Aug 2020 09:43:50 +0900
Message-ID: <0b339b06646e2c56a3aa081699322346@oss.nttdata.com> (raw)
In-Reply-To: <34517933-7e90-f644-f606-86d7132b240b@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>
<e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com>
<34517933-7e90-f644-f606-86d7132b240b@oss.nttdata.com>
On 2020-08-18 22:54, Fujii Masao wrote:
> On 2020/08/18 18:41, torikoshia wrote:
>> 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.
>
> Isn't it better to add AssertArg(MemoryContextIsValid(context)),
> instead?
Thanks, that's better.
>>
>> | 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?
>
> I was thinking that we can simplify the code as follows.
> That is, we can just pass "name" as the argument of
> PutMemoryContextsStatsTupleStore()
> since "name" indicates context->name or ident (if name is "dynahash").
>
> 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);
> + child, name, level + 1);
I got it, thanks for the clarification!
Attached a revised patch.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:0009-Adding-a-function-exposing-memory-usage-of-local-backend.patch (text/x-diff, 12.2K) download
From 055af903a3dbf146d97dd3fb01a6a7d3d3bd2ae0 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 19 Aug 2020 09:20:01 +0900
Subject: [PATCH] This patch implements a new SQL-callable function
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 | 135 +++++++++++++++++++++++++++
src/include/catalog/pg_proc.dat | 9 ++
src/test/regress/expected/rules.out | 10 ++
5 files changed, 279 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..c5fb5d5dab 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,130 @@ 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;++ AssertArg(MemoryContextIsValid(context));++ 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)+ {+ PutMemoryContextsStatsTupleStore(tupstore, tupdesc,+ child, name, 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: <0b339b06646e2c56a3aa081699322346@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