Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN

On Fri, Sep 6, 2024 at 7:21 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Fri, 6 Sept 2024 at 19:08, Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > The changes look better. A nitpick though. With their definitions
> > changed, I think it's better to change the names of the functions
> > since their purpose has changed. Right now they report the storage
> > type and size used, respectively, at the time of calling the function.
> > With this patch, they report maximum space ever used and the storage
> > corresponding to the maximum space. tuplestore_space_used() may be
> > changed to tuplestore_maxspace_used(). I am having difficulty with
> > tuplestore_storage_type_name(); tuplestore_largest_storage_type_name()
> > seems mouthful and yet not doing justice to the functionality. It
> > might be better to just have one funciton tuplestore_maxspace_used()
> > which returns both the maximum space used as well as the storage type
> > when maximum space was used.
>
> How about just removing tuplestore_storage_type_name() and
> tuplestore_space_used() and adding tuplestore_get_stats(). I did take
> some inspiration from tuplesort.c for this, so maybe we can defer back
> there for further guidance. I'm not so sure it's worth having a stats
> struct type like tuplesort.c has. All we need is a char ** and an
> int64 * output parameter to pass to the stats function. I don't think
> we need to copy the tuplesort_method_name(). It seems fine just to
> point the output parameter of the stats function at the statically
> allocated constant.

tuplestore_get_stats() similar to tuplesort_get_stats() looks fine. In
future the stats reported by this function might expand e.g. maximum
number of readers may be included in the stats. If it expands beyond
two values, we could think of a separate structure, but for now it
looks fine given its limited use. A comment explaining why we aren't
using a stats structure and some guidance on when that would be
appropriate will be better.

--
Best Wishes,
Ashutosh Bapat