Skip to content

FetchFromClassList fetches every invalid candidate one by one instead of falling back to the single DB query #97

Description

@bero

TBoldDirectMultiLinkController.MakeDbCurrent takes the FetchFromClassList shortcut whenever the other-end class extent (or a superclass extent) is current, to answer a multilink from memory instead of running the one-query DbFetchOwningMember. The shortcut assumes every candidate's single link is current. Nothing checks that assumption, so when it does not hold the shortcut goes to the database once per invalid candidate. (Loaded-ness is already guaranteed: TBoldClassListController drops the extent to non-current on beObjectUnloaded, so a current extent has all its objects loaded.) Found while rehabilitating the two ignored maan_FetchRefetch tests (#83).

Symptom

With the class extent current and three objects whose parent link is invalid, one ObjA.child.EnsureContentsCurrent issues three SELECT statements, each a full default-member row fetch of one object, and leaves all three parent members current. The classic path issues one query and leaves invalid members alone. Recorded by maan_FetchRefetch.TestClassListFetchRefetchesInvalidSingleLinks (5f39dea), which documents the current behaviour without endorsing it.

Root cause

  1. FetchFromClassList (Source/ObjectSpace/BORepresentation/BoldLinks.pas, nested in MakeDbCurrent) loops over the loaded objects of the class extent.
  2. It then reads lBoldObjectReference.Locator. TBoldObjectReference.GetLocator (BoldSystem.pas) starts with EnsureContentsCurrent, so an invalid reference is fetched right there, one object at a time, outside any span fetch.
  3. The result is correct (values come from the database) but the cost is N round trips where the replaced path costs one, and members of unrelated objects change persistence state and notify their subscribers because someone read a different object's multilink.
  4. The same loop shape is used for link-class multilinks (two more FetchFromClassList sites in the same unit, ~1733 and ~2200).

The define's own comment in Bold.inc states the intent: "When fetching an object which class list is loaded then search there instead of going to db." Going to the database N times to avoid going once is the opposite of that intent, so this reads as an unconsidered case rather than a design choice. Systems that invalidate members in bulk (object-space synchronization) are the ones exposed.

Fix

  • FetchFromClassList becomes a function that gives up as soon as memory cannot answer: a candidate's single link is not bvpsCurrent. It returns False without touching anything (the state check does not fetch).
  • MakeDbCurrent runs DbFetchOwningMember when the shortcut is not applicable or gives up: if not (IsCurrentOrSuperClassIsCurrent and FetchFromClassList) then DbFetchOwningMember.
  • Same guard in the two sibling sites: TBoldDirectSingleLinkController.MakeDbCurrent already had an AllMembersLoaded fallback for unassigned members; it now also treats a non-current member as "memory cannot answer". TBoldIndirectMultiLinkController.MakeDbCurrent pre-scans both references of every candidate link object.
  • No new code path: the fallback is the path every installation without the define uses today.

Rejected alternative: batching the invalid links into one query while keeping the shortcut. Same cost as the fallback, keeps the state-changing side effect, needs new batching code.

Files changed

  • Source/ObjectSpace/BORepresentation/BoldLinks.pas
  • UnitTest/Code/ObjectSpace/maan_FetchRefetch.pas

Testing

  • Side effect reproduced and measured: 3 invalid links -> 3 SELECTs (TestClassListFetchRefetchesInvalidSingleLinks, 5f39dea)
  • Test flipped to the specification: no fetch of the invalid members, members stay invalid, list correct (RED before the fix: "Expected [1] but got [3]")
  • Unloaded candidates verified unreachable (extent goes non-current on unload); no test needed
  • Classic path still pinned by TestFetchNonEmbeddedRoleInvalid
  • Full suite green on SQLite and SQL Server (2182/2182, 0 ignored)
  • Daniel: does this match the intent of the FetchFromClassList define?

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

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions