Date: Tue, 09 Jul 2024 11:44:12 +0900 (JST)
Message-Id: <20240709.114412.199303920149670049.ishii@postgresql.org>
To: dgrowleyml@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAApHDvpQ3Duc7+c5=-7b60Mq-B6A=eGJT=gEb2V=J4c15-KLdw@mail.gmail.com>
References: <20240706.202254.89740021795421286.ishii@postgresql.org>
	<CAApHDvpQ3Duc7+c5=-7b60Mq-B6A=eGJT=gEb2V=J4c15-KLdw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240709.114412.199303920149670049.ishii%40postgresql.org>
Precedence: bulk

> On Sat, 6 Jul 2024 at 23:23, Tatsuo Ishii <ishii@postgresql.org> wrote:
>> So I wanted to Add memory/disk usage for WindowAgg. Patch attached.
> 
> Thanks for working on that.

Thank you for the infrastructure you created in tuplestore.c and explain.c.

BTW, it seems these executor nodes (other than Materialize and Window
Aggregate node) use tuplestore for their own purpose.

CTE Scan
Recursive Union
Table Function Scan

I have already implemented that for CTE Scan. Do you think other two
nodes are worth to add the information? I think for consistency sake,
it will better to add the info Recursive Union and Table Function
Scan.

>> Since WindowAgg node could create multiple tuplestore for each Window
>> partition, we need to track each tuplestore storage usage so that the
>> maximum storage usage is determined. For this purpose I added new
>> fields to the WindowAggState.
> 
> I'd recently been looking at the code that recreates the tuplestore
> for each partition and thought we could do a bit better. In [1], I
> proposed a patch to make this better.
> 
> If you based your patch on [1], maybe a better way of doing this is
> having tuplestore.c track the maximum space used on disk in an extra
> field which is updated with tuplestore_clear().  It's probably ok to
> update a new field called maxDiskSpace in tuplestore_clear() if
> state->status != TSS_INMEM. If the tuplestore went to disk then an
> extra call to BufFileSize() isn't going to be noticeable, even in
> cases where we only just went over work_mem. You could then adjust
> tuplestore_space_used() to look at maxDiskSpace and return that value
> if it's larger than BufFileSize(state->myfile) and state->maxSpace.
> You could check if maxDiskSpace == 0 to determine if the tuplestore
> has ever gone to disk. tuplestore_storage_type_name() would also need
> to check maxDiskSpace and return "Disk" if that's non-zero.

Thank you for the suggestion. Yes, I noticed [1] and once it is
committed, I will start to study tuplestore.c in this direction.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp