On Fri, Jul 01, 2016 at 09:12:42PM -0700, Peter Geoghegan wrote:
> On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch <noah@leadboat.com> wrote:
> > 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?
Project policy is silent on those matters. If your test performs one sort on
a table <3 MiB, I expect no trouble.
> > 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.
Don't risk bundling tests for other sorting scenarios. A minimal test for the
bug in question helps to qualify your patch as an exemplary bug fix. Broader
test coverage evicts your patch from that irreproachable category, inviting
reviewers to determine that you went too far.
nm