Re: Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch <noah@leadboat.com> wrote:
> Yes, please produce a patch version bearing a regression test that exercises
> the bug.  I feel it counts even if the you need Valgrind Memcheck to see that
> it exercises the bug, but I won't interfere if Robert disagrees.  The test
> should take moderate steps to minimize costs.  For example, it should decrease
> maintenance_work_mem, not sort >16 MiB of data.

I agree with both points. I think that we can do just as well with a
1MiB maintenance_work_mem setting.

> PostgreSQL is open to moving features from zero test coverage to nonzero test
> coverage.  The last several releases have each done so.  Does that
> sufficiently clarify the policy landscape?

Not quite, no. There are costs associated with adding regression tests
that exercise external sorting. What are the costs, and how are they
felt? How can we add more extensive test coverage without burdening
those that run extremely slow buildfarm animals, for example?

>> I think that a new tuplesort.sql test file is where a test like this
>> belongs (not in the existing cluster.sql test file). If I write a
>> patch along those lines, is it ever going to be accepted? I would be
>> happy to work on this, but we need to have a sensible conversation on
>> what's acceptable to everyone in this area first. I've screamed bloody
>> murder about the test coverage in this area before, and nothing
>> happened.
>
> I'll guess that Robert would prefer a minimal test in cluster.sql over
> starting a new file.  If you want to be sure, wait for his input.

There are significant advantages to a tuplesort.sql file. It gives us
an easy way to avoid running the tests where they are inappropriate.
Also, there are reasons to prefer a datum or heap tuplesort for
testing certain facets of external sorting: setting work_mem to 64KiB
can allow a test that exercises multiple polyphase merge passes and is
relative fast (fast compared to a CLUSTER-based test, with 1MiB of
maintenance_work_mem, say).

It's also true that there are special considerations for both hash
tuplesorts and datum tuplesorts. As recently as a week ago, hash
tuplesorts were fixed by Tom, having been *completely* broken for
about one week shy of an entire year. I plan to address that
separately, per discussion with Tom.

> I don't know, either.  You read both Robert and me indicating that this bug
> fix will be better with a test, and nobody has said that a test-free fix is
> optimal.  Here's your chance to obliterate that no-tests precedent.

I'm happy that I can do at least that much, but I see no reason to not
go significantly further.

-- 
Peter Geoghegan