Thread: [HACKERS] [PATCH] Tests for reloptions
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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.