Skip to content

loadRelated can return stale children while blocks are queued for writing (!= any in FindDerivedQuery; unrelated queued writes not excluded) #6726

Description

@madumas

Summary

With the pipelined writer (the default, GRAPH_STORE_WRITE_QUEUE > 0), store.loadRelated (EntityCache::load_related → WritableStore::get_derived → Queue::get_derived) can return children that a queued block has already removed, or has already moved to another parent. The answer depends on how far the background writer has progressed. Two indexers, or two runs of the same indexer, can therefore see different loadRelated results for the same block. When a mapping derives what it writes from that result, which is the usual reason to call loadRelated, entities and the PoI can diverge.

Queue::get_derived merges the queued changes with a database read taken at the block before the queue. The keys found in the queue are passed as excluded_keys, so the database does not return an older version of them. There are two independent problems with this exclusion.

A. != any($excluded) excludes nothing once two or more keys are listed

store/postgres/src/relational_queries.rs, FindDerivedQuery (master 6838f4e, line 2244):

out.push_identifier(&self.table.primary_key().name)?;
out.push_sql(" != any(");
self.excluded_keys.push_bind_param(&mut out)?;
out.push_sql(") and ");

id != any(array) is true when id differs from at least one element of the array. With one key it behaves like id <> key. With two or more keys, every row satisfies it, so nothing is excluded, and a child that a queued block removes comes back from the database. The intended predicate is id <> all(array). This was introduced by 4d2479c ("store: Avoid 'too many bind params' error in FindDerivedEntityQuery", April 2024), which replaced not in (...) with != any(...). The first release that contains it is v0.35.1.

B. A queued write that no longer points to the parent does not exclude its key

store/postgres/src/writable.rs, Queue::get_derived → effective_ops (master 6838f4e, lines 1340-1355):

EntityOp::Write { key, entity } if is_related(derived_query, entity) => Some((key.clone(), Some(entity.clone()))),
EntityOp::Write { .. } => None,
EntityOp::Remove { key } => Some((key.clone(), None)),

If a child changes its @derivedFrom field (moves from p0 to p1) in a queued block, the queued write is not related to p0. It yields None, so its key is not excluded, and the database still returns the old version under p0. A single queued key is enough. This form of the match dates from 12427fd (v0.31.0).

Reproduction

Four deterministic store tests, added in the accompanying PR to store/test-store/tests/postgres/writable.rs (using the existing pause_writer helper):

  1. Write block 1 to the database: parents p0 and p1; children w and x under p0.
  2. Pause the writer (pause_writer, which uses allow_steps).
  3. Queue an empty block 2. It absorbs a writer that may already be past its pause point; the test asserts that the database head is still ≤ 2.
  4. Queue block 3, call get_derived for the children of p0 while block 3 is queued, then flush and call it again.
case queued block 3 expected returned while queued, before the fix
one removal (control) remove x [w] [w]
two removals (A) remove w, remove x [] [w, x]
removal + creation (A) remove x; insert y under p0 [w, y] [w, x, y]
parent change (B) x.parent: p0 → p1 [w] [w, x]

After flush every case returns the expected value. The failures do not depend on timing: the queued block is held, and the tests fail deterministically.

We also built an end-to-end runner fixture. Children are moved and removed in some blocks, and loadRelated results are written to an immutable entity in later blocks. With the writer holding a batch, the recorded results and the PoI differ from a run with a synchronous writer. With the natural batching behaviour, the result also varied between runs.

Possibly related

#5866 ("Load Related requests not deterministic") reported a PoI discrepancy where the number of entities returned from the database and the queue varied between runs, with the root cause unidentified; it was closed for inactivity. Both defects above produce exactly that symptom, so they may be its root cause. The pattern that triggers case A (loading children with loadRelated, then removing several of them in the same handler) is common in production mappings.

A PR with both fixes and the tests above follows.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions