Horton

Thread

Commits

  1. Add more tests for reloptions

  1. [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2017-09-05T09:41:34Z

    This thread continues discussion at https://www.postgresql.org/message-id/20170903094543.kkqdbdjuxwa5z6ji@alvherre.pgsql
(Shortly: I refactored reloptions code, Alvaro offered to commit tests before 
the full patch)

> I see that this patch adds a few tests for reloptions, for instance in
> bloom.  I think we should split this in at least two commits, one that
> adds those tests before the functionality change, so that they can be
> committed in advance, verify that the buildfarm likes it with the
> current code, and verify the coverage.

This sounds as a really good idea.

Though I have several questions. This tests also covers some functionality 
that were introduced only in my patch:

1. Forbid SET and RESET options where they should not be changed
2. Forbid creating tables with toasts options when no toast table is created
3. Split StdRdOptions into HeapOptions and ToastOptions and forbid uising Heap 
specific options for toast.

In the patch I've attached I've commented out this functionality. But I am not 
quite sure that it is good idea to commit it this way in master.

May be it would be good to make 1-3 as a separate patches and bring it's tests 
with, as a separate commit. But...

2nd can be easily ported to master. It does not depend much on my reloptions 
patch as far as I remember.

It would be insane to port 1st feature to master. It highly integrated with 
reloptions mechanism, It would require complete reimplementation of this 
feature using old reloptions tools. I totally do not want to do it 

The 3rd functionality came from philosophy one relation-type -- one options 
catalog, that was implemented in my reloptions patch. It is not really needed 
in master without the full patch. With some efforts I think it can be made as a 
separate patch, thought I would also try to avoid it if possible.


So the questions still is: should we commit not existent functionality tests 
commented, uncomented but with no proper error response in expected output, or 
just remove these tests from this patch?

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

  2. Re: [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2017-09-30T20:18:35Z

    While working with reloptions refactoring patch, I've written series of tests 
that triggers reloptions related code in all access methods. (I needed it to 
make sure I did not break anything while coding)

I've included these tests to that patch.

Meanwhile Alvaro suggested to commit these tests before the main patch, in 
order to make sure, that this patch does not change usual behavior.

So these tests separated from reloptions patch is in the attachment.

I've removed tests that check functionality that were introduced only in my 
patch, and kept those that checks things that are already in postgres.

I also compared test coverage before and after applying this patch 
(You can also compare, I put coverage results online
http://lj.nataraj.su/2017/reloptions_fix/coverage-master/
http://lj.nataraj.su/2017/reloptions_fix/coverage-patched/ )

Tests adds almost 600 lines to the test covered code (but see note at the end 
of the letter)

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...

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

I think we should commit it.







  

 

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

  3. Re: [PATCH] Tests for reloptions

    Michael Paquier <michael.paquier@gmail.com> — 2017-10-03T02:48:43Z

    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

  4. Re: [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2017-10-06T09:27:32Z

    В письме от 3 октября 2017 11:48:43 пользователь Michael Paquier написал:

> > 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.
Might be... As I have noticed, tests  for indexes filled with random data test 
coverage, gives random coverage. There needed more random data to give steady 
results. I am gathering statistics now, and later I hope I will offer another 
patch(es) to fix it. So not all of 58 lines might be result of this patch :-)
 
> +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?
I actually run only simple "make check". running "make check" for bloom will 
also add some add_ routines to coverage.


> > 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.

I've been thinking a lot, and rereading the patch. When I reread it I've been 
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 
with you. But I've been thinking that tests also should check that everything 
behaves as expected (or as written in documentation).

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

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



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


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

  5. Re: [PATCH] Tests for reloptions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-10-19T12:20:52Z

    Nikolay Shaplov wrote:
> В письме от 3 октября 2017 11:48:43 пользователь Michael Paquier написал:

> I've been thinking a lot, and rereading the patch. When I reread it I've been 
> 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 
> with you. But I've been thinking that tests also should check that everything 
> behaves as expected (or as written in documentation).
> 
> I would say that options names is a of part of SQL dialect that postgres uses, 
> kind of part of the syntax. It is good to be sure that they still supported. 
> So I would add a test for every option heap supports.

Yeah, it would perhaps be good idea to ensure we don't break things that
are documented to work.  If the tests don't take too long, I'm not
opposed to testing every single option.  As you say, code coverage is
important but it's not the only goal.

I'm hesitant to hardcode things like the number of bits in bloom, as you
had in the original.  If I understand correctly, that number could
change with compile options (different blocksize?), so I removed that
part.  I also fixed a few spelling errors.

And pushed.  Let's see what the buildfarm says about this.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: [PATCH] Tests for reloptions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-10-19T12:27:53Z

    Oh, one more thing: be careful when editing parallel_schedule.  There
are constraints on the number of entries in each group; you had added a
20th entry after the comment that the group can only have 19.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2017-10-22T13:24:30Z

    В письме от 19 октября 2017 14:20:52 Вы написали:

> I'm hesitant to hardcode things like the number of bits in bloom, as you
> had in the original.  If I understand correctly, that number could
> change with compile options (different blocksize?), so I removed that
> part.  

#define MAX_BLOOM_LENGTH		(256 * SIGNWORDBITS)

#define SIGNWORDBITS ((int) (BITS_PER_BYTE * sizeof(BloomSignatureWord)))

typedef uint16 BloomSignatureWord;

Here everything is based on uint16, and it is platform independent, as far as 
I can get.

But this is not really important now... 


> I also fixed a few spelling errors.
Thank you. I am not so good with natural languages :-)

> And pushed.
Thanx!

> Let's see what the buildfarm says about this.
It seems to me that it is quite happy about these tests :-)

> Oh, one more thing: be careful when editing parallel_schedule.  There
> are constraints on the number of entries
Oh, I did not payed attention to this issue, through it it mentioned in  
parallel_schedule comments. I've added it to the wiki 
https://wiki.postgresql.org/wiki/Regression_test_authoring So it was all 
described in one place.


> in each group; you had added a
> 20th entry after the comment that the group can only have 19.
Oups it was definitely my mistake. I should be more attentive... :-( 


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

  8. Re: [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2017-10-29T14:08:29Z

    В письме от 19 октября 2017 14:20:52 Вы написали:

> Yeah, it would perhaps be good idea to ensure we don't break things that
> are documented to work.  If the tests don't take too long, I'm not
> opposed to testing every single option.  As you say, code coverage is
> important but it's not the only goal.
> 
> I'm hesitant to hardcode things like the number of bits in bloom, as you
> had in the original.  If I understand correctly, that number could
> change with compile options (different blocksize?), so I removed that
> part.  I also fixed a few spelling errors.
> 
> And pushed.  Let's see what the buildfarm says about this.
While merging this commit to my branch, I found two issues that as I think 
needs fixing. Hope this does not require creating new commit request...

First is missing tab.

Second i think it is better to write "The OIDS option is not stored as 
_reloption_" otherwise it cat be read as if it is not stored at all.

See patch in the attachment.

Thank you again for your work with the patch. I've seen how much you have 
change it.

PS do I get right that 80 character code width rule is applied to SQL tests 
too?

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

  9. Re: [HACKERS] [PATCH] Tests for reloptions

    Nikolay Shaplov <dhyan@nataraj.su> — 2018-01-22T20:14:56Z

    В письме от 29 октября 2017 17:08:29 пользователь Nikolay Shaplov написал:
 
> While merging this commit to my branch, I found two issues that as I think
> needs fixing. Hope this does not require creating new commit request...

To draw more attention to this issue, I've filed it as a new thread
https://www.postgresql.org/message-id/3951112.MkJ1MAZpIQ%40x200m
with a entry for next commit fest.
Hope this will help fixing missing tab.


-- 
Do code for fun.