Re: Updated posix fadvise patch v19

"Robert Haas" <robertmhaas(at)gmail(dot)com> writes:

> 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.
>
> - 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.

When I posted the plan was that people would run experiments with other
systems. In particular I was hoping Zoltan who original reported the
sequential file strategy stuff being helpful might be able to see if this
works for him.

As this hasn't happened and I haven't been able to demonstrate it being useful
myself I guess it makes more sense to separate the two now and set the
sequential scan stuff aside until someone can demonstrate it being useful.

> - 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 put it in for debugging since it was hard to know if it was actually doing
anything otherwise. I figured whoever was reviewing the patch would run into
the same problem. It would be trivial to strip out though.

> - 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.

Well there is a difference because bitmap heap scans do that slow-start thing.
They ramp up the prefetching as you fetch more tuples from the bitmap up to
the user configurable GUC so the point was to instrument what level they get
to. Regular index scans prefetch whatever they find in the index leaf pages
which will vary from page to page and isn't really something the user can
influence anyways.

But as I said, it was mostly so I could tell what the slow start algorithm was
doing and make sure it was doing anything at all in testing.

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

Er, uh, yeah that's bogus. The RelationIsValid is pointless as
RelationOpenSmgr will just crash if that's not true.

But I'm not nearly as convinced that BlockNumberIsValid() isn't worth
asserting for. I don't see anything in the buffer tag lookup code which will
fail in that case. What confuses me is that there's no corresponding check in
the ReadBuffer code.

Tom? Heikki? Should we have an assert in ReadBuffer asserting
BlockNumberIsValid -- especially for the zero-page case where we're not
actually going to do I/O? Or am I missing where that will seg fault?

> Department of nitpicking:

Will clean these up, they all look valid. I thought I did clean things up
already -- I guess you should be happy you're looking at it *after* that
cleanup :/

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning