Reduce the query cost of taking a place

Taking a place costs 22 queries in the request that handles the click. PlaceMapPerformanceTest pins the number, so it is measured rather than estimated, and it is measured in the context that produces it: a real request against MySQL with database cache bins. A kernel probe cannot answer this question, because it runs on SQLite with memory cache bins.

Where this lands. The slot tariff backing measured below went into the merge request of #3615440: Move the hold clock from the booking line to the order, now merged, which already touched the hold path this issue measures. The section settings backing is its own merge request against 1.x. Keeping them in one place avoids two merge requests moving the same numbers past each other, and the performance test that pins those numbers lives there too. This issue stays open for the parts not covered by it: the pre-lock pass, the remaining lookups, and the booking list cache tag.

The measured request

The click that matters is the recurring one, a visitor taking a second place into a basket that is already open. Statements 1 to 3 are the session and the user, and are not yoyaku. The rest:

• Q04 the cart pointer, from the private tempstore
• Q05 the order lines, by transaction
• Q06 the configuration sections of the place map
• Q07 is this place taken, by slot and place
• Q08 consumption on the slot, SUM(quantity) grouped by tariff and allotment
• Q09 the slot tariffs, by slot
• Q10 the order lines again, now filtered by state
• Q11 the slot row lock, FOR UPDATE
• Q12 the order lines a third time, under the lock
• Q13 the place count again, under the lock
• Q14 consumption again, FOR UPDATE
• Q15 the slot tariffs again
• Q16 savepoint
• Q17 the booking insert
• Q18 release savepoint
• Q19 existence check on cachetags for yoyaku_booking_list
• Q20 the increment of that tag
• Q21 the order lines a fourth time, which has to see the row just written
• Q22 loading that line

Statements 12 to 15 repeat 10, 7, 8 and 9. Statements 19 and 20 are cache bookkeeping. That is six of the twenty-two doing work the request has already done or does not need.

And the same request once the slot tariff backing is in

Twenty statements. Nothing else moved, so the two that went can be read off directly.

• Q01 to 04 the session, the user, the roles, the cart pointer: unchanged
• Q05 the order lines, by transaction
• Q06 the configuration sections of the place map
• Q07 is this place taken, by slot and place
• Q08 consumption on the slot, grouped by tariff and allotment
• Q09 the order lines again, filtered by state (was Q10)
• Q10 the slot row lock, FOR UPDATE (was Q11)
• Q11 the order lines under the lock (was Q12)
• Q12 the place count under the lock (was Q13)
• Q13 consumption again, FOR UPDATE (was Q14)
• Q14 to 16 savepoint, the booking insert, release
• Q17, 18 the yoyaku_booking_list existence check and increment
• Q19 the order lines a third time, seeing the row just written
• Q20 loading that line

Gone: the old Q09 and Q15, both SELECT ids FROM yoyaku_slot_tariff WHERE slot IN (1). The first was the optimistic pass and the second the re-read under the lock. Neither is asked now: the bin answers both, and the entry survives between requests, so even the first read of the click is a hit once the map render has warmed it.

Nothing else in the list changed position on its own account, and no statement was added. The cachetags pair at 17 and 18 is still the booking list tag being invalidated by the hold, which is item 4 below and untouched by this.

1. The capacity pass runs twice, and the first one is only an early exit

Before taking the slot row lock, holdInTransaction() runs runConstraints() and assertFits(), then runs the same rules again under the lock where the answer is binding. The code is honest about why the first pass is there: nothing is lost here but the early exit. So in the refusal case it saves taking a lock, and in the success case, which is the ordinary case for a seat click, it is four queries of pure duplication.

The trade-off, which is the actual question: dropping it means every refusal takes the slot row lock. That is exactly the wrong direction during an on-sale, where refusals are frequent and serialising them behind the lock is the failure mode. So simply deleting it is probably wrong. Two shapes to weigh instead, neither of them free.

Shape A: make the pre-lock pass cheaper

Less available than it first looks. The four are not four ways of asking one question: the place count and the consumption sum hit yoyaku_booking on slot, the tariff read hits yoyaku_slot_tariff, and the order lines hit yoyaku_booking on transaction. And consumedOnSlot() is already a merge, by its own docblock: one read answers all three bounds. The one honest merge left is folding the place count into the consumption query as a conditional sum, so four becomes three. Or-ing the order lines onto the slot condition would defeat both indexes.

