Greenstand / Greenstand/treetracker-web-map-api
SQLCase1.js runs the organization tree query two times per request (duplicated UNION ALL arm)
- Dominant language
- JavaScript
- Stars
- 27
- Forks
- 50
- PR merge metrics
- No merged PRs in 30d
Description
**Repository:** `Greenstand/treetracker-web-map-api`
**File:** `src/models/sqls/SQLCase1.js`
**Function:** `getFilter()`, inside the `if (this.mapName)` block (the map/organization filter path).
**Related work:** infrastructure PR `Greenstand/treetracker-infrastructure#325` (raises `work_mem` for `readonlyuser`).
## Summary
The Case 1 query builds a filter `tree_region.tree_id IN (...)`. Inside that filter, the same sub-query runs two times. The two copies are identical. One copy gives the correct result. The second copy does no useful work. It only makes the database do more work and use more memory.
## The code today
The sub-query has three arms, joined by `UNION ALL`, and wrapped by `SELECT DISTINCT`:
```sql
AND tree_region.tree_id IN (
SELECT DISTINCT * FROM (
-- ARM 1
SELECT trees.id AS id FROM trees
INNER JOIN (
SELECT id FROM planter
JOIN (
SELECT entity_id FROM getEntityRelationshipChildren(
(SELECT id FROM entity WHERE map_name = '')
)
) org ON planter.organization_id = org.entity_id
) planter_ids ON trees.planter_id = planter_ids.id
UNION ALL
-- ARM 2 (identical to ARM 1)
SELECT trees.id AS id FROM trees
INNER JOIN (
SELECT id FROM planter
JOIN (
SELECT entity_id FROM getEntityRelationshipChildren(
(SELECT id FROM entity WHERE map_name = '')
)
) org ON planter.organization_id = org.entity_id
) planter_ids ON trees.planter_id = planter_ids.id
UNION ALL
-- ARM 3 (a different arm, keep this one)
SELECT id FROM trees WHERE planting_organization_id = (
SELECT id FROM entity WHERE map_name = ''
)
) t1
)
```
**ARM 1** and **ARM 2** are the same query. They differ only in whitespace, so the generated SQL is the same. Both do the same three steps:
1. They scan and join the `trees` table through `planter_ids` on `trees.planter_id`. The `trees` table is large, so this scan and join is the heavy step.
2. They join `planter` to the organization set on `planter.organization_id`.
3. They call the set-returning function `getEntityRelationshipChildren(...)`. This function walks the `entity` / `entity_relationship` tree from the map root down to every child organization. It returns a small set of organization IDs.
**ARM 3** is different. It selects trees by `planting_organization_id`. This arm is correct and stays.
## Why the duplicate arm is a problem
`UNION ALL` keeps every input row. It does not remove duplicates. PostgreSQL also does not deduplicate identical `UNION ALL` branches during planning. So the planner runs ARM 1 and ARM 2 as two separate scans, and each produces the same rows. The outer `SELECT DISTINCT` then removes the duplicates, so the final result is correct. The database has already paid for the second scan, then thrown its rows away.
The cost appears at two levels:
1. **The `trees` join runs two times.** ARM 1 and ARM 2 each scan and join the large `trees` table in full. This is the main saving from the fix: one fewer full `trees` join. The recursive function and the `planter` join also repeat, but they touch smaller sets.
2. **The `DISTINCT` input is larger than needed.** The input to the `DISTINCT` is `2N + M` rows, where `N` is the organization-tree arm (ARM 1 plus the duplicate ARM 2) and `M` is the `planting_organization_id` arm (ARM 3). Only the `N` part is counted two times. When `N` is much larger than `M`, the `DISTINCT` input is close to two times the needed size, so the sort or hash for the `DISTINCT` is close to two times heavier.
When the session `work_mem` limit is small, the sort or hash for the `DISTINCT` does not fit in memory. PostgreSQL then spills the intermediate rows to temporary files on disk. In the query plan this shows as `Sort Method: external merge Disk: kB` or as a batched `HashAggregate`. Disk spill is far slower than an in-memory operation, and it adds disk I/O to the read replica.
This is exactly the disk-spill load that infrastructure PR https://github.com/Greenstand/treetracker-infrastructure/pull/325 tries to reduce by raising `work_mem` to 64MB. The larger `work_mem` hides this bug, because more of the doubled data now fits in memory. It does not remove the duplicated work. The duplicated `trees` scan, the duplicated join, and the duplicated function call all remain. Removing the duplicate arm cuts that duplicated work at the query source, and helps even after https://github.com/Greenstand/treetracker-infrastructure/pull/325 ships.
## Fix
Delete **ARM 2** and the `UNION ALL` that joins it. Keep two arms:
- **ARM 1**, the organization-tree arm (the `trees` join through `planter.organization_id` and `getEntityRelationshipChildren(...)`), and
- **ARM 3**, the `planting_organization_id` arm.
In the current file this means removing the second identical `SELECT ... UNION ALL` block (lines ~58 to ~68). No other change is needed. The outer `SELECT DISTINCT` already treats the second arm as a no-op, so the returned set of tree IDs does not change.
### Before you delete: confirm the arm is an accidental duplicate
Two identical arms usually mean a copy-paste error. It is also possible the author meant ARM 2 to carry a different filter, for example a different `role` or `type` from `getEntityRelationshipChildren(...)`, or a second organization link. Deleting ARM 2 keeps the current output the same, and the verify step below guards that for the tested inputs. But if a different filter was intended, the delete removes a placeholder for a missing rule, not only dead code. Check `git blame` on this block first to confirm intent.
## How to verify
1. **Correctness (result unchanged).** Call the same tile endpoint before and after the fix, for a large organization such as `freetown` at a low zoom level:
`GET /webmap/trees?map_name=freetown&zoom_level=&...`
The response body must be byte-equivalent. If it changes, stop, because the two arms were not truly identical for that input.
2. **Query plan (spill reduced).** Run `EXPLAIN (ANALYZE, BUFFERS)` on the generated `case1` SQL for `freetown`, before and after the fix. Expect:
- one `trees` join instead of two,
- one call to `getEntityRelationshipChildren` instead of two,
- fewer rows entering the `Sort` or `HashAggregate` node for the `DISTINCT`, and
- a smaller `external merge Disk:` value, or no disk merge at all.
3. **Benchmark cross-check (optional).** Re-run the `work_mem` production benchmark on the `case1 + wallet.token join` family. Expect the per-call temp-file write to drop.
## Related (separate issue)
`map_name = ''` interpolates the value straight into the SQL text (four places in `getFilter()`). If `mapName` reaches this class from a request parameter, it is a SQL injection surface. This is out of scope for this issue and needs its own issue: parameterize `mapName` or validate it against a strict allowlist.
## Context
Found while extracting the heavy Case 1 SQL for the `work_mem` production benchmark. The `case1` organization-filter path is the top temp-file spiller measured on the production read replica.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in src/models/sqls/SQLCase1.js at getFilter(), inside the this.mapName block, and use git blame on the duplicated UNION ALL arm to confirm its intent. Remove the second identical organization-tree arm while keeping the planting_organization_id arm, then verify the case1 SQL with the tile endpoint and EXPLAIN (ANALYZE, BUFFERS) for a large organization such as freetown.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, postgresql
- Domain
- backend, databases, performance
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100