Skip to content

fix(retain): match chunk-delete link endpoints through indexable joins (#3387) - #3393

Merged
nicoloboschi merged 2 commits into
mainfrom
fix/3387-chunk-link-delete-seqscan
Aug 11, 2026
Merged

fix(retain): match chunk-delete link endpoints through indexable joins (#3387)#3393
nicoloboschi merged 2 commits into
mainfrom
fix/3387-chunk-link-delete-seqscan

Conversation

@nicoloboschi

@nicoloboschi nicoloboschi commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #3387.

The problem

The ordered memory_links pre-delete in delete_chunks_by_ids matched link endpoints with a single predicate:

WHERE EXISTS (SELECT 1 FROM target_units tu
              WHERE tu.id = ml.from_unit_id OR tu.id = ml.to_unit_id)

An OR spanning two columns of ml cannot be driven from either endpoint index, so PostgreSQL makes memory_links the outer relation of a nested-loop semi join and sequentially scans the whole table on every delete — O(rows_in_memory_links x target_units). There is no bank_id predicate either, so each bank's delta retain scans every other bank's rows too.

Past a few million links this exceeds the asyncpg command timeout and delta retain fails with a bare TimeoutError. Because str(TimeoutError()) is empty, it surfaces as Task execution failed: batch_retain, error: with nothing after it.

The fix

Split the OR into a UNION of two single-column joins, so each half is an index scan on the existing idx_memory_links_from_type_weight / idx_memory_links_to_type_weight. The deterministic ORDER BY and FOR UPDATE OF ml that #2570 introduced stay in ordered_links, which still locks the rows in that order (LockRows sits above Sort in the plan, before and after).

Measurements

Reproduced on PostgreSQL 18 with the current shipped index set, 300k units / 1.5M links, running the emitted statement verbatim (the full DELETE ... USING, not a count):

rows deleted execution
before, 3 chunks (150 units) 1,464 20,420 ms — Seq Scan, 224.9M rows removed by join filter
before, 10 chunks (500 units) 4,973 49,828 ms
after, 3 chunks (150 units) 1,464 31 ms — two index scans

10 changed chunks already blew the 60s default at a table size well below production, which matches the reporter's finding that raising the timeout to 120s did not move the failure rate.

Row sets are identical — EXCEPT in both directions returns zero.

Tests

  • test_delete_chunks_by_ids_matches_link_endpoints_through_indexable_joins — asserts the endpoint match stays two single-column joins and the OR-across-columns predicate is not reintroduced.
  • test_delete_chunks_by_ids_sweeps_links_on_both_endpoints — real-DB equivalence guard: a link is now matched twice, once from each side, so a rewrite that dropped one arm would silently leave half the links to the FK cascade and reopen the Fix chunk delete deadlock ordering #2570 deadlock. Seeds a link in each direction on the doomed unit plus one between survivors, and asserts only the survivors' link remains.

Deliberately not in this PR

  • No bank_id predicate. The UNION removes the whole-table scan, which was the actual cost; bank_id would be redundant given uuid endpoints, and bank_id is an optional argument of this function.
  • The insert-side lock ordering. The reporter notes the Fix chunk delete deadlock ordering #2570 deadlock has not disappeared for them. link_utils.bulk_insert_links sorts by (from_unit_id, to_unit_id) while this delete orders by (LEAST, GREATEST, link_type, COALESCE(entity_id, ...)) — different orders, so INSERT-vs-DELETE cycles are not covered by either. That is a separate defect from the timeout and is worth its own issue.
  • Oracle. This statement is already PostgreSQL-only — ctid and DELETE ... USING pass through _rewrite_pg_to_oracle untouched, and neither is valid Oracle. The rewrite keeps exactly that shape, so Oracle behaviour is unchanged. Filed separately rather than widened into this fix.

Also in this PR: two test doubles broken by #3388

test-api shard 2 has been red on main since #3388 landed — unrelated to this fix, but it blocks the shard, so it is repaired here:

  • test_integrity_violation_not_retried stubbed the store as SimpleNamespace(writes_memory_rows_in_sql=True); the capability is now consulted per bank as writes_memory_rows_in_sql_for(bank_id).
  • test_list_banks_non_sql_store._NonSqlStore lacked drop_bank_storage, which the test's own delete_bank teardown routes through the store for a non-SQL bank.

Both doubles are hand-rolled rather than subclasses of the store base, which is why #3388 could not update them mechanically.

#3387)

The ordered memory_links pre-delete in delete_chunks_by_ids matched link
endpoints with `tu.id = ml.from_unit_id OR tu.id = ml.to_unit_id`. An OR
spanning two columns of ml cannot be driven from either endpoint index, so
PostgreSQL made memory_links the outer relation of a nested-loop semi join
and sequentially scanned the whole table on every delete — O(links x units),
with no bank_id predicate, so every bank scanned every other bank's rows.

Past a few million links that exceeded the asyncpg command timeout and delta
retain failed with a bare TimeoutError (str(TimeoutError()) is empty, so it
logged as "Task execution failed: batch_retain, error:" with nothing after).

Splitting the OR into a UNION of two single-column joins makes each half an
index scan. Measured on PG18 with the current index set, 300k units /
1.5M links, deleting the links of 3 chunks (150 units):

  before  20,420 ms   Seq Scan, 224.9M rows removed by join filter
  after       31 ms   two index scans, same 1,464 rows

10 chunks took 49.8s before — already over the 60s default at a table size
well below production. The row set is identical (EXCEPT in both directions
returns nothing), and the deterministic ORDER BY + FOR UPDATE that #2570
added stay in ordered_links, which still locks the rows in that order.
#3388 moved the store capability to writes_memory_rows_in_sql_for(bank_id) and
added drop_bank_storage, but two duck-typed test doubles still carried the old
surface, so test-api shard 2 has been red on main since it merged:

  test_integrity_violation_not_retried — SimpleNamespace(writes_memory_rows_in_sql=True)
    -> AttributeError: no attribute 'writes_memory_rows_in_sql_for'
  test_list_banks_non_sql_store._NonSqlStore — teardown's delete_bank routes the
    drop through the store for a non-SQL bank
    -> AttributeError: no attribute 'drop_bank_storage'

Both doubles are hand-rolled rather than subclasses of the store base, which is
why the refactor could not update them mechanically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

delete_chunks_by_ids: ordered memory_links pre-delete seq-scans the whole table — delta retain times out at scale

1 participant