Fix/optionset preheat n1 master

[jason-p-pickering]

Same PR as #24815

Awaiting Team tracker approval.

[muilpp]

Just wondering if here would be worth it to query directly the pairs we need, something like

select optionsetid, code from optionvalue  
where (optionsetid || ':' || code) in (:pairs);

pairs being the objects in the chunk list

[jason-p-pickering]

Good point. Let me check.

[jason-p-pickering]

I looked at it quickly, and I do not think so. Ran both plans against the Sierra Leone DB.

  EXPLAIN ANALYZE SELECT optionsetid, code from optionvalue
  where (optionsetid::text || ':' || code) in ('3150104' || ':' || 'B023');
  Seq Scan on optionvalue  (cost=0.00..646.34 rows=76 width=13) (actual time=0.159..2.428 rows=1 loops=1)
    Filter: ((((optionsetid)::text || ':'::text) || (code)::text) = '3150104:B023'::text)
    Rows Removed by Filter: 15214
  Execution Time: 2.438 ms

vs. the current shape:

  EXPLAIN ANALYZE SELECT optionsetid, code from optionvalue
  where optionsetid in (3150104) and code in ('B023');
  Index Only Scan using optionvalue_unique_optionsetid_and_code on optionvalue  (cost=0.29..4.30 rows=1 width=13) (actual time=0.012..0.012
  rows=1 loops=1) 
    Index Cond: ((optionsetid = 3150104) AND (code = 'B023'::text))
    Heap Fetches: 0
  Execution Time: 0.022 ms

optionsetid::text || ':' || code has no matching expression index, so Postgres has to sequential-scan and evaluate the concatenation on every row of optionvalue . The proposed alternative route is more than 100 times slower.

The two separate INs aren't an approximation of the pair match either. They're sargable via optionvalue_unique_optionsetid_and_code (the UNIQUE (optionsetid, code) constraint from V2_41_6__Unique_code_within_each_optionset.sql), and we still get an exact pair match, just done in memory.

One caveat worth flagging: that unique constraint is created conditionally by the 2.41 migration. It's skipped on any instance that already had duplicate (optionsetid, code) rows at upgrade time. On such an instance neither query shape gets the index, and we're back to a seq scan regardless.

[jason-p-pickering]

@muilpp I added a regression test to guarantee correctness.

The test mocks a database containing (1,"B00"), (2,"A00"), (3,"C00") while the payload asks about (1,"A00"), (2,"B00"), (3,"C00"). Every requested optionSetId and every requested code shows up in some row, but the fabricated pairings never co-occur. The test asserts the two fabricated pairs stay invalid and the genuine one still validates.

I also performed a mutation test to ensure that it was not vacuous.

Hope this helps to address your concerns about the query pattern, while still showing why it should be this way due to the index.

[teleivo]

These forms query the pairs we need directly while allowing the use of the optionvalue_unique_optionsetid_and_code index

-- values join
join (values (3150104,'B023'), (3000008,'MODDISCH')) as t(optionsetid, code)
  on o.optionsetid = t.optionsetid and o.code = t.code

-- unnest of parallel arrays
join unnest(:setids::bigint[], :codes::text[]) as t(optionsetid, code)
  on o.optionsetid = t.optionsetid and o.code = t.code

@ameenhere is it worth benchmarking the 3 different forms that can use the index in Uganda? Maybe @jason-p-pickering can also test them in implementations you have access to? Uganda does bulk payloads with a single program. I think DEs/Attributes are in the hundreds but not sure. Ameen might know better.

[ameenhere]

I think it is worth exploring direct SQL rather than in-memory mechanisms. The approach here has a cost relative to the payload size (in addition to the metadata size). And that makes me uncomfortable because I know that can cause a lot of variations in performance from implementation to implementation.

The import payloads in Uganda was not "small". On an average they had 10s/100s of TEs, with maybe twice/thrice the events. And each event potentially having average 500 (and maybe upto 1000) DEValues. So that payload would cause unnecessary overhead with this approach. I also know there is a WHO implementation that wants to import huge payload (once a year procedure), payload file potentially reaching a GB or so. Not saying that huge payload approach is ideal, but reminding ourself how implementation nature can vary a lot.

Note that in the query comparisons above, we are only looking at the "gain" on the DB side. So we have no idea how the in-memory filtering affects the overall e2e performance.

That said, if we are able to conclude on a direct SQL mechanism that has an acceptable performance (perhaps the ones that @teleivo suggested above?), that is definitely preferred.

WDYT @jason-p-pickering ?

[jason-p-pickering]

Last comment here...GB payloads should be rejected by the reverse proxy (usually 100 MB max). I am assuming you mean when unzipped. That sounds however like an extremely bad idea. I do not think its a use case we should code for.

[teleivo]

Last comment here...GB payloads should be rejected by the reverse proxy (usually 100 MB max)
I don't think they are importing a GB at once @ameenhere do they? I think they are chunking it but of course want to import that giant data set as fast as possible.

