Re: [HACKERS] Regression tests vs existing users in an installation
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2019-06-26T03:33:22Z
Lists: pgsql-hackers
Attachments
- enforce-rules-about-regression-global-object-names.patch (text/x-diff) patch
[ blast from the past department ] So, this thread about ensuring the regression tests don't create random globally-visible names seems to have got lost in the weeds. I'm going to resurrect it after noticing that two different places have grown violations of the rule since I fixed things in 18555b132. I think we were largely overdesigning the fix. The right thing is to do something approximately as attached, and then make at least one buildfarm critter build with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS. This proposed patch intentionally emits only WARNINGs, not ERRORS. This is so that TAP tests won't fail if the warnings fire. Since TAP tests never run against an existing installation, there's no reason for them to follow the restriction. Admittedly, this is a pretty ad-hoc way of getting that end result, but I'm tired of waiting for somebody to implement a less ad-hoc way. There are still two things we'd have to argue about before committing this. One is the already-discussed-upthread point that rolenames.sql insists on creating and then deleting users with names like "user". I remain of the opinion that that's just a damfool idea; there is nearly zero chance that those test cases will ever catch a bug, and much more than zero chance that they'll cause problems in some pre-existing installation. So my vote is to take them out. The other is that we've also grown a bunch of tests that create subscriptions and replication origins with randomly-chosen names. Since those objects also have globally-visible names, I think the "name them regress_something" policy should apply to them too, and the attached patch tries to enforce it. But of course that causes a bunch of failures right now. (While I'm looking at that, I wonder why we don't have a restriction against "pg_xxx" names for those object types.) Comments? regards, tom lane
Commits
-
Move rolenames test out of the core regression tests.
- c91504b958e1 12.0 landed
-
Add an enforcement mechanism for global object names in regression tests.
- 54100f5c6052 12.0 landed
-
Fix regression tests to use only global names beginning with "regress_".
- ca129e58c01f 12.0 landed
-
Disallow user-created replication origins named "pg_xxx".
- a1e61badf97b 12.0 landed