Hi Stephen, On Tue, Sep 04, 2018 at 04:14:15PM -0700, Michael Paquier wrote: > On Tue, Sep 04, 2018 at 06:02:51PM -0400, Stephen Frost wrote: >> * Tom Lane (t...@sss.pgh.pa.us) wrote: >>> I'm confused. Isn't the point of that script exactly to create a modified >>> extension for testing pg_dump with? > > Not really, the regression tests run for pg_regress and the TAP test > suite are two completely isolated things and share no dependencies. > e54f757 has actually changed test_pg_dump.sql so as it adds regression > tests for pg_init_privs for ALTER EXTENSION ADD/DROP, and visibly those > have been added to test_pg_dump because they were easier to add there, > and this has no interactions with pg_dump. What I think should have > been done initially is to add those new tests in test_extensions > instead.
I am able to come back to this thread, and I still don't grep from where sql/test_pg_dump.sql is called. Stephen, if the test suite is aiming at tracking that pg_init_privs is correctly set up with ALTER EXTENSION ADD/DROP, shouldn't it also query the catalog to make sure that the correct entries are added and removed after running the so-said command? At least that's one way I could see to perform the necessary sanity checks without running directly pg_dump. Perhaps I am missing something? >>> What I'd do is leave the final state as-is and add a "drop role if exists" >>> at the start, similar to what some of the core regression tests do. >> >> I've not followed this thread but based on Tom's response, I agree with >> his suggestion of what to do here. > > Not as far as I can see.. Please note that using installcheck on the > main regression test suite does not leave around any extra roles. I can > understand the role of having a DROP ROLE IF EXISTS though: if you get a > crash while testing, then the beginning of the tests are repeatable, so > independently of the root issue Tom's suggestion makes sense to me. Attached is a patch with more comments about the intents of the test suite, and the separate issue pointed out by Tom fixed. It seems to me that actually checking the contents of pg_init_privs would improve the reason why the test exists.. I would welcome input about this last point. -- Michael
diff --git a/src/test/modules/test_pg_dump/expected/test_pg_dump.out b/src/test/modules/test_pg_dump/expected/test_pg_dump.out index c9bc6f7625..ad730f18fd 100644 --- a/src/test/modules/test_pg_dump/expected/test_pg_dump.out +++ b/src/test/modules/test_pg_dump/expected/test_pg_dump.out @@ -1,3 +1,11 @@ +-- +-- Set of regression tests to test that ALTER EXTENSION ADD/DROP +-- handles correctly pg_init_privs. +-- +-- Clean up in case a prior regression run failed +SET client_min_messages TO 'warning'; +DROP ROLE IF EXISTS regress_dump_test_role; +RESET client_min_messages; CREATE ROLE regress_dump_test_role; CREATE EXTENSION test_pg_dump; ALTER EXTENSION test_pg_dump ADD DATABASE postgres; -- error @@ -92,3 +100,12 @@ ALTER EXTENSION test_pg_dump DROP SERVER s0; ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1; ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1; ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1; +-- Clean up +DROP EXTENSION test_pg_dump; +DROP SCHEMA test_pg_dump_s1; +DROP MATERIALIZED VIEW test_pg_dump_mv1; +DROP FOREIGN TABLE ft1; +DROP SERVER s0; +DROP TYPE test_pg_dump_e1; +DROP FUNCTION test_pg_dump(integer); +DROP ROLE regress_dump_test_role; diff --git a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql index e463dec404..1becc2b609 100644 --- a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql +++ b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql @@ -1,3 +1,13 @@ +-- +-- Set of regression tests to test that ALTER EXTENSION ADD/DROP +-- handles correctly pg_init_privs. +-- + +-- Clean up in case a prior regression run failed +SET client_min_messages TO 'warning'; +DROP ROLE IF EXISTS regress_dump_test_role; +RESET client_min_messages; + CREATE ROLE regress_dump_test_role; CREATE EXTENSION test_pg_dump; @@ -105,3 +115,13 @@ ALTER EXTENSION test_pg_dump DROP SERVER s0; ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1; ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1; ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1; + +-- Clean up +DROP EXTENSION test_pg_dump; +DROP SCHEMA test_pg_dump_s1; +DROP MATERIALIZED VIEW test_pg_dump_mv1; +DROP FOREIGN TABLE ft1; +DROP SERVER s0; +DROP TYPE test_pg_dump_e1; +DROP FUNCTION test_pg_dump(integer); +DROP ROLE regress_dump_test_role;
signature.asc
Description: PGP signature