[teleivo]

What would cause such a bug?

[jason-p-pickering]

Traced the pipeline (create() → preheat → validate → runRuleEngine() → validate again) and the value-mutating action types (ProgramRuleActionType.IMPLEMENTED_ACTIONS = {SENDMESSAGE, SCHEDULEMESSAGE, ASSIGN}). Of those, ASSIGN is the only one that touches a data value/attribute, and collectRuleAssignedValues() covers it exhaustively. So I can't currently construct a path that reaches that branch. It's a canary, not a known hole. It would fire if, a future PR adds a new value-mutating action type and forgets to teach OptionValueSupplier about it, or if the preheat ordering (ClassBasedSupplier → OptionValueSupplier → ...) gets reordered. Happy to drop it if you'd rather not carry defensive logging for a case that's unreachable today. Your call.

[teleivo]

This is more about the architecture / code placement. I am not sure this is the best place. I think @enricocolasante who is back next week might have a better answer than me with regards to rule engine.

If the fix is urgent we could merge it and Enrico and myself can debate on where to move it later on.

[jason-p-pickering]

It's a fair flag. I think ( I could be wrong) this is the earliest point which I could find where the rule engine's ASSIGN existed to preheat. Happy to wait until @enricocolasante gets back, but considering that we have seen a 5-6x drop in production numbers, I would lean towards merging it and revisiting.

[teleivo]

These forms query the pairs we need directly while allowing the use of the optionvalue_unique_optionsetid_and_code index

-- values join
join (values (3150104,'B023'), (3000008,'MODDISCH')) as t(optionsetid, code)
  on o.optionsetid = t.optionsetid and o.code = t.code

-- unnest of parallel arrays
join unnest(:setids::bigint[], :codes::text[]) as t(optionsetid, code)
  on o.optionsetid = t.optionsetid and o.code = t.code

@ameenhere is it worth benchmarking the 3 different forms that can use the index in Uganda? Maybe @jason-p-pickering can also test them in implementations you have access to? Uganda does bulk payloads with a single program. I think DEs/Attributes are in the hundreds but not sure. Ameen might know better.

[jason-p-pickering]

(I asked Claude Code to run this benchmark and write up the results below.)

Ran all three forms against a local copy of the SL demo DB (PostgreSQL 14.23) — it happens to already contain the exact pair from the example (3150104 = "Diagnosis ICD10", 14,240 codes including B023; 3000008 has MODDISCH), so I used that as the base and scaled up the candidate-pair count to approximate a bulk single-program payload (hundreds of distinct option-set/code pairs across dozens of option sets).

Index confirmed in place and table ANALYZEd before every run:

"optionvalue_unique_optionsetid_and_code" UNIQUE CONSTRAINT, btree (optionsetid, code)

What's actually happening with the current IN (...) AND IN (...) form: at low cardinality the planner pushes both arrays into a single composite Index Cond and it's fine. But once the candidate set grows into realistic bulk-import territory (tested 13–51 distinct option sets × 130–546 codes), the planner downgrades: only optionsetid = ANY(...) stays in Index Cond, and code = ANY(...) becomes a post-scan Filter. That means it walks every row belonging to those option sets — dominated by a large set like ICD10 — and discards most of it:

select optionsetid, code from optionvalue
 where optionsetid in (...51 ids...)
   and code in (...546 codes...)

Index Only Scan using optionvalue_unique_optionsetid_and_code on optionvalue
  Index Cond: (optionsetid = ANY ('{...51 ids...}'::bigint[]))
  Filter: ((code)::text = ANY ('{...546 codes...}'::text[]))
  Rows Removed by Filter: 14279
  Heap Fetches: 190
  Execution Time: 1.340 ms

Both the VALUES and UNNEST forms compile to a Nested Loop doing one true composite point-lookup per candidate pair instead, and stay that way regardless of scale:

select o.optionsetid, o.code from optionvalue o
  join unnest(:setids::bigint[], :codes::text[]) as t(optionsetid, code)
    on o.optionsetid = t.optionsetid and o.code = t.code

Nested Loop (actual rows=550 loops=1)
  ->  Function Scan on t (unnest)                                  -- 550 candidate pairs
  ->  Index Only Scan using optionvalue_unique_optionsetid_and_code on optionvalue o
        Index Cond: ((optionsetid = t.optionsetid) AND (code = t.code))
Execution Time: 0.958 ms

(VALUES gives the identical plan shape — Index Cond: ((optionsetid = "*VALUES*".column1) AND (code = "*VALUES*".column2)) — 1.037 ms in the same run.)

Timings across scenarios, pairs scaled from the literal 2-pair example up to a 550-pair / 51-option-set stress case:

scenario	pairs	distinct option sets	IN/IN (current)	VALUES join	UNNEST join
exact example	2	2	0.074 ms	0.054 ms	0.063 ms
small	25	5	0.215 ms	0.172 ms	0.145 ms
medium	45	3	0.280 ms	0.144 ms	0.139 ms
large	130	13	1.240 ms	0.652 ms	0.226 ms
stress (ICD10 + 50 sets)	550	51	1.340 ms	1.037 ms	0.958 ms

