From: torikoshia <torikoshia@oss.nttdata.com>
To: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: Creating a function for exposing memory usage of backend process
Date: Fri, 21 Aug 2020 23:27:06 +0900
Message-ID: <a8f3c385e9edeba4833c712270d6e6d9@oss.nttdata.com> (raw)
In-Reply-To: <CAP0=ZVJ5VZsUYLFkc-E5_JHTc1ZdemX28VgQqUXJLTywC=9hfA@mail.gmail.com>
References: <e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com>
<34517933-7e90-f644-f606-86d7132b240b@oss.nttdata.com>
<0b339b06646e2c56a3aa081699322346@oss.nttdata.com>
<f1ae4071-31b8-d7aa-db5d-2781e5612e6f@oss.nttdata.com>
<261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com>
<66ae5b88-208d-96fa-c20a-fb539b597859@oss.nttdata.com>
<20200819135545.GC19121@paquier.xyz>
<1414992.1597849297@sss.pgh.pa.us>
<20200819225343.c7rxteydi35nhrdp@alap3.anarazel.de>
<1554676.1597886946@sss.pgh.pa.us>
<20200820014301.5pr7qjugnvpovu62@alap3.anarazel.de>
<d97ad430-444d-9ec8-21dd-a1584569309b@oss.nttdata.com>
<CAP0=ZVJ5VZsUYLFkc-E5_JHTc1ZdemX28VgQqUXJLTywC=9hfA@mail.gmail.com>
Thanks for all your comments!
Thankfully it seems that this feature is regarded as not
meaningless one, I'm going to do some improvements.
On Wed, Aug 19, 2020 at 10:56 PM Michael Paquier <michael@paquier.xyz>
wrote:
> On Wed, Aug 19, 2020 at 06:12:02PM +0900, Fujii Masao wrote:
>> On 2020/08/19 17:40, torikoshia wrote:
>>> Yes, I didn't add regression tests because of the unstability of the
>>> output.
>>> I thought it would be OK since other views like pg_stat_slru and
>>> pg_shmem_allocations
>>> didn't have tests for their outputs.
>>
>> You're right.
>
> If you can make a test with something minimal and with a stable
> output, adding a test is helpful IMO, or how can you make easily sure
> that this does not get broken, particularly in the event of future
> refactorings, or even with platform-dependent behaviors?
OK. Added a regression test on sysviews.sql.
(0001-Added-a-regression-test-for-pg_backend_memory_contex.patch)
Fujii-san gave us an example, but I added more simple one considering
the simplicity of other tests on that.
On Thu, Aug 20, 2020 at 12:02 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > By the way, I was looking at the code that has been committed, and I
> > think that it is awkward to have a SQL function in mcxt.c, which is a
> > rather low-level interface. I think that this new code should be
> > moved to its own file, one suggestion for a location I have being
> > src/backend/utils/adt/mcxtfuncs.c.
>
> I agree with that,
Thanks for pointing out.
Added a patch for relocating the codes to mcxtfuncs.c.
(patches/0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch)
> On Thu, Aug 20, 2020 at 11:09 AM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> On 2020/08/20 0:01, Tom Lane wrote:
>> Given the lack of clear use-case, and the possibility (admittedly
>> not strong) that this is still somehow a security hazard, I think
>> we should revert it. If it stays, I'd like to see restrictions
>> on who can read the view.
> For example, allowing only the role with pg_monitor to see this view?
Attached a patch adding that restriction.
(0001-Restrict-the-access-to-pg_backend_memory_contexts-to.patch)
Of course, this restriction makes pg_backend_memory_contexts hard to use
when the user of the target session is not granted pg_monitor because
the
scope of this view is session local.
In this case, I imagine additional operations something like temporarily
granting pg_monitor to that user.
Thoughts?
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:download all (3 files, .tar.gz)0001-Added-a-regression-test-for-pg_backend_memory_contex.patch (text/x-diff, 1.5K) download
From 23fe541d5bd3cead787bb7c638f0086b9c2e13eb Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Fri, 21 Aug 2020 21:22:10 +0900
Subject: [PATCH] Added a regression test for pg_backend_memory_contexts.
---
src/test/regress/expected/sysviews.out | 7 +++++++
src/test/regress/sql/sysviews.sql | 3 +++
2 files changed, 10 insertions(+)
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.outindex 06c4c3e476..06e09fd10b 100644--- a/src/test/regress/expected/sysviews.out+++ b/src/test/regress/expected/sysviews.out@@ -19,6 +19,13 @@ select count(*) >= 0 as ok from pg_available_extensions;
t
(1 row)
+-- There will surely be at least one context.+select count(*) > 0 as ok from pg_backend_memory_contexts;+ ok +----+ t+(1 row)+
-- At introduction, pg_config had 23 entries; it may grow
select count(*) > 20 as ok from pg_config;
ok
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sqlindex 28e412b735..2c3b88c855 100644--- a/src/test/regress/sql/sysviews.sql+++ b/src/test/regress/sql/sysviews.sql@@ -12,6 +12,9 @@ select count(*) >= 0 as ok from pg_available_extension_versions;
select count(*) >= 0 as ok from pg_available_extensions;
+-- There will surely be at least one context.+select count(*) > 0 as ok from pg_backend_memory_contexts;+
-- At introduction, pg_config had 23 entries; it may grow
select count(*) > 20 as ok from pg_config;
--
2.18.1
0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch (text/x-diff, 10.1K) download
From 4eee73933874fbab91643e7461717ba9038d8d76 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Fri, 21 Aug 2020 19:01:38 +0900
Subject: [PATCH] Rellocated the codes for pg_backend_memory_contexts in mcxt.c
to src/backend/utils/adt/mcxtfuncs.c as they are low low-level interface.
---
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/mcxtfuncs.c | 157 ++++++++++++++++++++++++++++++
src/backend/utils/mmgr/mcxt.c | 137 --------------------------
3 files changed, 158 insertions(+), 137 deletions(-)
create mode 100644 src/backend/utils/adt/mcxtfuncs.c
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefileindex 5d2aca8cfe..54d5c37947 100644--- a/src/backend/utils/adt/Makefile+++ b/src/backend/utils/adt/Makefile@@ -57,6 +57,7 @@ OBJS = \
lockfuncs.o \
mac.o \
mac8.o \
+ mcxtfuncs.o \
misc.o \
name.o \
network.o \
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.cnew file mode 100644index 0000000000..50e1b07ff0--- /dev/null+++ b/src/backend/utils/adt/mcxtfuncs.c@@ -0,0 +1,157 @@+/*-------------------------------------------------------------------------+ *+ * mcxtfuncs.c+ * Functions to show backend memory context.+ *+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ *+ * IDENTIFICATION+ * src/backend/utils/adt/mcxtfuncs.c+ *+ *-------------------------------------------------------------------------+ */++#include "postgres.h"++#include "funcapi.h"+#include "miscadmin.h"+#include "mb/pg_wchar.h"+#include "utils/builtins.h"++/* ----------+ * The max bytes for showing identifiers of MemoryContext.+ * ----------+ */+#define MEMORY_CONTEXT_IDENT_DISPLAY_SIZE 1024++/*+ * 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));++ if (name)+ values[0] = CStringGetTextDatum(name);+ else+ nulls[0] = true;++ 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)+ values[2] = CStringGetTextDatum(parent);+ else+ nulls[2] = true;++ 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/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.cindex d9bb2499db..88c76f290c 100644--- a/src/backend/utils/mmgr/mcxt.c+++ b/src/backend/utils/mmgr/mcxt.c@@ -21,10 +21,8 @@
#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"
@@ -69,11 +67,6 @@ 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 *
@@ -1228,133 +1221,3 @@ 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));-- if (name)- values[0] = CStringGetTextDatum(name);- else- nulls[0] = true;-- 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)- values[2] = CStringGetTextDatum(parent);- else- nulls[2] = true;-- 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;-}--
2.18.1
0001-Restrict-the-access-to-pg_backend_memory_contexts-to.patch (text/x-diff, 2.0K) download
From f816ba6a94125c213b9e383db9cd9cdb6230381a Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Fri, 21 Aug 2020 09:31:29 +0900
Subject: [PATCH] Restrict the access to pg_backend_memory_contexts to members
of the pg_monitor role.
---
doc/src/sgml/catalogs.sgml | 1 +
src/backend/catalog/system_views.sql | 3 +++
2 files changed, 4 insertions(+)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgmlindex 1232b24e74..8a455d36dc 100644--- a/doc/src/sgml/catalogs.sgml+++ b/doc/src/sgml/catalogs.sgml@@ -9592,6 +9592,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
<para>
The view <structname>pg_backend_memory_contexts</structname> displays all
the memory contexts of the server process attached to the current session.
+ Access is granted to members of the <literal>pg_monitor</literal> role.
</para>
<para>
<structname>pg_backend_memory_contexts</structname> contains one row
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sqlindex ba5a23ac25..5a3a60ecea 100644--- a/src/backend/catalog/system_views.sql+++ b/src/backend/catalog/system_views.sql@@ -1482,6 +1482,8 @@ REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM public;+
--
-- We also set up some things as accessible to standard roles.
--
@@ -1490,6 +1492,7 @@ GRANT EXECUTE ON FUNCTION pg_ls_waldir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_archive_statusdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_get_backend_memory_contexts() TO pg_monitor;
GRANT pg_read_all_settings TO pg_monitor;
GRANT pg_read_all_stats TO pg_monitor;
--
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, kasahara.tatsuhito@gmail.com, masao.fujii@oss.nttdata.com, andres@anarazel.de, tgl@sss.pgh.pa.us, michael@paquier.xyz, robertmhaas@gmail.com
Subject: Re: Creating a function for exposing memory usage of backend process
In-Reply-To: <a8f3c385e9edeba4833c712270d6e6d9@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