Re: Updated posix fadvise patch v19

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

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

Well, yes, we have lots of XXX and FIXMEs throughout the source code. We need
to make progress even if we don't know for certain that we have the perfect
crystalline code structure in all its glory.

I have the same reaction and fear when I see reviewers' questions about
whether a patch is "finished". If we wait until someone is "finished" working
on a feature we'll never get the feature -- there's always more that can be
done. We want to commit things when they are ready for use, not wait until
there's nothing more to do.

The XXX is something that probably needs to be fixed but it's just a question
of what header file to put a declaration in. I couldn't find a good choice but
perhaps someone else has an idea?

For the FIXMEs I don't have any problem leaving them in place. They're
warnings to future coders working in the same area of what they may have to do
to make the code more general. In particular both FIXMEs are related to memory
management of the iterator structures. I think just allocating them in the
bitmap memory context is fine for existing callers. I would rather leave them
there because I would like a reviewer to double check that we don't have a
memory leak there.

The #if 0 is part of the FileStrategy code to use POSIX_FADV_SEQUENTIAL which
I'm removing now so it's irrelevant.

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

Maintaining two interdependent patches is a bit of a pain. This is one reason
it's annoying when a patch goes uncommitted for a long time. It's hard to work
on further improvements which are dependent on it.

Nobody seems to care about the sequential scan case any more so I doubt I'll
bother maintaining it after I remove it from this patch.

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

My feeling is that it was useful but only marginally and mostly only for
testing that things were working properly. The main benefit is giving people a
real feeling for how much data a given effective_io_concurrency actually
translates to.

I'm removing this now. If anyone else thinks it was nice to have scream now.

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