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
From 055af903a3dbf146d97dd3fb01a6a7d3d3bd2ae0 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@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.sgml
index 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.sql
index 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.c
index 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.dat
index 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.out
index 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
