Обсуждение: Guidelines on dropping objects in regression tests, sqlsmith
Andreas Seltenreich's sqlsmith tool has found an impressive number of bugs. In light of that, it seems to me that it would be reasonable for a contributor to write a regression test that avoids dropping database objects specifically so that sqlsmith had some chance of finding bugs after the fact, by generating a query whose plan ends up accessing said objects. Commit 975ad4e6 fixed a nasty bug in BRIN that was originally found by Andreas' fuzz testing. Perhaps that bug would still be around if Alvaro had included any DROP statements within brin.sql. Presumably his choice to not to do so was completely arbitrary. Actually, it seems likely that it was a fortunate accident. I understand that there is a cost to increasing the high-watermark amount of disk space used during regression testing, since many buildfarm animals are rather underpowered. I also understand that in most cases the objects themselves aren't really what we're testing, so it doesn't make much sense to leave them behind. However, it still seems like a good idea to try to think about this a little more strategically. For example, the new index_including.sql file drops all INCLUDE indexes/tables proactively, even though it looks like they're rather small, and in a certain sense worth keeping around. I also see that both spgist.sql and gin.sql avoid dropping objects, but gist.sql seems to drop several. hash_index.sql drops some objects, too (sqlsmith also found hash index bugs despite this). Maybe a cheap, scalable solution is possible. For example, perhaps we could maintain a version of the regression tests that have Postgres somehow silently ignore most DROP statements. I doubt that people like Andreas will ever be remotely concerned about the extra disk space overhead, since fuzzing is inherently a computationally intensive process; CPU costs are bound to dominate. A strategy like this would also make it easier to run amcheck on indexes left behind by the regression tests. How can we do better? -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> Andreas Seltenreich's sqlsmith tool has found an impressive number of
> bugs.
Indeed.
> In light of that, it seems to me that it would be reasonable for
> a contributor to write a regression test that avoids dropping database
> objects specifically so that sqlsmith had some chance of finding bugs
> after the fact, by generating a query whose plan ends up accessing
> said objects.
Traditionally, we've left around instances of various sorts of objects
so that pg_dump/pg_upgrade would be exercised on those objects. It's
possible that sqlsmith has different needs in this area, but hard to
say without more thought.
In any case, I'd say that there's never been a scorched-earth policy
so far as leaving regression test objects behind is concerned --- with
the exception of objects visible outside the regression database, ie
roles or tablespaces. I wonder whether that restriction is problematic
from this standpoint.
> For example, the new index_including.sql file drops all
> INCLUDE indexes/tables proactively, even though it looks like they're
> rather small, and in a certain sense worth keeping around.
I agree that that's a completely bad idea, especially if nothing's
been done to ensure pg_dump test coverage for the feature otherwise.
regards, tom lane
Peter Geoghegan wrote: > Commit 975ad4e6 fixed a nasty bug in BRIN that was > originally found by Andreas' fuzz testing. Perhaps that bug would > still be around if Alvaro had included any DROP statements within > brin.sql. Presumably his choice to not to do so was completely > arbitrary. Actually, it seems likely that it was a fortunate accident. As I recall, it was deliberate, as I tend to do that; I did it in indexing.sql as well, and I think in foreign_key.sql too. These objects are also dumped/restored by pg_dump in the pg_upgrade test. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Apr 14, 2018 at 6:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Traditionally, we've left around instances of various sorts of objects > so that pg_dump/pg_upgrade would be exercised on those objects. It's > possible that sqlsmith has different needs in this area, but hard to > say without more thought. Clearly it would be a shame if there were serious gaps in our test coverage because nobody did some simple analysis. >> For example, the new index_including.sql file drops all >> INCLUDE indexes/tables proactively, even though it looks like they're >> rather small, and in a certain sense worth keeping around. > > I agree that that's a completely bad idea, especially if nothing's > been done to ensure pg_dump test coverage for the feature otherwise. I took a look through all of the SQL files that the INCLUDE covering indexes patch added tests to. At no point do they leave behind any INCLUDE indexes. I'll do something about that as part of the INCLUDE patch that I'm working on at the moment. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> I took a look through all of the SQL files that the INCLUDE covering
> indexes patch added tests to. At no point do they leave behind any
> INCLUDE indexes. I'll do something about that as part of the INCLUDE
> patch that I'm working on at the moment.
A quick look at the current final state of the regression database
says
regression=# select indexrelid::regclass, indrelid::regclass from pg_index where indnatts != indnkeyatts;
indexrelid | indrelid
---------------------+-------------
covidxpart1_a_b_idx | covidxpart1
covidxpart_a_b_idx | covidxpart
covidxpart2_a_b_idx | covidxpart2
covidxpart3_a_b_idx | covidxpart3
covidxpart4_a_b_idx | covidxpart4
(5 rows)
So we've got *some*, but it sure looks like they were all added by the
patch to fix covering indexes for partitions. I'd want to see some for
plain-table cases as well.
regards, tom lane
On Sat, Apr 14, 2018 at 8:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So we've got *some*, but it sure looks like they were all added by the > patch to fix covering indexes for partitions. I'd want to see some for > plain-table cases as well. Will do. -- Peter Geoghegan