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 <mich...@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 <t...@sss.pgh.pa.us> wrote:
Michael Paquier <mich...@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.fu...@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
From 23fe541d5bd3cead787bb7c638f0086b9c2e13eb Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@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.out
index 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.sql
index 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
From 4eee73933874fbab91643e7461717ba9038d8d76 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@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/Makefile
index 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.c
new file mode 100644
index 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.c
index 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
From f816ba6a94125c213b9e383db9cd9cdb6230381a Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@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.sgml
index 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.sql
index 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
