Re: Explain analyze getrusage tracking

On Tue, Nov 16, 2010 at 12:19 PM, Greg Stark <stark@mit.edu> wrote:
> On Tue, Nov 16, 2010 at 2:43 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I don't really think these changes to the INSTR macros make much
>> sense.  The macros don't really add much notational convenience;
>> they're mostly wrappers to make the WIN32 and non-WIN32 cases work
>> similarly for the instrumentation stuff, so hacking them up to use
>> them for this doesn't seem like it adds anything.  Just do whatever it
>> is you need to do, or define macros locally in explain.c.
>
> Well they're going to be just like the macros in instr_time and I
> didn't really want to duplicate them. What's really goofy here is that
> on Windows the getrusage portability wrapper is actually consing up
> those times from FILETIMEs which I believe are actually just like the
> instr_time data type that we're already using in those macros so it's
> going of doing a silly dance converting to struct timeval just so it
> can use macros that aren't convenient at all on Windows.
>
> I definitely agree this is awkward. I would love to find a cleaner
> solution here. I'll try copying the macros and see if that works out
> more cleanly.

OK.

>> It doesn't make much sense to me to normalize the memory for this
>> output to a variable unit when the other memory values we use in
>> explain.c are still going to be printed as kB.  I think we should just
>> print it in kB and call it good.  Alternatively, we could apply the
>> same normalization algorithm across the board, but I don't think
>> that's as good.
>
> I think we should have a project policy of always printing memory and
> disk usage in kB, MB, GB etc unless they're functions returning an
> integer intended for machine use. Effectively this is the dual of
> accepting units on all our memory gucs. I don't know about others but
> I find it pretty hard to read things like 1234567kB and compare it to
> 125765kB in my head.

We have no other place in the system that takes 12345678kB and
converts it inexactly to MB or GB.

rhaas=# set work_mem to '1048576kB';
SET
rhaas=# show work_mem;work_mem
----------1GB
(1 row)

rhaas=# set work_mem to '1048577kB';
SET
rhaas=# show work_mem;work_mem
-----------1048577kB
(1 row)

We could decide that for text-format EXPLAIN output, an inexact
conversion is OK, but it's still not OK to do it for the new fields
you're adding and not for the existing fields (see the sort and hash
instrumentation).  Personally, I'm not really convinced that making
such a change has a lot of value, and I think it should be submitted
as a separate patch and discussed separately, rather than being rolled
in here.  But if we are going to change it then we at least need to be
consistent.

>>  Using the verbose option to control how much data the resource option
>> prints is, I think, not a good idea.  If you want to have two modes,
>> one for partial rusage data and one for full rusage data, you can just
>> as easily implement EXPLAIN (RESOURCE [PARTIAL|FULL]).  I believe that
>> the existing grammar is adequate to support that; you'd just need to
>> write the appropriate DefElem-parsing code.  But personally I'd just
>> print the whole kit and kaboodle regardless.
>
> This is a separate question. The numbers I'm outputing without VERBOSE
> now are the numbers that I understand and which relate to the other
> numbers in explain. My intent was that as we understand how the other
> numbers relate to Postgres functioning we'll learn which ones to
> include. But mostly they're hard to relate to Postgres and the query
> execution in any way and just noise that are hard to explain to users.
> I'm not even sure which ones it makes sense to sum over time -- is the
> integral virtual memory usage over time something that it makes sense
> to sum over time? What units is it in?

I suspect that the ru_maxrss, ru_ixrss, ru_idrss, and ru_isrss values
are useless to us for EXPLAIN purposes because it sounds like they
don't increment over time.  The others presumably do, so it makes
sense to show 'em if the system is collecting them.

> I don't see why VERBOSE isn't the right key to use. The other things
> VERBOSE toggles are also extra detail that's usually useless but might
> be useful if you're interested in the inner workings. Things like
> which fields are being included at each level of the plan for example.

Yeah, VERBOSE is kind of a catch-all for things that we don't have
individual flags for.  But I think it's better for each piece of data
to depend on one setting, rather than a combination of two or more
settings.  Otherwise you end up being forced to use VERBOSE and then
you get this deluge of output.  I'd actually sort of like to remove
some things from VERBOSE and give them their own settings, rather than
adding more.  The fact that VERBOSE turns on "Output:" is particularly
annoying.

...Robert