Re: Summary Sort workers Stats in EXPLAIN ANALYZE

[Ibrar Ahmed]

On Mon, Mar 28, 2022 at 2:55 PM Jian Guo <gj...@vmware.com> wrote:

>
> I have updated the patch addressing the review comments, but I didn't
> moved this code block into VERBOSE mode, to keep consistency with `
> show_incremental_sort_info`:
>
>
> https://github.com/postgres/postgres/blob/d4ba8b51c76300f06cc23f4d8a41d9f7210c4866/src/backend/commands/explain.c#L2890
>
> Please review, thanks.
>
> ------------------------------
> *From:* Julien Rouhaud <rjuju...@gmail.com>
> *Sent:* Friday, March 25, 2022 17:04
> *To:* Jian Guo <gj...@vmware.com>
> *Cc:* pgsql-hackers@lists.postgresql.org <
> pgsql-hackers@lists.postgresql.org>; Zhenghua Lyu <z...@vmware.com>
> *Subject:* Re: Summary Sort workers Stats in EXPLAIN ANALYZE
>
> ⚠ External Email
>
> Hi,
>
> On Thu, Mar 24, 2022 at 07:50:11AM +0000, Jian Guo wrote:
> > For a simple demo, with this explain statement：
> >
> > -- Test sort stats summary
> > set force_parallel_mode=on;
> > select explain_filter('explain (analyze, summary off, timing off, costs
> off, format json) select * from tenk1 order by unique1');
> >
> > Before this patch, we got plan like this:
> >
> >
> >            "Node Type": "Sort",                +
> >            "Parent Relationship": "Outer",     +
> >            "Parallel Aware": false,            +
> >            "Async Capable": false,             +
> >            "Actual Rows": 10000,               +
> >            "Actual Loops": 1,                  +
> >            "Sort Key": ["unique1"],            +
> >            "Workers": [                        +
> >              {                                 +
> >                "Worker Number": 0,             +
> >                "Sort Method": "external merge",+
> >                "Sort Space Used": 2496,        +
> >                "Sort Space Type": "Disk"       +
> >              }                                 +
> >            ],                                  +
>
> > After this patch, the effected plan is this:
> >
> >            "Node Type": "Sort",               +
> >            "Parent Relationship": "Outer",    +
> >            "Parallel Aware": false,           +
> >            "Async Capable": false,            +
> >            "Actual Rows": N,                  +
> >            "Actual Loops": N,                 +
> >            "Sort Key": ["unique1"],           +
> >            "Workers planned": N,              +
> >            "Sort Method": "external merge",   +
> >            "Average Sort Space Used": N,      +
> >            "Peak Sort Space Used": N,         +
> >            "Sort Space Type": "Disk",         +
>
> I think the idea is interesting, however there are a few problems in the
> patch.
>
> First, I think that it should only be done in the VERBOSE OFF mode.  If
> you ask
> for a VERBOSE output you don't need both the details and the summarized
> version.
>
> Other minor problems:
>
> - why (only) emitting the number of workers planned and not the number of
>   workers launched?
> - the textual format is missing details about what the numbers are, which
> is
>   particularly obvious since avgSpaceUsed and peakSpaceUsed don't have any
> unit
>   or even space between them:
>
> +                        "Sort Method: %s  %s: " INT64_FORMAT INT64_FORMAT
> "kB\n",
> +                        sortMethod, spaceType, avgSpaceUsed,
> peakSpaceUsed);
>
>
> ________________________________
>
> ⚠ External Email: This email originated from outside of the organization.
> Do not click links or open attachments unless you recognize the sender.
>

The patch failed different regression tests on all platforms. Please
correct that and send an updated patch.

[06:40:02.370] Test Summary Report
[06:40:02.370] -------------------
[06:40:02.370] t/002_pg_upgrade.pl (Wstat: 256 Tests: 13 Failed: 1)
[06:40:02.370] Failed test: 4
[06:40:02.370] Non-zero exit status: 1
[06:40:02.370] Files=2, Tests=21, 45 wallclock secs ( 0.02 usr 0.00 sys +
3.52 cusr 2.06 csys = 5.60 CPU)
-- 
Ibrar Ahmed