Re: machine-readable explain output v4

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: machine-readable explain output v4
Date: 2009-08-02 16:06:40
Message-ID: 200908021806.40634.andres@anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Robert, Hi all,

On Thursday 30 July 2009 05:05:48 Robert Haas wrote:
> OK, here's the updated version of my machine-readable explain output
> patch. This needed heavy updating as a result of the changes that Tom
> asked me to make to the explain options patch, and the further changes
> he made himself. In addition to any regressions I may have introduced
> during the rebasing process, there is one definite problem here: in
> the previous version of this patch, explain (format xml) returned XML
> data; now, it's back to text.

> The reason for this regression is that Tom asked me to change
> ExplainStmt to just carry a list of nodes and to do all the parsing in
> ExplainQuery. Unfortunately, the TupleDesc is constructed by
> ExplainResultDesc() which can't trivially be changed to take an
> ExplainState, because UtilityTupleDescriptor() also wants to call it.
> We could possibly fix this by a hack similar to the one we already
> added to GetCommandLogLevel(), but I haven't done that here.
Hm. I think its cleaner to move the option parsing into its own function - its
3 places parsing options then and probably not going to get less.
I am not sure this is cleaner than including the parsed options in ExplainStm
though... (possibly in a separate struct to avoid changing copy/equal-funcs
everytime)

Some more comments on the (new) version of the patch:
- The regression tests are gone?
- Currently a value scan looks like »Values Scan on "*VALUES*"« What about
adding its alias at least in verbose mode? This currently is inconsistent with
other scans. Also he output columns of a VALUES scan are named column$N even
if names as specified like in AS foo(colname)
- why do xml/json contain no time units anymore? (e.g. Total Runtime).
Admittedly thats already inconsistent in the current text format...

- Code patterns like:
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, "Total runtime: %.3f ms\n",
1000.0 * totaltime);
else if (es->format == EXPLAIN_FORMAT_XML)
appendStringInfo(es->str,
" <Total-Runtime>%.3f</Total-Runtime>\n",
1000.0 * totaltime);
else if (es->format == EXPLAIN_FORMAT_JSON)
appendStringInfo(es->str, ",\n \"Total runtime\" : %.3f",
1000.0 * totaltime);
or
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, " for constraint %s", conname);
else if (es->format == EXPLAIN_FORMAT_XML)
{
appendStringInfoString(es->str, " <Constraint-Name>");
escape_xml(es->str, conname);
appendStringInfoString(es->str, "</Constraint-Name>\n");
}
else if (es->format == EXPLAIN_FORMAT_JSON)
{
appendStringInfo(es->str, "\n \"Constraint Name\": ");
escape_json(es->str, conname);
}

possibly could be simplified using ExplainPropertyText or a function accepting
a format string.
At least for the !EXPLAIN_FORMAT_TEXT this seems simple at multiple places in
ExplainOnePlan and report_triggers.

On Friday 31 July 2009 23:13:54 Robert Haas wrote:
> On Sat, Jul 18, 2009 at 10:29 PM, Andres Freund<andres(at)anarazel(dot)de> wrote:
> > One part where I find the code flow ugly is 'did_boilerplate' in
> > report_triggers/its callsites.
> > I can see why it is done that way, but its not exactly obvious to read
> > when you want to find out how the format looks.
> Suggestions?
The only idea without building more xml/json infrastructure I have is using a
separate stringbuffer inside report_triggers - but thats not much nicer.

Thats all I could find right now...

Andres

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2009-08-02 16:29:19 Re: IMMUTABLE break inlining simple SQL functions.
Previous Message Pavel Stehule 2009-08-02 15:49:38 Re: WIP: to_char, support for EEEE format