It also has a design cost. The module keeps one source for the availability rule on purpose, which is why hold() is private: a second entry point is where enforcement goes to be forgotten. A bespoke pre-check is a second implementation that can drift from the binding one, and drift here means the two passes disagree, which is worse than the query. Avoiding that means the merged read serves both passes, so a FOR UPDATE variant of a more complicated query has to be maintained.

It also makes the duplicated work cheaper rather than stopping it, and it does not cover the constraint plugins, which do their own reads. The fixture configures no constraint policies, so the measured four is a floor for a real site.

Shape B: let the place map hand its availability in

The map was drawn from availability fetched moments ago, which is why taken places are already greyed. If the request carries that reading, the pre-lock pass reads nothing and the lock does what it was always going to do. Client input decides only whether to skip the optimistic check; the pass under the lock is unchanged and still refuses, so a client claiming a stale reading gains nothing but reaching the lock sooner, which is the contention the pre-lock pass exists to avoid. That is the trade, and it is the one worth arguing.

Shape B has the headroom. Shape A is lower risk with a ceiling of about one query.

2. The slot tariff read is answered from the cache bin: done, measured

SlotTariffLookup already caches per request, primes a whole page of sessions in one query and memoises the empty answer too. The second read is not a cache miss: reloadForCheck() calls forget() on purpose, so the pass under the lock sees rows committed since.

The first instinct was to drop the forget(). That was wrong, and the reason is worth recording. The two reads beside it, loadUnchanged() on the session and on the tariff, cost no query at all. ContentEntityStorageBase::loadUnchanged() resets only the static cache and then reads the persistent one, querying solely on a miss, its own comment explaining that the persistent cache never changes until the entity is saved. So two of the three re-reads under the lock were already free, and the slot tariff paid a query only because this module keeps its own map with nothing behind it.

So the fix is not to check less. It is to give the slot tariff the same backing the session and the tariff already have: one bin entry per session holding the ids. forget() stays, the freshness semantics are unchanged, and the repeat read stops costing a query.

The entries carry no cache tag, and that is the point

A tagged entry is validated on every read, and validating a tag it has not seen before costs the request a SELECT tag, invalidations FROM cachetags of its own. Tagging cost back one of the two queries saved, which was measured rather than guessed: the click read 20 queries but 2 tag lookups instead of 1.

So the entries are untagged and BookingHooks drops the entry of the session a slot tariff was written for, on insert, update and delete, plus the session a moved row came from. AllotmentSettings::inUse() already makes this exact trade, and its hook says why: it is not a cache tag on purpose: validating one costs a lookup on every request that reads the flag, which is the cost the flag exists to avoid.

Two things this buys beyond the query. Invalidation becomes narrow: the list tag yoyaku_slot_tariff_list ends the cached entry of every session on the site, so one operator adjusting one quota cold-starts them all, and on an institutional site that editing happens during the day while people are booking. A purge ends one. And the write path loses a database write: invalidating a tag is an UPDATE cachetags SET invalidations = invalidations + 1 against a globally hot table, where a purge is a delete against the bin.

What it costs is that the invalidation is ours rather than core's. A row written around the entity API, by hand or by a migration, would leave an entry nobody drops. Everything in this module writes through entity save, so that is a rule we keep rather than a hope, but it is a rule someone could break later without noticing.

Measured, same rig and same test

Two queries off the click and one off the render, with nothing given back. On a site whose bins are in memory both reads leave the database altogether. PlaceMapPerformanceTest pins the new numbers, and SlotTariffCacheTest covers the three behaviours that matter: the repeat read asks the database nothing, a quota just saved is what the next read returns, and a quota changed on an already cached session is seen too. It was watched failing against unfixed code before being kept.

3. The same backing applied to the other per-request lookups

The pattern comes from one fact: core caches an entity load and never caches an entity query. A service that memoises ids for the request pays a query for them again on the next one, for data only an operator changes. Two candidates were named here. One took it and one did not, and the difference is worth recording because it decides where else this is worth trying.

SectionSettings: done, in #3615440: Move the hold clock from the booking line to the order and then its own merge request

A configuration's section settings are the venue layout: which areas it opens, closes or pools. Backed by the bin, keyed per configuration, untagged, and dropped by PlacementHooks::configurationSectionWritten() for the configuration a setting was written for plus the one a moved setting came from.

