Re: Updated posix fadvise patch v19

I was pretty leery about reviewing this one due to the feeling that I
might well be in over my head, but they talked me into it, so here
goes nothin'. Apologies in advance for any deficiencies in this
review.

- Overall, this looks pretty clean. The style appears to be
consistent with the surrounding code, the patch applies with only
minor fuzz, there is not much cruft in the diff (I found a few very
minor unnecessary hunks, see department of nitpicking below) and the
whole thing compiles and passes regression tests. Also, while I'm not
totally qualified to judge the success of your efforts, it appears
that you've attempted to make the changes in a way that preserves the
existing abstractions, which is good.

- However, having said that, it looks as if there is still a bit of
experimentation going on in terms of what you actually want the patch
to do. There are a couple of things that say FIXME or XXX, and at
least one diff hunk that adds code surrounded by #if 0 (in FileRead).
Maybe committing FIXME is OK, but certainly #if 0 isn't, so there's at
least a bit of work needed here to put this in its final form, though
I think perhaps not that much. As you mentioned when posting this
version of the patch, it does two unrelated things only one of which
you are confident is beneficial. I suggest splitting this into two
patches and trying to get the prefetch part committed first. I think
that would make for easier review, too.

- I dislike cluttering up EXPLAIN ANALYZE with even more crap. On the
flip side, I am loathe to take away any sort of instrumentation that
might be helpful. I think we need to revamp the syntax of EXPLAIN
[ANALYZE] to support some kind of option list, so that users can
request the information they care about and not be overwhelmed by what
they don't care about. (ISTR that a similar proposal was made with
regard to VACUUM some time ago... perhaps the same ideas could be
applied to both.) That would need to be done as a separate patch, so
maybe we shouldn't worry about it here.

- It's not clear to me whether there is a reason why we are only
adding instrumentation for bitmap heap scan; would not other scan
types benefit from something similar? If we're not going to add
instrumentation for all the cases that can benefit from it, then we
should just rip it out.

- StrategyFileStrategy doesn't handle the recently added BAS_BULKWRITE
strategy. I'm not sure whether it needs to, but it seems to me that
this a trap for the unwary: we should probably add a comment where the
BAS_* constants are defined warning that any changes here may/will
also necessitate changes there. I think a detailed comment on the
function itself explaining why it does what it does and how to decide
what to do for a new type of BufferAccessStrategy would be a good
idea.

- I am mildly concerned that we are overusing the word Strategy. The
purpose of StrategyFileStrategy is, of course, to take a
BufferAccessStrategy (which is an object) and return a FILE_STRATEGY_*
constant. But someone might not find that entirely evident from the
name. If we're going to have multiple things floating around that are
different from each other but all called strategies, ISTM we should
name functions etc. to make clear which one we're talking about. Or
else pick a different word. Maybe for now it's sufficient to rename
this function to AccessStrategyGetFileStrategy, or something.

- The WARNING at the top of PrefetchBuffer() seems strange. Is that
really only a WARNING? We just continue anyway?

Department of nitpicking:
- The very first comment change in src/backend/commands/explain.c is
apparently extraneous.
- guc.c misspells the work "concurrent" as "concurrenct".
- The first diff hunk in each of fd.h and smgr.h is an unnecessary
whitespace change.
- nodeBitmapHeapscan.c adds an ELOG at DEBUG2 - do we really want
this, or is it leftover debugging code?

...Robert