On 2020-08-24 13:13, Fujii Masao wrote:
On 2020/08/24 13:01, torikoshia wrote:
On 2020-08-22 21:18, Michael Paquier wrote:
Thanks for reviewing!
On Fri, Aug 21, 2020 at 11:27:06PM +0900, torikoshia wrote:
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.
What you have sent in 0001 looks fine to me. A small test is much
better than nothing.
+1
But as I proposed upthread, what about a bit complicated test as
follows,
e.g., to confirm that the internal logic for level works expectedly?
SELECT name, ident, parent, level, total_bytes >= free_bytes FROM
pg_backend_memory_contexts WHERE level = 0;
OK!
Attached an updated patch.
Added a patch for relocating the codes to mcxtfuncs.c.
(patches/0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch)
Thanks for the patch! Looks good to me.
Barring any objection, I will commit this patch at first.
The same code is moved around line-by-line.
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.
Hmm. I am not completely sure either that pg_monitor is the best fit
here, because this view provides information about a bunch of
internal
structures. Something that could easily be done though is to revoke
the access from public, and then users could just set up GRANT
permissions post-initdb, with pg_monitor as one possible choice.
This
is the safest path by default, and this stuff is of a caliber similar
to pg_shmem_allocations in terms of internal contents.
I think this is a better way than what I did in
0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch.
You mean
0001-Restrict-the-access-to-pg_backend_memory_contexts-to.patch?
Oops, I meant
0001-Restrict-the-access-to-pg_backend_memory_contexts-to.patch.
Attached a patch.
Thanks for updating the patch! This also looks good to me.
It seems to me that you are missing one "REVOKE ALL on
pg_backend_memory_contexts FROM PUBLIC" in patch 0003.
By the way, if that was just for me, I would remove used_bytes, which
is just a computation from the total and free numbers. I'll defer
that point to Fujii-san.
Yeah, I was just thinking that displaying also used_bytes was useful,
but this might be inconsistent with the other views' ways.
Regards,
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
From 335b9eb0c60a7f12debd4c45d435888109b2bfcf Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Mon, 24 Aug 2020 21:28:20 +0900
Subject: [PATCH] Added a regression test for pg_backend_memory_contexts.
---
src/test/regress/expected/sysviews.out | 9 +++++++++
src/test/regress/sql/sysviews.sql | 5 +++++
2 files changed, 14 insertions(+)
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 06c4c3e476..1cffc3349d 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -19,6 +19,15 @@ select count(*) >= 0 as ok from pg_available_extensions;
t
(1 row)
+-- The entire output of pg_backend_memory_contexts is not stable,
+-- we test only the existance and basic condition of TopMemoryContext.
+select name, ident, parent, level, total_bytes >= free_bytes
+ from pg_backend_memory_contexts where level = 0;
+ name | ident | parent | level | ?column?
+------------------+-------+--------+-------+----------
+ TopMemoryContext | | | 0 | 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..ac4a0e1cbb 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -12,6 +12,11 @@ select count(*) >= 0 as ok from pg_available_extension_versions;
select count(*) >= 0 as ok from pg_available_extensions;
+-- The entire output of pg_backend_memory_contexts is not stable,
+-- we test only the existance and basic condition of TopMemoryContext.
+select name, ident, parent, level, total_bytes >= free_bytes
+ from pg_backend_memory_contexts where level = 0;
+
-- At introduction, pg_config had 23 entries; it may grow
select count(*) > 20 as ok from pg_config;
--
2.18.1
