Re: Trying to add more tests to gistbuild.c

On Sun, 31 Jul 2022 at 00:33, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Matheus Alcantara <mths(dot)dev(at)pm(dot)me> writes:
> > On Friday, July 29th, 2022 at 19:53, Tom Lane tgl(at)sss(dot)pgh(dot)pa(dot)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.
>

I'm happy, that the improvement of the tests I've forgotten about so long
ago is finally committed. Thank you!

--
Best regards,
Pavel Borisov