On Fri, Jan 13, 2012 at 7:21 AM, Robert Haas <robertmh...@gmail.com> wrote: > I think that people who are using index-only scans are going to want > some way to find out the degree to which the scans are in fact > index-only. > > So here's a 5-line patch that adds the number of heap fetches to the > EXPLAIN ANALYZE output. This might not be all the instrumentation > we'll ever want here, but I think we at least want this much.
A review: It is not in context diff format. It applies, builds, and runs cleanly, including under --enable-cassert No docs, but the output format of other EXPLAIN ANALYZE fields generally aren't documented either. But it is fairly self-explanatory (provided one knows what an index-only scan is in the first place). No regression tests, but that too seems appropriate. It does what it says. We (or I, anyway) want what it does. As mentioned, we might want counts also exposed via the stats collector, but that is another matter. I don't see how it could possibly cause a meaningful slow-down, and tests on all-memory index-only scans cannot detect a slow down. On my machine, using EXPLAIN ANALYZE causes a 16 fold slow down on an in memory -i -s100 "select count(*) from pgbench_accounts" conducted after a run of pgbench -T30, so that there are some heap fetches needed. Compared to that ANALYZE slowdown, any additional slow down from this patch is ridiculously slow. It would be nice if you could get the output of this patch (and of the BUFFERS option to EXPLAIN) without incurring the timing overhead of ANALYZE. But again, that is not the subject of this patch. I wondered about the type of ioss_HeapFetches. Other members of execnodes.h structs tend to be int64, not long. But those others are not reported in EXPLAIN. Other things reported in explain.c tend to be long. This seems to be a foot in both worlds, and your response to Peter is convincing. I agree with Tom on the pre increment versus post increment of ioss_HeapFetches. I also wondered if ioss_HeapFetches ought to be initialized to zero. I looked for analogous code but didn't find enough analogy to convince me one way or the other. All the other members of IndexOnlyScanState have entries in the big comment block preceding the typedef, so I would expect ioss_HeapFetches should have one as well. These are all minor issues, and since you are a committer I'm going to mark it ready for committer. As a side-note, I noticed that I needed to run vacuum twice in a row to get the Heap Fetches to drop to zero. I vaguely recall that only one vacuum was needed when ios first went in (and I had instrumented it to elog heap-fetches). Does anyone know if this the expected consequence of one of the recent changes we made to vacuum? Thanks, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers