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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
