Re: [PATCH] Tests for reloptions

On Sun, Oct 1, 2017 at 5:18 AM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
> src/backend/access/common/reloptions.c get only 7 lines, it was quite covered
> by existing test, but all most of the access methods gets some coverage
> increase:
>
> src/backend/access/brin         1268 -> 1280 (+18)
> src/backend/access/gin          2924 -> 2924 (0)
> src/backend/access/gist         1673 -> 2108 (+435)
> src/backend/access/hash 1580 -> 1638 (+58)
> src/backend/access/heap 2863 -> 2866 (+3)
> src/backend/access/nbtree       2565 -> 2647 (+82)
> src/backend/access/spgist       2066 -> 2068 (+2)
>
> Though I should say that incredible coverage boost for gist, is the result of
> not steady results of test run. The real value should be much less...

+-- Test buffering and fillfactor reloption takes valid values
+create index gist_pointidx2 on gist_point_tbl using gist(p) with
(buffering = on, fillfactor=50);
+create index gist_pointidx3 on gist_point_tbl using gist(p) with
(buffering = off);
+create index gist_pointidx4 on gist_point_tbl using gist(p) with
(buffering = auto);
Those are the important bits doing the boost in gist visibly. To be kept.

-CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops);
+CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random
float8_ops) WITH (fillfactor=60);
Woah. So that alone increases hash by 58 steps.

+CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0);
+ERROR:  value 0 out of bounds for option "length"
+DETAIL:  Valid values are between "1" and "4096".
contrib/bloom contributes to the coverage of reloptions.c thanks to
its coverage of the add_ routines when the library is loaded. And
those don't actually improve any coverage, right?

> Nevertheless tests touches the reloptions related code, checks for proper
> error handling, and it is good.

Per your measurements reloptions.c is improved by 1.7%, or 7 lines as
you say. Five of them are in parse_one_reloption for integer (2) and
reals (2), plus one error at the top. Two are in transformRelOptions
for a valid namespace handling. In your patch reloptions.sql is 141
lines. Couldn't it be shorter with the same improvements? It looks
that a lot of tests are overlapping with existing ones.

> I think we should commit it.

My take is that a lighter version could be committed instead. My
suggestion is to keep the new test reloptions minimal so as it tests
the relopt types and their bounds, and I think that you could remove
the same bounding checks in the other tests that you added: bloom,
gist, etc.
-- 
Michael