Aleksander Alekseev <aleksander@timescale.com> writes:
> Personally I believe this change makes perfect sense. Although this is
> arguably not an ideal test for gistInitBuffering(), writing proper
> tests for `static` procedures is generally not an easy task. Executing
> the code at least once in order to make sure that it doesn't crash,
> doesn't throw errors and doesn't violate any Asserts() is better than
> doing nothing.
Yeah, our poor test coverage for gist buffering builds has been
complained of before [1]. It'd be good to do something about that;
the trick is to not bloat the runtime of the core regression tests
too much.
I checked this patch and what I see is:
* gistbuild.c coverage improves to 81.8% line coverage, more or less
as stated (probably depends on if you use --enable-cassert)
* gistbuildbuffers.c coverage improves from 0 to 14.0%
* gist.sql runtime goes from ~215ms to ~280ms
The results for gistbuildbuffers.c are kind of disappointing, especially
given the nontrivial runtime cost. (YMMV on the runtime of course, but
I see pretty stable numbers under non-parallel "make installcheck".)
In the previous thread, Pavel Borisov offered a test patch [2] that
still applies, and it brings the line count coverage to 95%+ in
both files. Unfortunately it more than doubles the test runtime,
to somewhere around 580ms, so I rejected it at the time hoping
for a better compromise.
The idea I see you using that Pavel missed is to reduce
effective_cache_size to persuade the buffering build logic to kick in
with less data. But it looks like multilevel buffering still doesn't
get activated, which is how come gistbuildbuffers.c coverage still
remains poor. (I tried reducing effective_cache_size further,
but it didn't help.)
I wonder if we can combine ideas from the two patches to get a
better tradeoff of code coverage vs. runtime.
Another thing we might consider is to move the testing responsibility
somewhere else. The reason I'm allergic to adding a lot of runtime
here is that the core regression tests are invoked at least four times
in a standard buildfarm run, often more. But that concern could be
alleviated if we put the test somewhere else. Maybe contrib/btree_gist
would be suitable?
regards, tom lane
[1] https://www.postgresql.org/message-id/flat/16329-7a6aa9b6fa1118a1%40postgresql.org
[2] https://www.postgresql.org/message-id/CALT9ZEECCV5m7wvxg46PC-7x-EybUmnpupBGhSFMoAAay%2Br6HQ%40mail.gmail.com
