Re: Summary Sort workers Stats in EXPLAIN ANALYZE

On Mon, Mar 28, 2022 at 2:55 PM Jian Guo <gjian(at)vmware(dot)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 <rjuju123(at)gmail(dot)com>
> *Sent:* Friday, March 25, 2022 17:04
> *To:* Jian Guo <gjian(at)vmware(dot)com>
> *Cc:* pgsql-hackers(at)lists(dot)postgresql(dot)org <
> pgsql-hackers(at)lists(dot)postgresql(dot)org>; Zhenghua Lyu <zlyu(at)vmware(dot)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