Flood database backend ::isAllowed() should call ::ensureTableExists()

Unless something very flukey is happening, which I am not ruling out yet, this reduces a core MR test run to 7m48s from ~10m.

Confirmed that this is removing three queries against information_schema when running performance tests.

I haven't yet confirmed that we're running this query every time we log in a user during tests - if the installer queries flood at all, then it will, if not, it might only affect second or third logins (which the performance tests do).

Modified StandardPerformanceTest::testLogin() to remove the cache warming.

This shows multiple information_schema queries on the first login in 11.x:

vs. with the MR, a single information_schema query, then a create table, then 'normal' flood queries after that:

The CREATE TABLE takes 46ms on my local machine, but a possible explanation for the faster test runs is that because gitlab is on a ramdisk, table creation is cheaper than running all those extra database queries. Or we're getting lucky with the runners today and it's not a full minute's improvement, but it shows that the performance tests are correctly diagnosing a bug!

Before image had the wrong queries expanded, so updating the issue summary.

Better 'before' screenshot too.

I've reviewed the code and checked all the previous comments with performance information. It makes total sense to create the table the first time we query it and don't find it, rather than catching exceptions all the time and not creating the table.

We can see performance gains in the run too, which is a nice side effect too.

Marking RTBC.

Great find!

Personally, I find on-the-fly table creation a rather bad approach. If we had the table created at installation, we wouldn’t have to care about this. On-the-fly table creation breaks transactions for DBs that do not support transactional DDL - not just the transaction where ensureTable is called, also any concurrent transaction from other sessions. But I suppose this train is lost now :(

@mondrake we've got a few more options now - for example services that need to create tables could subscribe to a module installed event and create the storage then. The main reason for on-the-fly table creation originally was to avoid creating database tables from hook_schema() for services that were using alternative storage - like multiple cache tables when a site was using redis or memcache. Worth a new issue to discuss.

Yeah… I was also thinking events would help in 📌 Adding GIN and GIST indexes to PostgreSQL databases RTBC , to allow database drivers to decorate db objects specifications with db specific features. I will open a new issue to discuss.

Not sure whether it's enabled by this issue or test runs have been slowly getting faster, but new tests we can mark with @group #slow which are overhanging the end of the test runs 📌 Add @group slow to ForumTest, HelpTopicSearchTest, ModulesListFormWebTest Needs review .

Committed to 11.x and backported to 10.2.x

Nice one!

larowlan → closed merge request !5921

This narrowed the query range too much, opened 📌 Fix random performance test failures Needs review . Ironically although I found this looking at performance test query spans, the assertions only count successful queries - if a query triggers an exception, the 'after' event doesn't get fired, so we don't see a strong decrease in the number of queries in the test coverage itself even though less are happening in reality. It might be possible to check if the start and end events match and log some kind of error if they don't to cover that case too.

if a query triggers an exception, the 'after' event doesn't get fired

We may want to add another statement execution event on top of the start/end ones, e.g. StatementExecutionFailureEvent, so that exception handling may dispatch them.

Filed ✨ Add event for statement execution failure Active .

Also filed ✨ Use events in Database Schema operations Active re #13/#14

Automatically closed - issue fixed for 2 weeks with no activity.