Re: Bug in bttext_abbrev_convert()

On Tue, Jun 30, 2015 at 9:39 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>> There is no real testing of sorting in the regression tests. It would
>> be nice to have a way of generating a large and varied selection of
>> sort operations programmatically, to catch this kind of thing.
>> pg_regress is not really up to it. The same applies to various other
>> cases where having a lot of "expected" output makes using pg_regress
>> infeasible.
>
> Well, the issue is double here:
> 1) Having a buildfarm member with valgrind would greatly help.

Not sure that I agree with that. It wouldn't hurt, but people are
running Valgrind often enough these days that I think it's unlikely
that having it run consistently actually adds all that much. Maybe it
would make a difference to do it on a 32-bit platform, since most of
us have not used a 32-bit machine as our primary development
environment in about a decade, and we probably miss some things
because of that.

If you run Valgrind on Postgres master these days, you actually have a
pretty good chance of not finding any issues, which is great.
shared_buffers support for Valgrind would also help a lot (e.g. making
it so that referencing a shared buffer that isn't pinned causes a
Valgrind error -- Noah and I discussed this a while back; should be
doable). If you're looking for a new project, may I highly recommend
working on that.  :-)

> 2) This code path is not used at all AFAIK in the test suite, so we
> definitely lack regression tests here. In your opinion what would be a
> sort set large enough to be able to trigger this code path? The idea
> is to not make the regression test suite take too much time because of
> it, and not to have the server created by pg_regress running the
> regression tests having a too large PGDATA folder. For example, could
> a btree index do it with a correct data set do it on at least one
> platform?

Maybe. The trick would be constructing a case where many different
code paths are covered, including the many different permutations and
combinations of how an external sort can go (number of runs, merge
steps, datatypes, etc).

In general, I think that there is a lot of value to be added by
someone making it their explicit goal to increase test coverage, as
measured by a tool like gcov (plus subjective expert analysis, of
course), particularly when it comes to things like memory corruption
bugs (hopefully including shared memory corruption bugs, and hopefully
including recovery). If someone was doing that in a focused way, then
the codepath where we must explicitly pfree() (in the case of this
bug) would probably have coverage, and then Valgrind probably would
catch this.

As long as that has to be a part of adding things to the standard
regression test suite (particularly with sorts), a suite which is
expected to run quickly, and as long as we're entirely relying on
pg_regress, we will not make progress here. Maybe if there was an
extended regression test suite that was explicitly about meeting a
code coverage goal we'd do better. Yes, I think mechanically
increasing code coverage is useful as an end in itself (although I do
accept that meeting a particularly coverage percentage is often not
especially useful). Increasing coverage has led me to the right
question before, just as static analysis has done that for you. For
example, the effort of increasing coverage can find dead code, and you
can intuitively get a sense of where to look for bugs manually by
determining mechanically what code paths are hard to hit, or are
naturally seldom hit.

It would be nice to always have a html report from gcov always
available on the internet. That would be something useful to automate,
IMV. Watching that regress over time might provide useful insight, but
I only use gcov a couple of times a year, so that's not going to
happen on its own.
-- 
Peter Geoghegan