Re: Trying to add more tests to gistbuild.c - Mailing list pgsql-hackers

Matheus Alcantara <mths.dev@pm.me> writes:
> On Friday, July 29th, 2022 at 19:53, Tom Lane tgl@sss.pgh.pa.us wrote:
>> I wonder if we can combine ideas from the two patches to get a
>> better tradeoff of code coverage vs. runtime.

> I was checking the Pavel patch and notice that he was using the fillfactor
> parameter when creating the gist index. I changed my previous patch to include
> this parameter and the code coverage of gistbuild.c and gistbuildbuffers.c was
> improved to 97.7% and 92.8% respectively.

Nice!

I poked at this some more, wondering if we could combine the two new
index builds into one test, and eventually realized something I should
probably have figured out before: if you make effective_cache_size
too small, it refuses to use buffering build at all, and you get here:

    if (levelStep <= 0)
    {
        elog(DEBUG1, "failed to switch to buffered GiST build");
        buildstate->buildMode = GIST_BUFFERING_DISABLED;
        return;
    }

In fact, at least on my machine, the first test case hits this and
thus effectively adds no coverage at all :-(.  If I remove that and
just add the second index build, the above-quoted bit is the *only*
thing in gistbuild.c or gistbuildbuffers.c that is missed compared
to using both test cases.  Moreover, the runtime of the test comes
down to ~240 ms, which is an increment of ~25ms instead of ~65ms.
(Which shows that the non-buffering build is slower, not surprising
I guess.)

I judge that covering those three lines is not worth the extra 40ms,
so pushed just the second test case.

Thanks for poking at this!  I'm much happier now about the state of
code coverage in that area.