The render gains two rather than one because the section settings are what that page is drawing, and it asks for them on both of its requests. Nothing is given back: the entries carry no tag, so nothing is validated on the way in. SectionSettingsCacheTest pins both halves, that a later request costs no query and that a setting just saved is what the next read applies, and it was watched failing against unfixed code first.

AllotmentSettings: declined, and it would have been a regression

Applying the same shape here looked obvious and is wrong. AllotmentSettings::records() is not an id query per key: it is a single raw UNION over yoyaku_slot_allotment and yoyaku_resource_allotment, primed for a whole page at once, so it already costs one query per page whatever the session count.

Caching it would be keyed per session and per resource. A calendar showing thirty sessions would then do thirty or more bin reads in place of one batched indexed query, and a hold would do two in place of one. That is worse on the page and no better on the hold, which is the whole arithmetic that made the other two win:

And the part of it that was worth caching is already cached, in the same untagged and hook-dropped shape this issue has been applying elsewhere: inUse(), the flag saying whether the site holds anything under an allotment at all, so a site using none pays nothing on the path every availability figure takes. That is where the pattern was borrowed from in the first place.

What is not ruled out is whether that UNION gets expensive as its IN list grows on a hall with many allotments. That is a question about load rather than about caching, and it belongs to #3615593: Load test the booking path: what it bears, and what gives way first.

4. Every hold invalidates the booking list cache tag: investigated, and it stays

Saving a held line invalidates yoyaku_booking_list, which is statements 17 and 18 of the measured click: an existence check against cachetags and then the increment. Two queries on a path that runs thousands of times during an on-sale, which invites the question of whether they are earning anything.

They are. Two consumers were found, and both count held lines rather than only confirmed ones, so the invalidation cannot be narrowed to confirmation either.

• views.view.yoyaku_bookings in yoyaku_views. A view over the entity type carries its list tag on the render cache, and held lines are exactly what an operator needs to see to understand where capacity went.
• AllotmentAccessControlHandler, which adds the tag to both branches of its delete decision. The decision is not about the allotment, it is whether any booking still names it, so it has to end when a booking is written rather than when the allotment is. CONSUMING_STATES includes held, so a hold is precisely the event that must flip it from allowed to forbidden. The tag on the forbidden branch matters equally: without it, cancelling the last booking would never make the allotment deletable again.

Worth noting what the two map builders do not use, since it is the obvious guess and it is wrong: VenueMapBuilder::inventoryTags() covers places, sections and grades, and PlaceAvailability::pinnedTags() covers pinned places, places and configuration sections. All venue layout, no bookings. The map does not read availability from a render cache keyed on this tag.

Where the cost actually comes from, and why nothing is being done about it yet

The access handler is a consumer, not the cause. What issues those two queries is core: EntityBase::invalidateTagsOnSave() invalidates the entity type list tag on every save, whether anything reads it or not. Removing the handler would not save a single query on the click.

Removing them means overriding invalidateTagsOnSave() on the booking entity and then re-solving both consumers. The allotment guard is easy and free, setCacheMaxAge(0) on that decision, since allotment screens are opened rarely and the recheck is one indexed query. The view is harder and means either accepting staleness or inventing a narrower tag. That is the same principle applied one layer up from the slot tariff above: make the rare reader pay rather than the hot writer.

Not being built now, deliberately. The query count is measured; the thing that would actually justify the change is not. The concern is contention on a single cachetags row under a rush, and there is no measurement of that here. It is also how every content entity in Drupal behaves, so it is not a yoyaku problem until an on-sale is observed waiting on that row. Filed as a known cost with a scoped fix rather than a speculative rewrite of core entity behaviour that would take a correctness guard and a view cache with it.

Resolved while investigating

The two loadUnchanged() calls in reloadForCheck() produce no statement, and the reason is ContentEntityStorageBase::loadUnchanged() reading the persistent cache rather than the database. That is settled, and it is what the fix above is built on.

What is not in scope

The session and the cart pointer, the configuration sections, the binding pass under the lock, the insert and its savepoint, and the re-read of the order lines after the write, which has to see what was just written. None of those look reducible without changing what the request guarantees.

Verification

Any change here has to move the PlaceMapPerformanceTest expectations down and stay green, and the concurrency behaviour has to be argued rather than assumed: the pre-lock pass exists for a reason, even if that reason does not pay for itself on the happy path. Query count is not latency, and nothing here has been timed, so a change that trades several indexed lookups for one broader query needs timing and not just a lower count.