Refactoring the regression tests for more independence
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2021-12-24T22:00:17Z
Lists: pgsql-hackers
Attachments
- 0001-refactor-functions.patch (text/x-diff) patch 0001
- 0002-refactor-tables.patch (text/x-diff) patch 0002
- script-dep-testing.sh.gz (application/x-gzip)
Noah suggested in [1] that we should make an effort to allow any one of the core regression tests to be run mostly standalone (i.e., after running only the test_setup script), so as to allow quicker iterations when adjusting a script. This'd presumably also lead to the tests being more independent, which seems like a good thing. I spent a bit of time looking into this idea, and attached are a couple of draft patches for discussion. I soon realized that complete independence was probably infeasible, and not very useful anyway. Notably, it doesn't seem useful to get rid of the geometry script's dependencies on the per-geometric-type scripts, nor of horology's dependencies on the per-datetime-type scripts. I suppose we could think of just merging the per-type scripts into geometry and horology, but that does not seem like an improvement. So my goal here is to get rid of *most* dependencies, and ensure that the remainder are documented in the parallel_schedule file. Also note that I'm explicitly not promising that the tests can now run in any order --- I've made no attempt to get rid of "A can't run before B" or "A can't run concurrently with B" restrictions. 0001 below gets rid of dependencies on the create_function_N scripts, by moving functions they define into either the particular script that uses the function (for the ones referenced in only one script, which is most) or into the test_setup script. It turns out that create_function_1 and create_function_2 go away entirely, because nothing's left. While I've not done so here, I'm tempted to rename create_function_0 to create_function_c and create_function_3 to create_function_sql, to give them better-defined charters and eliminate the confusion with trailing digits for variant files. (With that division of labor in mind, 0001 does move a couple of SQL functions from create_function_0 to create_function_3.) 0001 also moves some hash functions that were created in insert.sql into test_setup, because they were also used elsewhere. I also cleaned up some other type-related script interdependencies, by consolidating the "widget"-related code into create_type, removing a dependency on the custom path ## path operator in favor of the equivalent built-in ?# operator, and declaring the textrange and float8range types in test_setup. Lastly, 0001 fixes the tab_core_types test case in type_sanity so that it only covers built-in types, not types that randomly happen to be created in test scripts that run before type_sanity. 0002 performs a similar set of transformations to get rid of table-related script interdependencies. I identified a dozen or so tables that are used in multiple scripts and (for the most part) are not modified once filled. I moved the creation and filling of those into test_setup. There were also some tables that were really only used in one script, so I could move their creation and filling to that script, leaving no cross-script dependencies on create_table.sql or copy.sql. I made some other adjustments to get rid of incidental cross-script dependencies. There are a lot more judgment calls in 0002 than 0001, though, so people might have objections or better ideas. Notably: * A few scripts insisted on modifying the "shared" tables, which seemed like something to get rid of. What I did, to minimize the diffs in these scripts, was to make them create temporary tables of the same names and then scribble on the temp tables. There's an argument to be made that this will be too confusing and we'd be better off changing the scripts to use different names for these local tables. That'd make the patch even bulkier, though. * create_index made some indexes on circle_tbl and polygon_tbl, which I didn't want to treat as shared tables. I moved those indexes and the associated test queries to the end of geometry.sql. They could have been made in circle.sql and polygon.sql, but I was worried that that would possibly change plans for existing queries in geometry.sql. * create_index also had some queries on array_op_test, which I'm now treating as private to arrays.sql. The purpose of those was to compare index-free results to indexable queries on array_index_op_test, which is now private to create_index. So what I did was to replace those by doing the same queries on array_index_op_test before building its indexes. This is a better way anyway since it doesn't require the unstated assumption that array_op_test and array_index_op_test contain identical data. * The situation with a_star and its child tables was a bit of a mess. They were created in create_table.sql, populated in create_misc.sql, then misc.sql did significant DDL on them, and finally select_parallel used them in queries (and would fail outright if the DDL changes hadn't been made). What I've done here is to move the create_table and misc steps into create_misc, and then allow select_parallel to depend on create_misc. You could argue for chopping that up differently, perhaps, but I'm not seeing alternatives I like better. * Having established the precedent that I'd allow some cross-script dependencies on create_misc, I adjusted a couple of places that were depending on the "b" table made by inherit.sql to depend on create_misc's b_star, which has just about the same schema including children. I figured multiple dependencies on create_misc was better than some on create_misc and some on inherit. (So maybe there's a case for moving that entire sequence into test_setup? But it seems like a big hunk that doesn't belong there.) * Another table with an unreasonably large footprint was the "tmp" table made (but not used) in select.sql, used in select_distinct and select_distinct_on, and then modified and eventually dropped in misc.sql. It's just luck this doesn't collide with usages of tables named "tmp" in some other scripts. Since "tmp" is just a copy of some columns from "onek", I adjusted select_distinct and select_distinct_on to select from "onek" instead, and then consolidated the usage of the table into misc.sql. (I'm half tempted to drop the table and test cases from misc altogether. The comments there indicate that this is a 25-year-old test for some b-tree problem or other --- but tmp has no indexes, so it can't any longer be testing what it was intended to. But removing test cases is not in the charter of this patch series, I guess.) * expressions.sql had some BETWEEN tests depending on date_tbl, which I resolved by moving those tests to horology.sql. We could alternatively change them to use some other table/datatype, or just accept the extra dependency. * The rules and sanity_check scripts are problematic because their results depend heavily on just which scripts execute before them. In this patch I've adopted a big hammer: I trimmed rules' output by restricting it to only print info about pg_catalog relations, and I dropped the troublesome sanity_check query altogether. I don't think that sanity_check query has any real use, certainly not enough to justify the maintenance effort we've put into it over the years. Maybe there's an objection to restricting the coverage of rules, though. (One idea to exercise ruleutils.c more is to let that query cover information_schema as well as pg_catalog. Local code-coverage testing says there's not much difference, though.) Some things I'm not totally happy about: * Testing shows that quite a few scripts have dependencies on create_index, because their EXPLAIN output or row output order varies if the indexes aren't there. This dependency could likely be removed by moving creation of some of the indexes on the "shared" tables into test_setup, but I'm unconvinced whether that's a good thing to do or not. I can live with documenting create_index as a common dependency. * I treated point_tbl as a shared table, but I'm not sure that's a great idea, especially since the non-geometry consumers of point_tbl both want to scribble on it. Doing something else would be more invasive though. * psql.sql has a dependency on create_am, because the "heap2" access method that that creates shows up in psql's output. This seems fairly annoying, since there's no good semantic excuse for such coupling. One quick-and-dirty workaround could be to run the psql test before create_am. * amutils depends on indexes from all over the map, so it has a rather horrid dependency list. Perhaps we should change it to print info about indexes it manufactures locally. Thoughts? regards, tom lane PS: To save anyone else the work of reinventing it, I attach a script I used to confirm that the modified test scripts have no unexpected dependencies. I don't propose to commit this, especially not in its current hacky state of overwriting the parallel_schedule file. (Maybe we should provide a way to run specified test script(s) *without* invoking the whole schedule first.) [1] https://www.postgresql.org/message-id/20211217182518.GA2529654%40rfd.leadboat.com
Commits
-
Rename create_function_N test scripts for clarity.
- 2da896182ce1 15.0 landed
-
Rearrange core regression tests to reduce cross-script dependencies.
- cc50080a828d 15.0 landed