From: Nikolay Shaplov <dhyan@nataraj.su>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>
Subject: Re: [PATCH] Tests for reloptions
Date: Fri, 06 Oct 2017 12:27:32 +0300
Message-ID: <1933219.021c0Pr2Ov@x200m>
User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: 
 <CAB7nPqSJyUgmrcjf+BMgBog-S8Spkq5VvcpZSX4_4sXNYFG7YQ@mail.gmail.com>
References: <2615372.orqtEn8VGB@x200m> <1514042.nPA0zSFN5r@x200m>
 <CAB7nPqSJyUgmrcjf+BMgBog-S8Spkq5VvcpZSX4_4sXNYFG7YQ@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 3 =D0=BE=D0=BA=D1=
=82=D1=8F=D0=B1=D1=80=D1=8F 2017 11:48:43 =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=
=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Michael Paquier =D0=BD=D0=B0=D0=BF=
=D0=B8=D1=81=D0=B0=D0=BB:

> > 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:
> >=20
> > 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)
> >=20
> > Though I should say that incredible coverage boost for gist, is the res=
ult
> > 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 =3D on, fillfactor=3D50);
> +create index gist_pointidx3 on gist_point_tbl using gist(p) with
> (buffering =3D off);
> +create index gist_pointidx4 on gist_point_tbl using gist(p) with
> (buffering =3D auto);
> Those are the important bits doing the boost in gist visibly. To be kept.
>=20
> -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=3D60);
> Woah. So that alone increases hash by 58 steps.
Might be... As I have noticed, tests  for indexes filled with random data t=
est=20
coverage, gives random coverage. There needed more random data to give stea=
dy=20
results. I am gathering statistics now, and later I hope I will offer anoth=
er=20
patch(es) to fix it. So not all of 58 lines might be result of this patch :=
-)
=20
> +CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=3D0);
> +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?
I actually run only simple "make check". running "make check" for bloom wil=
l=20
also add some add_ routines to coverage.


> > Nevertheless tests touches the reloptions related code, checks for prop=
er
> > error handling, and it is good.
>=20
> 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.
>=20
> > I think we should commit it.
>=20
> 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.

I've been thinking a lot, and rereading the patch. When I reread it I've be=
en=20
thinking that I would like to add more tests to it now... ;-)

If the only purpose of tests is to get better coverage, then I would agree=
=20
with you. But I've been thinking that tests also should check that everythi=
ng=20
behaves as expected (or as written in documentation).

I would say that options names is a of part of SQL dialect that postgres us=
es,=20
kind of part of the syntax. It is good to be sure that they still supported=
.=20
So I would add a test for every option heap supports.

Checking minimum and maximum values are more disputable. The argumentation =
for=20
it is that min and max are written in documentation, and it is good to chec=
k=20
that postgres behaves according to documentation...


But it you tell me for sure that test only for code coverage and should not=
 do=20
anything more, than I surely remove all tests that are not increase test=20
coverage.


--=20
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)


--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers