On Tue, Sep 04, 2018 at 06:02:51PM -0400, Stephen Frost wrote: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Michael Paquier <mich...@paquier.xyz> writes: >>> While hacking another patch, I have noticed that triggerring multiple >>> times in a row installcheck on test_pg_dump results in a failure because >>> it is missing clean up actions on the role regress_dump_test_role. >>> Roles are shared objects, so I think that we ought to not let traces of >>> it when doing any regression tests on a running instance. >> >>> Attached is a patch to clean up things. >> >> 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. >> 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. -- Michael
signature.asc
Description: PGP signature
