Repository navigation
Conversation
`id != any($excluded)` holds as soon as the id differs from one element of the array, so with two or more excluded keys no row was excluded. `Queue::get_derived` reads the database at the block before the queue and relies on this list to hide children that queued blocks remove or rewrite, so `loadRelated` could return entities that a queued block had already removed. Use `id <> all($excluded)`. Add store tests that hold the writer and call `get_derived` while the block is queued: one removal (control), two removals, and a removal plus a creation.
In `Queue::get_derived`, a queued write whose `@derivedFrom` field no longer points to the parent was dropped instead of excluding its key. The database, read at the block before the queue, then still returned the previous version of that child under the old parent, so `loadRelated` could return a child that a queued block had moved to another parent. A newer queued version always supersedes the database row, related or not: exclude its key. Add a store test that moves a child to another parent in a queued block.
dimitrovmaksim
left a comment
dimitrovmaksim
left a comment
There was a problem hiding this comment.
The root-cause analysis checks out, and the fix is correct for mutable entities. One skipDuplicates case is inline.
A question on cost: with commit 78e60f0 the exclusion list grows from the parent's related queued writes to every queued key of the type, and <> all has to check each candidate row against that list. 4d2479c moved this list to a single array bind to avoid potentially hitting the Postgres bind-parameter limit (65,535), so lists that large were already anticipated with the smaller set
Queue::get_many settles queue-vs-database precedence in Rust, and FindDerivedQuery has no LIMIT or ORDER BY. If get_derived dropped database rows whose id is in entities_in_queue in the Rust code instead, would the result differ? Would excluded_keys (and the != any / <> all question) still need to exist?
| .map(|op| match op { | ||
| EntityOp::Write { key, entity } if is_related(derived_query, entity) => { | ||
| Some((key.clone(), Some(entity.clone()))) | ||
| (key.clone(), Some(entity.clone())) | ||
| } | ||
| EntityOp::Write { .. } => None, | ||
| EntityOp::Remove { key } => Some((key.clone(), None)), | ||
| // A newer queued version supersedes the database row even | ||
| // when it is no longer related: exclude its key | ||
| EntityOp::Write { key, .. } => (key.clone(), None), | ||
| EntityOp::Remove { key } => (key.clone(), None), | ||
| }) |
There was a problem hiding this comment.
#6458 introduced skipDuplicates for immutable entities: a duplicate insert doesn't fail the subgraph; when it reaches the table, ON CONFLICT DO NOTHING drops it, so the stored version stays.
With that in mind, does "a newer queued version supersedes the database row" hold for a queued duplicate of a skipDuplicates entity? For example, with a stored t { parent: p0 } and a queued duplicate t { parent: p1 }, what does loadRelated(p0) return while the block is queued, and after the flush?
Not sure how often skipDuplicates is used, but it is a real path.
Fixes #6726.
Queue::get_derivedcombines queued changes with a database read at the block before the queue, and hides the queued keys from that read throughexcluded_keys. Two defects let older database versions through, soloadRelatedcould return children that a queued block had removed or moved to another parent. Whether that happened depended on writer timing.Two commits, which can be reviewed separately:
store: Exclude every listed key in FindDerivedQuery:id != any($excluded)→id <> all($excluded). With two or more keys, the old predicate excluded nothing.store: Exclude queued keys no longer related in get_derived: ineffective_ops, a queued write that is no longer related to the parent now excludes its key instead of being dropped. A newer queued version always supersedes the database row, whether or not it is still related.Tests: four store tests in
store/test-store/tests/postgres/writable.rs(get_derived_pending_*). They hold the writer, callget_derivedwhile a block is queued, then flush. Without the fixes:two_removalsandremoval_and_creationfail without commit 1;parent_changefails without commit 2;one_removalis a control that passed before.The existing
get_derived_batchandget_derived_nobatchstill pass.Notes:
if !self.excluded_keys.is_empty()).<> all(array)has the same planner characteristics as!= any(array).loadRelatedon children moved or removed while blocks are queued, results and therefore PoIs can differ from released versions. The current behaviour is timing-dependent, so this probably deserves a release note.Checks run locally on this branch:
cargo fmt --all -- --check,cargo clippy --all-targets(no warnings),cargo check --release,cargo test -p graph-store-postgres --lib,cargo test -p test-store --test postgres, and the workspace unit tests.