test_pg_dump missing cleanup actions
Hi Stephen,
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.
Thanks,
--
Michael
Attachments:
test-pg-dump-cleanup.patchtext/x-diff; charset=us-asciiDownload
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..8d7b0ec926 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
@@ -92,3 +92,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..2ceb84d223 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
@@ -105,3 +105,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;
Michael Paquier <michael@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?
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.
regards, tom lane
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Michael Paquier <michael@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?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.
Thanks!
Stephen
On Tue, Sep 04, 2018 at 06:02:51PM -0400, Stephen Frost wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Michael Paquier <michael@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
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 (tgl@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
Attachments:
test-pg-dump-cleanup-2.patchtext/x-diff; charset=us-asciiDownload
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;
On Thu, Sep 06, 2018 at 09:20:15AM -0700, Michael Paquier wrote:
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.
So, Stephen, any input to offer? This has been around for the last
three weeks. I am tracking this thread in the section for older bugs in
v11 open items.
--
Michael
Michael,
* Michael Paquier (michael@paquier.xyz) wrote:
On Thu, Sep 06, 2018 at 09:20:15AM -0700, Michael Paquier wrote:
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.So, Stephen, any input to offer? This has been around for the last
three weeks. I am tracking this thread in the section for older bugs in
v11 open items.
It's on my list of things to look at and hope to do so soon.
Thanks!
Stephen
Hi Stephen,
On Thu, Sep 27, 2018 at 06:49:26PM -0400, Stephen Frost wrote:
* Michael Paquier (michael@paquier.xyz) wrote:
On Thu, Sep 06, 2018 at 09:20:15AM -0700, Michael Paquier wrote:
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.So, Stephen, any input to offer? This has been around for the last
three weeks. I am tracking this thread in the section for older bugs in
v11 open items.It's on my list of things to look at and hope to do so soon.
So it has already been two months. Do you have any input to offer or
should I do the cleanup myself? If none, then I still propose my
cleanup patch from upthread, and I propose to apply it within the next
couple of days if there are no objections.
The annoying part about this stuff is that when running tests on Windows
we get failures after multiple runs on the same installed instance, and
the module is not marked with NO_INSTALLCHECK. I just got annoyed by
that test a portion of this week to test some other patch with MSVC as
well :(
--
Michael
Michael,
* Michael Paquier (michael@paquier.xyz) wrote:
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 (tgl@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 is perhaps not clear is that the pg_init_privs work was all
entirely, specifically, for pg_dump to then work with and leverage to
only export out the correct set of privileges. Testing *just*
pg_init_privs without also seeing what pg_dump does with values that end
up in that table wouldn't really be a complete test that things are
actually working.
Now, that test_pg_dump script with all of the various ALTER EXTENSION
ADD things that it's doing with access methods and aggregates and such,
which were quite intentionally left behind, were for testing
*pg_upgrade* as well and that happens to also test pg_dump since
pg_upgrade mostly just wraps pg_dump. Note that the pg_upgrade test
runs installcheck which is what's installing all of those various things
into the database to test pg_upgrade/pg_dump with.
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.
Now, all that said, the TAP tests for test_pg_dump also create the
extension and then perform further GRANTs and such there and then check
the results of doing a pg_dump.
Hopefully the above helps explain where these are used and why we do
*not* want to have all of those DROP commands at the end to 'clean up'-
we'd be removing a bunch of important testing that we're getting thanks
to pg_upgrade.
In short, let's please not apply this patch. To make it repeatable, we
could have DROP EXTENSION (et al) IF EXISTS at the top of the script,
similar to the DROP ROLE IF EXISTS that was suggested. I'm not sure if
we have all the necessary DROP .. IF EXISTS options though today.. :/
Thanks!
Stephen