public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ashutosh Bapat <[email protected]>
To: Tatsuo Ishii <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN
Date: Fri, 13 Sep 2024 14:36:41 +0530
Message-ID: <CAExHW5vRPRLvsZYLmNGcDLkPDWDHXGSWYjox-to-OsCVFETd3w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<CAApHDvpF2_bn0WergLrxQvS8qHSKXV3-HOUjku49jCaK3DaMRQ@mail.gmail.com>
	<[email protected]>

The patch looks fine but it doesn't add a test case where Storage is
Disk or the case when the last usage fit in memory but an earlier
usage spilled to disk. Do we want to cover those. This test would be
the only one where those code paths could be tested.

On Fri, Sep 13, 2024 at 11:41 AM Tatsuo Ishii <[email protected]> wrote:
>
> David,
>
> Thank you for your review.
>
> > Yes, I think it's a good idea to move that into a helper function. If
> > you do the other node types, without that helper the could would have
> > to be repeated quite a few times. Maybe show_storage_info() can be
> > moved up with the other helper functions, say below
> > show_sortorder_options() ?
>
> Yeah, that makes sense. Looks less random.
>
> > It might be a good idea to keep the "if
> > (!es->analyze || tupstore == NULL)" checks in the calling function
> > rather than the helper too.
>
> I agree with this. This kind of check should be done in the calling
> function.
>
> > I thought about the location of the test for a while and read the
> > "This file is concerned with testing EXPLAIN in its own right."
> > comment at the top of that explain.out.  I was trying to decide if
> > testing output of a specific node type met this or not. I can't pick
> > out any other tests there which are specific to a node type, so I'm
> > unsure if this is the location for it or not. However, to put it
> > anywhere else means having to add a plpgsql function to mask out the
> > unstable parts of EXPLAIN, so maybe the location is good as it saves
> > from having to do that. I'm 50/50 on this, so I'm happy to let you
> > decide.
>
> Yeah. Maybe we should move the function to elsewhere so that it can be
> shared by other tests. However in this case it's purpose is testing an
> additional output in an explain command. I think this is not far from
> "This file is concerned with testing EXPLAIN in its own right.". So I
> would like to keep the test in explain.sql.
>
> > You could also shrink that 100 rows into a smaller number for
> > the generate_series without losing any coverage.
>
> Right. I will make the change.
>
> > Aside from that, I think the patch is good.  Thanks for working on it.
>
> Thanks. Attached is the v4 patch. I am going push it if there's no
> objection.
>
> After this, I will work on remaining node types.
>
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp



-- 
Best Wishes,
Ashutosh Bapat

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN
  In-Reply-To: <CAExHW5vRPRLvsZYLmNGcDLkPDWDHXGSWYjox-to-OsCVFETd3w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox