On Thu, May 9, 2013 at 4:29 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: > This updated version works for me and addresses previous comments. > > I think that such tests are definitely valuable, especially as many corner > cases which must trigger errors are covered. > > I recommend to apply it.
I'm attaching an update of this patch that renames both the new test file (user->role), and the regression users that get created. It also fixes the serial schedule to match the parallel schedule. However, before it can get committed, I think this set of tests needs streamlining. It does seem to me valuable, but I think it's wasteful in terms of runtime to create so many roles, do just one thing with them, and then drop them. I recommend consolidating some of the tests. For example: +-- Should work. ALTER ROLE with (UN)ENCRYPTED PASSWORD +CREATE ROLE regress_rol_rol14; +ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD 'abc'; +DROP ROLE regress_rol_rol14; +CREATE ROLE regress_rol_rol15; +ALTER ROLE regress_rol_rol15 WITH UNENCRYPTED PASSWORD 'abc'; +DROP ROLE regress_rol_rol15; + +-- Should fail. ALTER ROLE with (UN)ENCRYPTED PASSWORD but no password value +CREATE ROLE regress_rol_rol16; +ALTER ROLE regress_rol_rol16 WITH ENCRYPTED PASSWORD; +DROP ROLE regress_rol_rol16; +CREATE ROLE regress_rol_rol17; +ALTER ROLE regress_rol_rol17 WITH UNENCRYPTED PASSWORD; +DROP ROLE regress_rol_rol17; + +-- Should fail. ALTER ROLE with both UNENCRYPTED and ENCRYPTED +CREATE ROLE regress_rol_rol18; +ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc'; +DROP ROLE regress_rol_rol18; There's no reason that couldn't be written this way: CREATE ROLE regress_rol_rol14; ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD 'abc'; ALTER ROLE regress_rol_rol14 WITH UNENCRYPTED PASSWORD 'abc'; ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD; ALTER ROLE regress_rol_rol14 WITH UNENCRYPTED PASSWORD; ALTER ROLE regress_rol_rol14 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc'; DROP ROLE regress_rol_rol14; Considering the concerns already expressed about the runtime of the test, I think it's important to minimize the number of create/drop role cycles that the tests perform. Generally, I think that the tests which return a syntax error are of limited value and should probably be dropped. That is unlikely to get broken by accident. If the syntax error is being thrown by something outside of bison proper, that's probably worth testing. But I think that testing random syntax variations is a pretty low-value proposition. Setting this to "Waiting on Author". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
regress_role_v3.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers