On Tue, Jul 28, 2020 at 12:54:35PM +1200, David Rowley wrote:
> On Mon, 27 Jul 2020 at 14:54, Justin Pryzby <pry...@telsasoft.com> wrote:
> > It's unrelated to hashAgg vs hashJoin, but I also noticed that this is shown
> > only conditionally:
> >
> > if (es->format != EXPLAIN_FORMAT_TEXT)
> > {
> > if (es->costs && aggstate->hash_planned_partitions > 0)
> > {
> > ExplainPropertyInteger("Planned Partitions", NULL,
> >
> > aggstate->hash_planned_partitions, es);
> >
> > That was conditional since it was introduced at 1f39bce02:
> >
> > if (es->costs && aggstate->hash_planned_partitions > 0)
> > {
> > ExplainPropertyInteger("Planned Partitions", NULL,
> >
> > aggstate->hash_planned_partitions, es);
> > }
> >
> > I think 40efbf870 should've handled this, too.
>
> hmm. I'm not sure. I think this should follow the same logic as what
> "Disk Usage" follows, and right now we don't show Disk Usage unless we
> spill.
Huh ? I'm referring to non-text format, which is what you changed, on the
reasoning that the same plan *could* spill:
40efbf8706cdd96e06bc4d1754272e46d9857875
if (es->format != EXPLAIN_FORMAT_TEXT)
{
if (es->costs && aggstate->hash_planned_partitions > 0)
{
ExplainPropertyInteger("Planned Partitions", NULL,
aggstate->hash_planned_partitions, es);
}
...
/* EXPLAIN ANALYZE */
ExplainPropertyInteger("Peak Memory Usage", "kB", memPeakKb,
es);
- if (aggstate->hash_batches_used > 0)
- {
ExplainPropertyInteger("Disk Usage", "kB",
aggstate->hash_disk_used, es);
ExplainPropertyInteger("HashAgg Batches", NULL,
aggstate->hash_batches_used, es);
> Since we only use partitions when spilling, I don't think it
> makes sense to show the estimated partitions when we don't plan on
> spilling.
In any case, my thinking is that we *should* show PlannedPartitions=0,
specifically to indicate *that* we didn't plan to spill.
> I think if we change this then we should change Disk Usage too.
> However, I don't think we should as Sort will only show "Disk" if the
> sort spills. I think Hash Agg should follow that.
>
> For the patch I posted yesterday, I'll go ahead in push it in about 24
> hours unless there are any objections.
