SQLCase1.js runs the organization tree query two times per request (duplicated UNION ALL arm) #362

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:

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 = '<mapName>')
          )
        ) 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 = '<mapName>')
          )
        ) 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 = '<mapName>'
    )
  ) 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: <N>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 Greenstand/treetracker-infrastructure#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 Greenstand/treetracker-infrastructure#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=<low>&...
   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 = '<mapName>' 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.