Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rNSD9-00CCmb-1r for pgsql-hackers@arkaria.postgresql.org; Wed, 10 Jan 2024 06:37:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rNSD7-000jpg-O4 for pgsql-hackers@arkaria.postgresql.org; Wed, 10 Jan 2024 06:37:09 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rNSD7-000jpY-EO for pgsql-hackers@lists.postgresql.org; Wed, 10 Jan 2024 06:37:09 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by makus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rNSD4-000n46-8V for pgsql-hackers@lists.postgresql.org; Wed, 10 Jan 2024 06:37:08 +0000 Received: from oss.nttdata.com (localhost [127.0.0.1]) by oss.nttdata.com (Postfix) with ESMTPA id 9CCBE60EB7; Wed, 10 Jan 2024 15:37:01 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 10 Jan 2024 15:37:01 +0900 From: torikoshia To: Melih Mutlu Cc: Andres Freund , Stephen Frost , PostgreSQL Hackers Subject: Re: Parent/child context relation in pg_get_backend_memory_contexts() In-Reply-To: References:

<20231012162309.4wmc2nb5tkt6pzml@awork3.anarazel.de> <2f34c14847fc03752f0bca7d97aeae27@oss.nttdata.com> User-Agent: Roundcube Webmail/1.4.11 Message-ID: X-Sender: torikoshia@oss.nttdata.com List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On 2024-01-03 20:40, Melih Mutlu wrote: > Hi, > > Thanks for reviewing. Please find the updated patch attached. > > torikoshia , 4 Ara 2023 Pzt, 07:43 > tarihinde şunu yazdı: > >> I reviewed v3 patch and here are some minor comments: >> >>> + >>> + >> role="column_definition"> >>> + path int4 >> >> Should 'int4' be 'int4[]'? >> Other system catalog columns such as pg_groups.grolist distinguish >> whther the type is a array or not. > > Right! Done. > >>> + Path to reach the current context from TopMemoryContext. >>> Context ids in >>> + this list represents all parents of the current context. >> This >>> can be >>> + used to build the parent and child relation. >> >> It seems last "." is not necessary considering other explanations >> for >> each field end without it. > > Done. > >> + const char *parent, int level, int >> *context_id, >> + List *path, Size >> *total_bytes_inc_chidlren) >> >> 'chidlren' -> 'children' > > Done. > >> + elog(LOG, "pg_get_backend_memory_contexts called"); >> >> Is this message necessary? > > I guess I added this line for debugging and then forgot to remove. Now > removed. > >> There was warning when applying the patch: >> >> % git apply >> > ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch >> >> > ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:282: >> >> trailing whitespace. >> select count(*) > 0 >> >> > ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:283: >> >> trailing whitespace. >> from contexts >> warning: 2 lines add whitespace errors. > > Fixed. > > Thanks,-- > > Melih Mutlu > Microsoft Thanks for updating the patch. > + > + role="column_definition"> > + context_id int4 > + > + > + Current context id. Note that the context id is a temporary id > and may > + change in each invocation > + > + > + > + > + role="column_definition"> > + path int4[] > + > + > + Path to reach the current context from TopMemoryContext. > Context ids in > + this list represents all parents of the current context. This > can be > + used to build the parent and child relation > + > + > + > + > + role="column_definition"> > + total_bytes_including_children > int8 > + > + > + Total bytes allocated for this memory context including its > children > + > + These columns are currently added to the bottom of the table, but it may be better to put semantically similar items close together and change the insertion position with reference to other system views. For example, - In pg_group and pg_user, 'id' is placed on the line following 'name', so 'context_id' be placed on the line following 'name' - 'path' is similar with 'parent' and 'level' in that these are information about the location of the context, 'path' be placed to next to them. If we do this, orders of columns in the system view should be the same, I think. > + ListCell *lc; > + > + length = list_length(path); > + datum_array = (Datum *) palloc(length * sizeof(Datum)); > + length = 0; > + foreach(lc, path) > + { > + datum_array[length++] = Int32GetDatum(lfirst_int(lc)); > + } 14dd0f27d have introduced new macro foreach_int. It seems to be able to make the code a bit simpler and the commit log says this macro is primarily intended for use in new code. For example: | int id; | | length = list_length(path); | datum_array = (Datum *) palloc(length * sizeof(Datum)); | length = 0; | foreach_int(id, path) | { | datum_array[length++] = Int32GetDatum(id); | } -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation