Re: [HACKERS] Regression tests vs existing users in an installation
On Mon, Jul 18, 2016 at 11:13 AM, Tom Lane wrote: >> I don't particularly like your suggestion of spooky action at a >> distance between force_parallel_mode and regression_test_mode. That >> just seems kooky. > > It's certainly a judgment call as to which way is cleaner, but I don't > understand your objection. There are plenty of ways in which multiple > GUCs determine a given behavior already. Also, breaking this behavior > into two variables would let us document the user-useful behavior (do > this to test parallel safety of functions) in a different place from the > developer-useful behavior (do this to make EXPLAIN lie to you, which > surely has no possible use except for regression testing). There are certainly cases where the behavior that you get depends on multiple GUCs. For example, the vacuum cost limit stuff is like that, and so are the cost factors which control the query planner. But in each of those cases, each GUC has a function that is fully orthogonal to each other GUC. That doesn't seem to be the case here. You're saying that force_parallel_mode will decide whether we get behavior A, and regression_test_mode will decide whether we get behavior B, but if you ask for both A and B you will also get an additional behavior C which would not have been selected by either GUC taken alone. Because the different settings are now non-orthogonal, there's now no way to get A and B without C, or A and C without B. Moreover, I don't want to end up in a situation where regression_test_mode=on enables a score of minor behavior changes that can't be separated out and tested individually. It's true that checking the names of regression roles is such a very minor thing that a generic name like regression_test_mode might be OK, with the idea that anything else similarly minor that comes along later can be folded into that as well. But I fear that it will become a crutch: my code makes the regression test fail. Instead of writing better tests, add another thing that's conditional on regression_test_mode! Eventually, we'll have a bug that the regression tests fail to catch because regression_test_mode papers over the problem. We're some distance from such a situation now, of course, but I bet the temptation to be undisciplined about hooking more behavior into that GUC will be almost irresistible for new developers, and in some cases experienced ones, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
On Mon, Jul 18, 2016 at 1:34 AM, Michael Paquier wrote: > One downside of the plugin is that any users willing to do make > installcheck would need to install it as well. Not really. If the only purpose of the plugin is to verify that we're not creating regression users whose names don't start with "regress", it should be good enough to run it for "make check" but not for "make installcheck". It's not there to test functionality, just to verify that we've followed our own rules for regression tests. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Peter Eisentraut writes: > On 7/15/16 6:13 PM, Tom Lane wrote: >> We've talked before about how the regression tests should be circumspect >> about what role names they create/drop, so as to avoid possibly blowing >> up an installation's existing users during "make installcheck". > I'm not particularly sure that that is a useful goal anymore. Setting > up a new instance is cheap, so if users are concerned, they should not > run the tests against their important instance. To my mind, the main point of "make installcheck" is to verify that your actual installation, not some other instance, is working right. This is far from a trivial issue; for instance on a SELinux machine, you need to be able to verify that the installed policy allows the DB to work, and that is very likely to be path-sensitive. So this remains an important requirement to me. It's true that it might be something you need to run only right after making the installation --- but that doesn't mean the DB is empty. Consider wanting to test a freshly pg_upgrade'd installation, for example. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
On 7/15/16 6:13 PM, Tom Lane wrote: > We've talked before about how the regression tests should be circumspect > about what role names they create/drop, so as to avoid possibly blowing > up an installation's existing users during "make installcheck". I'm not particularly sure that that is a useful goal anymore. Setting up a new instance is cheap, so if users are concerned, they should not run the tests against their important instance. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Robert Haas writes: > On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane wrote: >> I'm coming to the conclusion that the only thing that will make this >> materially better in the long run is automatic enforcement of a convention >> about what role names may be created in the regression tests. See my >> response to Stephen just now for a concrete proposal. > We could also do this by loading a C module during the regression > tests, which seems maybe less ugly than adding a GUC. Meh, I'm not convinced. As Michael points out, arranging for such a module to get loaded in an installcheck context would be difficult --- maybe not impossible, but complicated. Also, we'd have to add hook function calls in all the places it would need to get control; most of those places would probably be one-off hooks with no other conceivable use. And we'd still need to have a GUC, because I think it's inevitable that we'd need to be able to turn off the restrictions for specific tests. So that seems like a lot of work and complication just to make a GUC be custom to some undocumented extension rather than built-in. If we had no other debugging GUCs then there might be some point in rejecting this one, but we have a bunch: https://www.postgresql.org/docs/devel/static/runtime-config-developer.html > I don't particularly like your suggestion of spooky action at a > distance between force_parallel_mode and regression_test_mode. That > just seems kooky. It's certainly a judgment call as to which way is cleaner, but I don't understand your objection. There are plenty of ways in which multiple GUCs determine a given behavior already. Also, breaking this behavior into two variables would let us document the user-useful behavior (do this to test parallel safety of functions) in a different place from the developer-useful behavior (do this to make EXPLAIN lie to you, which surely has no possible use except for regression testing). Possibly a single "regression_test_mode" variable is a bad idea and we should instead have distinct developer-oriented GUCs for each special behavior we decide we need. I'm not particularly set on that, but to me it seems like less of a mess to have just one. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
On Mon, Jul 18, 2016 at 10:37 AM, Robert Haas wrote: > On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane wrote: > We could also do this by loading a C module during the regression > tests, which seems maybe less ugly than adding a GUC. > I don't particularly like your suggestion of spooky action at a > distance between force_parallel_mode and regression_test_mode. That > just seems kooky. One downside of the plugin is that any users willing to do make installcheck would need to install it as well. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane wrote: > I'm coming to the conclusion that the only thing that will make this > materially better in the long run is automatic enforcement of a convention > about what role names may be created in the regression tests. See my > response to Stephen just now for a concrete proposal. We could also do this by loading a C module during the regression tests, which seems maybe less ugly than adding a GUC. I don't particularly like your suggestion of spooky action at a distance between force_parallel_mode and regression_test_mode. That just seems kooky. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
I've gone ahead and pushed a patch that does all of the cosmetic renamings needed to clean up the global-object-names situation. I've not done anything yet about those special cases in the rolenames test, since it's open for discussion exactly what to do there. I figured that this patch was bulky enough, and mechanical enough, that there wasn't much point in putting it up for review; the buildfarm will do a lot better at finding any mistakes I may have made. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Alvaro Herrera writes: > Tom Lane wrote: >> We've talked before about how the regression tests should be circumspect >> about what role names they create/drop, so as to avoid possibly blowing >> up an installation's existing users during "make installcheck". In >> particular I believe there was consensus that such names should begin >> with, or at least include, "regress". I got around today to instrumenting >> CreateRole to see what names we were actually creating, and was quite >> depressed as to how thoroughly that guideline is being ignored (see >> attached). > I would propose that we have one test run near the beginning or right at > the beginning of the serial schedule that sets up a small number of > roles to cover most of the needs of every other test, so that most such > other tests do not need to create any roles at all. I don't think that's a very attractive idea. It would create hazards for concurrent test cases, I fear. Moreover, an un-enforced convention of "don't create roles" isn't really any safer than an un-enforced convention of "only create roles named thus-and-such"; it just takes one person who is not familiar with the convention, and one committer not paying close attention, and we're right back where we started. I'm coming to the conclusion that the only thing that will make this materially better in the long run is automatic enforcement of a convention about what role names may be created in the regression tests. See my response to Stephen just now for a concrete proposal. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> One could certainly argue that these are safe enough because nobody would >> ever create real roles by those names anyway. I'm not very comfortable >> with that though; if we believe that, why did we go to the trouble of >> making sure these cases work? > Sadly, it's not quite so simple: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831234 Hah. Well, I have zero interest in supporting "user" as the name of the bootstrap superuser. Even if that seemed like a good idea, why not also current_user, session_user, Public, or any other name we might want to use in the tests? The variant-output-files answer definitely doesn't scale. What seems a more useful approach is for packagers to not allow the O/S username to affect the results of "make check". initdb already has the --username switch to override its choice of the bootstrap superuser name, and pg_regress has a --user switch, so in principle it should be possible for a package to ensure that its build tests are run with the standard superuser name rather than something dependent on the environment. I'm not sure how *easy* it is, mind you. We might want to add some Makefile plumbing to make it easier to do that. But that's not what I'm on about today ... >> A more aggressive answer would be to decide we don't need these test cases >> at all and drop them. An advantage of that is that then we could >> configure some buildfarm animal to fail the next time somebody ignores >> the "test role names should contain 'regress'" rule. > I'd really like to have more test coverage rather than less, so I'd find > this a bit hard to swallow, but it might still be better than the > alternatives. As Greg mentioned, we could improve things if we were willing to invent something like a "regression_test_mode" GUC. The rough sketch would be: 1. pg_regress adds "regression_test_mode = on" to the ALTER DATABASE SET settings it already applies to created databases. 2. One of the effects of the GUC would be to throw an error (or warning?) for creation of disallowed-by-convention role names. 3. The rolenames test could turn off the GUC during creation of these specific test names. Or if we make it report WARNING not ERROR, we don't even have to do that, just include the warnings in the expected output. I find myself liking this idea, because there would be other uses for such a GUC. One thing that is awful darn tempting right now is to get rid of the "force_parallel_mode = regress" wart, making that variable back into a plain bool, and instead have that behavior emerge from having both force_parallel_mode and regression_test_mode turned on. We'd still want creation of these specific role names to be wrapped in a rolled-back transaction, so that we could document that only role names beginning with "regress_" are at hazard from "make installcheck". But I don't think that doing that really represents any meaningful loss of coverage. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > We've talked before about how the regression tests should be circumspect > about what role names they create/drop, so as to avoid possibly blowing > up an installation's existing users during "make installcheck". In > particular I believe there was consensus that such names should begin > with, or at least include, "regress". I got around today to instrumenting > CreateRole to see what names we were actually creating, and was quite > depressed as to how thoroughly that guideline is being ignored (see > attached). I had started in on this but hadn't made as much progress as I had hoped, so I'm glad to see that you're taking a look at it. > I propose to go through the regression tests and fix this (in HEAD only). > However, there's one place where it's not so easy to just substitute a > different name, because rolenames.sql is intentionally doing this: > > CREATE ROLE "Public"; > CREATE ROLE "None"; > CREATE ROLE "current_user"; > CREATE ROLE "session_user"; > CREATE ROLE "user"; > > in order to test whether we properly distinguish role-related keywords > from quoted identifiers. Obviously, modifying these would defeat the > point of the test. > > One could certainly argue that these are safe enough because nobody would > ever create real roles by those names anyway. I'm not very comfortable > with that though; if we believe that, why did we go to the trouble of > making sure these cases work? Sadly, it's not quite so simple: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831234 Note that Christoph went ahead and closed out the bug report as it was just an issue in the regression test and not unexpected behavior, but if we're going to do something in this area then we may wish to consider fixing the case that caused that bug report. > What I'm inclined to do with this is to reduce the test to be something > like > > BEGIN; > CREATE ROLE "Public"; > CREATE ROLE "None"; > CREATE ROLE "current_user"; > CREATE ROLE "session_user"; > CREATE ROLE "user"; > ROLLBACK; > > with maybe a couple of ALTERs and GRANTs inside the transaction to verify > that the names can be referenced as well as created. This would be safe > against modifying any conflicting existing users; the only bad consequence > would be a phony failure of the test. Unfortunately, the above wouldn't fix the case where someone attempts to run the regression tests as a Unix user named "user". One suggestion discussed on the bug report was to include different result files, but that does seem pretty special-cased and overkill, though if the overall set of tests in rolenames.sql is reduced then perhaps it isn't as much of an issue. Then again, how many different result files would be needed to cover all cases? Seems like there could be quite a few, though, with this specific case, it looks like we'd at least only have to deal with one for each role which is allowed to exist already (such as "user"), without any multiplicative factors (can't run the regression test as more than one Unix user at a time). > A more aggressive answer would be to decide we don't need these test cases > at all and drop them. An advantage of that is that then we could > configure some buildfarm animal to fail the next time somebody ignores > the "test role names should contain 'regress'" rule. I'd really like to have more test coverage rather than less, so I'd find this a bit hard to swallow, but it might still be better than the alternatives. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Regression tests vs existing users in an installation
On 16 Jul 2016 12:59 pm, "Michael Paquier" wrote: > > Thanks for doing this. +1 Though I might highlight this as the kind of issue that a bug tracker would help avoid falling through the cracks and make visible to newcomers. > I am -1 for dropping the tests. We could just have a CFLAGS that adds > an elog(ERROR) in CreateRole and checks that the created role has a > wanted prefix, or have a plugin that uses the utility hook to do this > filtering. If we make a hidden regression_test_safety GUC then we could have pg_regress enable it and have these specific tests disable it explicitly with comments on why it's safe. It might even be handy for other people writing application regression tests depending on what other things it blocked. A hook might even be possible to use the same way. pg_regress would have to build and install a .so which might be tricky.
Re: [HACKERS] Regression tests vs existing users in an installation
On Sat, Jul 16, 2016 at 7:13 AM, Tom Lane wrote: > We've talked before about how the regression tests should be circumspect > about what role names they create/drop, so as to avoid possibly blowing > up an installation's existing users during "make installcheck". In > particular I believe there was consensus that such names should begin > with, or at least include, "regress". I got around today to instrumenting > CreateRole to see what names we were actually creating, and was quite > depressed as to how thoroughly that guideline is being ignored (see > attached). Thanks for doing this. > A more aggressive answer would be to decide we don't need these test cases > at all and drop them. An advantage of that is that then we could > configure some buildfarm animal to fail the next time somebody ignores > the "test role names should contain 'regress'" rule. I am -1 for dropping the tests. We could just have a CFLAGS that adds an elog(ERROR) in CreateRole and checks that the created role has a wanted prefix, or have a plugin that uses the utility hook to do this filtering. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regression tests vs existing users in an installation
Tom Lane wrote: > We've talked before about how the regression tests should be circumspect > about what role names they create/drop, so as to avoid possibly blowing > up an installation's existing users during "make installcheck". In > particular I believe there was consensus that such names should begin > with, or at least include, "regress". I got around today to instrumenting > CreateRole to see what names we were actually creating, and was quite > depressed as to how thoroughly that guideline is being ignored (see > attached). > > I propose to go through the regression tests and fix this (in HEAD only). I would propose that we have one test run near the beginning or right at the beginning of the serial schedule that sets up a small number of roles to cover most of the needs of every other test, so that most such other tests do not need to create any roles at all. (Of course, there would be a few cases where this would defeat the purpose of the test because creating or dropping the role is precisely what is being created; those cases would have additional roles, with the proposed prefix.) So currently we have 97 roles? Probably we can get away with a dozen or so, maybe even less than that. > What I'm inclined to do with this is to reduce the test to be something > like > > BEGIN; > CREATE ROLE "Public"; > CREATE ROLE "None"; > CREATE ROLE "current_user"; > CREATE ROLE "session_user"; > CREATE ROLE "user"; > ROLLBACK; > > with maybe a couple of ALTERs and GRANTs inside the transaction to verify > that the names can be referenced as well as created. This would be safe > against modifying any conflicting existing users; the only bad consequence > would be a phony failure of the test. > > I thought about trying to preserve all the existing test cases while still > keeping these roles inside a transaction, by inserting savepoints around > the intentional failures. But there are enough intentional failures in > rolenames.sql that that would be really tedious. The existing test cases > seem enormously duplicative to me anyway, so I think a fairly short > transaction with a few tests would be sufficient to cover this territory. > A more aggressive answer would be to decide we don't need these test cases > at all and drop them. Hmm ... I think a blanket removal would go against generally accepted principle that our tests need to cover more functionality. Maybe we did go overboard on that one and the rolled-back creation is enough test for that functionality. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Regression tests vs existing users in an installation
We've talked before about how the regression tests should be circumspect about what role names they create/drop, so as to avoid possibly blowing up an installation's existing users during "make installcheck". In particular I believe there was consensus that such names should begin with, or at least include, "regress". I got around today to instrumenting CreateRole to see what names we were actually creating, and was quite depressed as to how thoroughly that guideline is being ignored (see attached). I propose to go through the regression tests and fix this (in HEAD only). However, there's one place where it's not so easy to just substitute a different name, because rolenames.sql is intentionally doing this: CREATE ROLE "Public"; CREATE ROLE "None"; CREATE ROLE "current_user"; CREATE ROLE "session_user"; CREATE ROLE "user"; in order to test whether we properly distinguish role-related keywords from quoted identifiers. Obviously, modifying these would defeat the point of the test. One could certainly argue that these are safe enough because nobody would ever create real roles by those names anyway. I'm not very comfortable with that though; if we believe that, why did we go to the trouble of making sure these cases work? What I'm inclined to do with this is to reduce the test to be something like BEGIN; CREATE ROLE "Public"; CREATE ROLE "None"; CREATE ROLE "current_user"; CREATE ROLE "session_user"; CREATE ROLE "user"; ROLLBACK; with maybe a couple of ALTERs and GRANTs inside the transaction to verify that the names can be referenced as well as created. This would be safe against modifying any conflicting existing users; the only bad consequence would be a phony failure of the test. I thought about trying to preserve all the existing test cases while still keeping these roles inside a transaction, by inserting savepoints around the intentional failures. But there are enough intentional failures in rolenames.sql that that would be really tedious. The existing test cases seem enormously duplicative to me anyway, so I think a fairly short transaction with a few tests would be sufficient to cover this territory. A more aggressive answer would be to decide we don't need these test cases at all and drop them. An advantage of that is that then we could configure some buildfarm animal to fail the next time somebody ignores the "test role names should contain 'regress'" rule. Comments? regards, tom lane LOG: created role tablespace_testuser1 LOG: created role tablespace_testuser2 LOG: created role regtestrole LOG: created role regress_rol_op1 LOG: created role regress_rol_op3 LOG: created role regress_rol_op4 LOG: created role regress_rol_op5 LOG: created role regress_rol_op6 LOG: created role regression_reindexuser LOG: created role regtest_unpriv_user LOG: created role test_def_superuser LOG: created role test_superuser LOG: created role Public LOG: created role None LOG: created role current_user LOG: created role session_user LOG: created role user LOG: created role testrol0 LOG: created role testrolx LOG: created role testrol2 LOG: created role testrol1 LOG: created role test_def_inherit LOG: created role test_inherit LOG: created role test_def_createrole LOG: created role test_createrole LOG: created role test_def_createdb LOG: created role test_createdb LOG: created role test_def_role_canlogin LOG: created role test_role_canlogin LOG: created role test_def_user_canlogin LOG: created role test_user_canlogin LOG: created role test_def_replication LOG: created role test_replication LOG: created role tu1 LOG: created role tr1 LOG: created role tg1 LOG: created role test_def_bypassrls LOG: created role test_bypassrls LOG: created role view_user1 LOG: created role view_user2 LOG: created role selinto_user LOG: created role regtest_addr_user LOG: created role regress_rls_alice LOG: created role regress_rls_bob LOG: created role regress_rls_carol LOG: created role regress_rls_exempt_user LOG: created role regress_rls_group1 LOG: created role regress_rls_group2 LOG: created role regress_rol_lock1 LOG: created role regressuser1 LOG: created role regressuser2 LOG: created role regressuser3 LOG: created role regressuser4 LOG: created role regressuser5 LOG: created role regressgroup1 LOG: created role regressgroup2 LOG: created role seclabel_user1 LOG: created role seclabel_user2 LOG: created role schemauser1 LOG: created role schemauser2 LOG: renamed role schemauser2 to schemauser_renamed LOG: created role locktable_user LOG: created role regress_rls_eve LOG: created role regress_rls_frank LOG: created role regress_rls_dob_role1 LOG: created role regress_rls_dob_role2 LOG: created role regress_user_mvtest LOG: created role regtest_alter_user3 LOG: created role regtest_alter_user2 LOG: created role regtest_alter_user1 LOG: created role regtest_alter_user LOG: created role regtest_alter_user5 LOG: created ro