On 2020/08/19 17:40, torikoshia wrote:
On 2020-08-19 15:48, Fujii Masao wrote:
On 2020/08/19 9:43, torikoshia wrote:
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.
Thanks for updating the patch! I pushed it.
Thanks a lot!
BTW, I guess that you didn't add the regression test for this view because
the contents of the view are not stable. Right? But isn't it better to just
add the "stable" test like
SELECT name, ident, parent, level, total_bytes >= free_bytes FROM
pg_backend_memory_contexts WHERE level = 0;
rather than adding nothing?
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.
I don't have strong objections for adding tests like you proposed, but I'm not
sure
whether there are special reasons to add tests compared with these existing
views.
Ok, understood. So I'd withdraw my suggestion.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
