Horton

Thread

Commits

  1. Remove contrib/chkpass

  2. doc: Clarify pg_inherits description

  1. additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-12T01:20:37Z

    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

  2. Re: additional contrib test suites

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-06T11:11:03Z

    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 cusr
 0.00 csys =  0.08 CPU)
Result: FAIL

-- 
Thomas Munro
http://www.enterprisedb.com

  3. Re: additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-08T17:32:46Z

    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

  4. Re: additional contrib test suites

    David Steele <david@pgmasters.net> — 2017-09-14T15:01:06Z

    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

  5. Re: additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-15T02:47:21Z

    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

  6. Re: additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-15T12:51:43Z

    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

  7. Re: additional contrib test suites

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T13:49:43Z

    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

  8. Re: additional contrib test suites

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T20:15:04Z

    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

  9. Re: additional contrib test suites

    Michael Paquier <michael.paquier@gmail.com> — 2017-09-15T22:52:35Z

    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

  10. Re: additional contrib test suites

    David Steele <david@pgmasters.net> — 2017-09-16T12:10:38Z

    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

  11. Re: additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-18T13:54:52Z

    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

  12. Re: additional contrib test suites

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-22T15:50:48Z

    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

  13. Re: additional contrib test suites

    Andres Freund <andres@anarazel.de> — 2017-09-22T17:21:09Z

    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