Обсуждение: [HACKERS] additional contrib test suites
Here are some small test suites for some contrib modules as well as pg_archivecleanup that didn't have one previously, as well as one patch to improve code coverage in a module. Will add to commit fest. Testing on different platforms and with different build configurations would be useful. -- Peter Eisentraut http://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
Вложения
On Sat, Aug 12, 2017 at 1:20 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here are some small test suites for some contrib modules as well as > pg_archivecleanup that didn't have one previously, as well as one patch > to improve code coverage in a module. > > Will add to commit fest. Testing on different platforms and with > different build configurations would be useful. After applying these patches cleanly on top of 0b554e4e63a4ba4852c01951311713e23acdae02 and running "./configure --enable-tap-tests --with-tcl --with-python --with-perl --with-ldap --with-icu && make && make check-world" I saw this failure: cd . && TESTDIR='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_archivecleanup' PATH="/home/travis/build/postgresql-cfbot/postgresql/tmp_install/usr/local/pgsql/bin:$PATH" LD_LIBRARY_PATH="/home/travis/build/postgresql-cfbot/postgresql/tmp_install/usr/local/pgsql/lib" PGPORT='65432' PG_REGRESS='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_archivecleanup/../../../src/test/regress/pg_regress' /usr/bin/prove -I ../../../src/test/perl/ -I . t/*.pl t/010_pg_archivecleanup.pl .. 1/42 # Failed test 'fails if restart file name is not specified: matches' # at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_archivecleanup/../../../src/test/perl/TestLib.pm line 330. # 'pg_archivecleanup: must specify oldest kept WAL file # Try "pg_archivecleanup --help" for more information. # ' # doesn't match '(?^:must specify restartfilename)' # Failed test 'fails with too many parameters: matches' # at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_archivecleanup/../../../src/test/perl/TestLib.pm line 330. # 'pg_archivecleanup: too many command-line arguments # Try "pg_archivecleanup --help" for more information. # ' # doesn't match '(?^:too many parameters)' # Failed test 'fails with invalid restart file name: matches' # at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_archivecleanup/../../../src/test/perl/TestLib.pm line 330. # 'pg_archivecleanup: invalid file name argument # Try "pg_archivecleanup --help" for more information. # ' # doesn't match '(?^:invalid filename)' # Looks like you failed 3 tests of 42. t/010_pg_archivecleanup.pl .. Dubious, test returned 3 (wstat 768, 0x300) Failed 3/42 subtests Test Summary Report ------------------- t/010_pg_archivecleanup.pl (Wstat: 768 Tests: 42 Failed: 3) Failed tests: 12, 16, 18 Non-zero exit status: 3 Files=1, Tests=42, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.05 cusr0.00 csys = 0.08 CPU) Result: FAIL -- Thomas Munro http://www.enterprisedb.com
On 9/6/17 07:11, Thomas Munro wrote: > After applying these patches cleanly on top of > 0b554e4e63a4ba4852c01951311713e23acdae02 and running "./configure > --enable-tap-tests --with-tcl --with-python --with-perl --with-ldap > --with-icu && make && make check-world" I saw this failure: Yes, some of the error messages had changed. Fixed patches attached. -- Peter Eisentraut http://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
Вложения
On 9/8/17 1:32 PM, Peter Eisentraut wrote: > > Yes, some of the error messages had changed. Fixed patches attached. Patches apply and all tests pass. A few comments: * [PATCH v2 1/7] adminpack: Add test suite There are no regular tests for pg_logdir_ls(). It looks like TAP tests would be required but I'm not sure it's worth it. The fact that the "default" log name format is hard-coded in is, um, interesting. Maybe add: + SELECT pg_logdir_ls(); + ERROR: could not read directory "log": No such file or directory to get a little more coverage? It would be good to at least have a note on why it is not tested. * [PATCH v2 4/7] chkpass: Add test suite Well, this is kind of scary: +CREATE EXTENSION chkpass; +WARNING: type input function chkpass_in should not be volatile I guess the only side effect is that this column cannot be indexed? The docs say that, so OK, but is there anything else a user should be worried about? The rest looks good. I'll mark this "Ready for Committer" since I'm the only reviewer. I don't think anything you might add based on my comments above requires a re-review. As for testing on more platforms, send it to the build farm? -- -David david@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/14/17 11:01, David Steele wrote: > On 9/8/17 1:32 PM, Peter Eisentraut wrote: >> >> Yes, some of the error messages had changed. Fixed patches attached. > > Patches apply and all tests pass. A few comments: > > * [PATCH v2 1/7] adminpack: Add test suite > > There are no regular tests for pg_logdir_ls(). Added a comment about that. > * [PATCH v2 4/7] chkpass: Add test suite > > Well, this is kind of scary: > > +CREATE EXTENSION chkpass; > +WARNING: type input function chkpass_in should not be volatile > > I guess the only side effect is that this column cannot be indexed? The > docs say that, so OK, but is there anything else a user should be > worried about? Well, we're just testing, not judging. ;-) > The rest looks good. I'll mark this "Ready for Committer" since I'm the > only reviewer. I don't think anything you might add based on my > comments above requires a re-review. > > As for testing on more platforms, send it to the build farm? OK, committed, let's see. -- Peter Eisentraut http://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
On 9/14/17 22:47, Peter Eisentraut wrote: >> As for testing on more platforms, send it to the build farm? > OK, committed, let's see. So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt() doesn't support the traditional two-character salt format. Option: - Use the resultmap features to make this an expected failure on OpenBSD. - Fix the module to work on OpenBSD. This would involve making a platform-specific modification to use whatever advanced salt format they want. - Replace the entire module with something that does not depend on crypt(). -- Peter Eisentraut http://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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt()
> doesn't support the traditional two-character salt format.
> Option:
> - Use the resultmap features to make this an expected failure on OpenBSD.
> - Fix the module to work on OpenBSD. This would involve making a
> platform-specific modification to use whatever advanced salt format they
> want.
> - Replace the entire module with something that does not depend on crypt().
Or (4) drop the module's regression test again.
I'd go for (1) at least as a short-term answer. It's not clear to me
that it's worth the effort to do (2) or (3). Also, (3) probably breaks
backwards compatibility, if there is anyone out there actually using
this datatype.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt()
>> doesn't support the traditional two-character salt format.
>> Option:
>> - Use the resultmap features to make this an expected failure on OpenBSD.
>> - Fix the module to work on OpenBSD. This would involve making a
>> platform-specific modification to use whatever advanced salt format they
>> want.
>> - Replace the entire module with something that does not depend on crypt().
> Or (4) drop the module's regression test again.
Noting that mandrill is showing yet a different failure, one that I think
is inherent to chkpass:
CREATE TABLE test (i int, p chkpass); INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye');
+ WARNING: type chkpass has unstable input conversion for "hello"
+ LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye');
+ ^
+ WARNING: type chkpass has unstable input conversion for "goodbye"
+ LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye');
+ ^
I'm starting to think that (4) might be the best avenue. Or we could
consider
(5) drop contrib/chkpass altogether, on the grounds that it's too badly
designed, and too obsolete crypto-wise, to be useful or supportable.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Sep 16, 2017 at 5:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt() >>> doesn't support the traditional two-character salt format. > >>> Option: > >>> - Use the resultmap features to make this an expected failure on OpenBSD. > >>> - Fix the module to work on OpenBSD. This would involve making a >>> platform-specific modification to use whatever advanced salt format they >>> want. > >>> - Replace the entire module with something that does not depend on crypt(). > >> Or (4) drop the module's regression test again. > > Noting that mandrill is showing yet a different failure, one that I think > is inherent to chkpass: > > CREATE TABLE test (i int, p chkpass); > INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); > + WARNING: type chkpass has unstable input conversion for "hello" > + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); > + ^ > + WARNING: type chkpass has unstable input conversion for "goodbye" > + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); > + ^ > > I'm starting to think that (4) might be the best avenue. Or we could > consider > > (5) drop contrib/chkpass altogether, on the grounds that it's too badly > designed, and too obsolete crypto-wise, to be useful or supportable. crypt() uses the 7 lowest characters, which makes for 7.2e16 values, so I would be fine with (5), then (4) as the test suite is not portable. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/15/17 6:52 PM, Michael Paquier wrote: > On Sat, Sep 16, 2017 at 5:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Noting that mandrill is showing yet a different failure, one that I think >> is inherent to chkpass: >> >> CREATE TABLE test (i int, p chkpass); >> INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); >> + WARNING: type chkpass has unstable input conversion for "hello" >> + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); >> + ^ >> + WARNING: type chkpass has unstable input conversion for "goodbye" >> + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); >> + ^ >> >> I'm starting to think that (4) might be the best avenue. Or we could >> consider >> >> (5) drop contrib/chkpass altogether, on the grounds that it's too badly >> designed, and too obsolete crypto-wise, to be useful or supportable. > > crypt() uses the 7 lowest characters, which makes for 7.2e16 values, > so I would be fine with (5), then (4) as the test suite is not > portable. I'd prefer 5, but can go with 4. I get that users need to store their own passwords, but we have support for SHA1 via the crypto module which seems by far the better choice. -- -David david@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/16/17 08:10, David Steele wrote: >>> (5) drop contrib/chkpass altogether, on the grounds that it's too badly >>> designed, and too obsolete crypto-wise, to be useful or supportable. >> crypt() uses the 7 lowest characters, which makes for 7.2e16 values, >> so I would be fine with (5), then (4) as the test suite is not >> portable. > I'd prefer 5, but can go with 4. > > I get that users need to store their own passwords, but we have support > for SHA1 via the crypto module which seems by far the better choice. I'm also tempted to just remove it. It uses bad/outdated security practices and it's also not ideal as an example module. Any objections? -- Peter Eisentraut http://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
On 9/18/17 09:54, Peter Eisentraut wrote: > On 9/16/17 08:10, David Steele wrote: >>>> (5) drop contrib/chkpass altogether, on the grounds that it's too badly >>>> designed, and too obsolete crypto-wise, to be useful or supportable. >>> crypt() uses the 7 lowest characters, which makes for 7.2e16 values, >>> so I would be fine with (5), then (4) as the test suite is not >>> portable. >> I'd prefer 5, but can go with 4. >> >> I get that users need to store their own passwords, but we have support >> for SHA1 via the crypto module which seems by far the better choice. > > I'm also tempted to just remove it. It uses bad/outdated security > practices and it's also not ideal as an example module. Any objections? Hearing none, thus removed. -- Peter Eisentraut http://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
On 2017-09-18 09:54:52 -0400, Peter Eisentraut wrote: > On 9/16/17 08:10, David Steele wrote: > >>> (5) drop contrib/chkpass altogether, on the grounds that it's too badly > >>> designed, and too obsolete crypto-wise, to be useful or supportable. > >> crypt() uses the 7 lowest characters, which makes for 7.2e16 values, > >> so I would be fine with (5), then (4) as the test suite is not > >> portable. > > I'd prefer 5, but can go with 4. > > > > I get that users need to store their own passwords, but we have support > > for SHA1 via the crypto module which seems by far the better choice. > > I'm also tempted to just remove it. It uses bad/outdated security > practices and it's also not ideal as an example module. Any objections? Uhm. I'm not objecting, but I doubt people really noticed your question in a thread about additional contrib test suites. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers