On Fri, Jun 19, 2020 at 03:03:41PM +1200, David Rowley wrote: > On Fri, 19 Jun 2020 at 14:20, Justin Pryzby <pry...@telsasoft.com> wrote: > > Please be sure to use two spaces between each field ! > > > > See earlier discussions (and commits referenced by the Opened Items page). > > https://www.postgresql.org/message-id/20200402054120.gc14...@telsasoft.com > > https://www.postgresql.org/message-id/20200407042521.GH2228%40telsasoft.com > > Thanks. I wasn't aware of that conversion.
To be clear, there wasn't a "conversion". There were (and are still) different formats (which everyone agrees isn't ideal) used by "explain(BUFFERS)" vs Sort and Hash. The new incr sort changed from output that looked like Buffers (one space, and equals) to output that looked like Sort/Hashjoin (two spaces and colons). And the new explain(WAL) originally used a hybrid (which, on my request, used two spaces), but it was changed (back) to use one space, for consistency with explain(BUFFERS). Some minor nitpicks now that we've dealt with the important issue of whitespace: + bool gotone = false; => Would you consider calling that "found" ? I couldn't put my finger on it at first why this felt so important to ask, but realized that my head was tripping over a variable whose name starts with "goto", and spending 0.2 seconds trying to figure out what you might have meant by "goto ne". + int n; + + for (n = 0; n < aggstate->shared_info->num_workers; n++) => Maybe use a C99 declaration ? + if (hash_batches_used > 0) + { + ExplainPropertyInteger("Disk Usage", "kB", hash_disk_used, + es); + ExplainPropertyInteger("HashAgg Batches", NULL, + hash_batches_used, es); + } => Shouldn't those *always* be shown in nontext format ? I realize that's not a change new to your patch, and maybe should be a separate commit. + size = offsetof(SharedAggInfo, sinstrument) + + pcxt->nworkers * sizeof(AggregateInstrumentation); => There's a couple places where I'd prefer to see "+" at the end of the preceding line (but YMMV). -- Justin