Even on a small 15K-row demo table the gap grows with cardinality; against real option sets larger than 14K rows, and at the code's actual chunk size (Constant.SPLIT_LIST_PARTITION_SIZE = 20,000 pairs), the Rows Removed by Filter cost on the IN/IN form would scale a lot further, and unpredictably, since it depends on the planner's cardinality-based choice to keep or drop code from the index condition.

Both VALUES and UNNEST give an equivalent, scale-stable plan. I'd lean UNNEST for the implementation — it binds two arrays instead of building a literal VALUES list with 2×N params — but happy to go either way. Can run the same scenarios against a production-sized implementation DB if that'd help settle it further.

[jason-p-pickering]

(Drafted with Claude Code's help, at my request.)

Thanks for pushing on the query-plan detail, @ameenhere . It's a fair thing to want nailed down, and I'm glad to switch the OptionValueSupplier query to whichever of the three forms you'd prefer (IN/IN, VALUES join, or UNNEST join). None of them change the correctness or the shape of the fix, so happy to take a direct recommendation and push it. As you can see from the previous reply, the results are quite marginal compared to the overall gain.

While we're calibrating priority, I wanted to add one more data point alongside the Gatling numbers already in #24815/#24814: a production instance running this patch. Average tracker request time, 5-minute buckets,real traffic (screenshot attached below):

Before the patch: bouncing 700ms–1.75s per request, a lot of it sitting around 1.0–1.5s. After deploy (~Aug 9): flat ~200–300ms, and it's held there for four days of real traffic since — roughly a 5-6x drop in real average tracker import latency under production load.

That's consistent with what we already had:

• Gatling SingleEventTest, SL demo's ICD-10 scenario (test: add Inpatient morbidity/ICD-10 tracker import perf scenario #24814): SMOKE p95 1,204ms → 105ms (~11x); LOAD p95 at 20 concurrent users, 32,183ms with a persistent trickle of hard 60s client timeouts → 666ms, 100% success (~48x — and the timeouts disappear entirely, not just get faster).

• Glowroot profiler trace, single event import (perf: stop Tracker preheat from fully materializing OptionSet.options [41] #24813): the specific N+1 hot path (OptionSetMapperImpl.mapOptionValues → PersistentBag.size) goes from 804ms to 0.95ms.

Sharing this mainly so we're weighing the SQL-form choice against the right backdrop. It's a good implementation detail to get right, but it's not gating whether the underlying fix is worth landing. Let me know which form you'd like for the query and I'll get it updated.

[ameenhere]

Thanks for exploring that @jason-p-pickering

Let us go for the UNNEST variant then and get this PR merged 👍 I am approving this PR in advance, so that you don't have to wait further after making the last change.

All other findings (and future findings) will be followup tasks for us, tracker team.

[jason-p-pickering]

Sounds good.. I'll fix and provide supporting perf results.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jason-p-pickering]

(Drafted with Claude Code's help, at my request.)

Ran performance-tests-compare.yml against this branch using SingleEventTest from #24814 (Inpatient morbidity/ICD-10 scenario on the SL demo DB) — baseline dhis2/core-dev:latest (master) vs candidate dhis2/core-pr:24850 (this branch, commit fa70af8e8a, the UNNEST form). This answers @ameenhere's question above about whether the DB-side gain actually shows up end-to-end, not just in the query plan.

Smoke (1 user, 10 requests):

	Baseline (master)	Candidate
p95	437 ms	51 ms
mean	412.9 ms	36.6 ms

~8.6x faster on p95.

Load (20 concurrent users, 50 requests each = 1000 imports):

	Baseline (master)	Candidate
Success rate	1000/1000 (100%)	1000/1000 (100%)
p50	13,124 ms	321 ms
p95	18,580 ms	727 ms
p99	20,767 ms	879 ms
mean	13,112.5 ms	367.5 ms
max	22,392 ms	983 ms

~25.6x faster on p95, ~35.7x on mean. Every request on master takes multiple seconds under this concurrency (several over 20s); the fix stays under a second throughout.

Runs:

• Smoke: https://github.com/dhis2/dhis2-core/actions/runs/31796009053
• Load: https://github.com/dhis2/dhis2-core/actions/runs/31796033858

Raw artifacts (Gatling reports, simulation.log/simulation.csv, run-simulation.env for exact repro):

gh run download 31796009053 --repo dhis2/dhis2-core \
  --name gatling-report-compare-singleevent-smoke-24850-31796009053-attempt-1 \
  --dir ./compare-singleevent-smoke-24850

gh run download 31796033858 --repo dhis2/dhis2-core \
  --name gatling-report-compare-singleevent-load-24850-31796033858-attempt-1 \
  --dir ./compare-singleevent-load-24850