fix(community/performance): address PR #134 review comments

Fixes technical inaccuracies and behavioral issues raised during review
of the community performance knowledge articles.

avoid-currpage-update-in-onaftergetrecord.md
- Removed OnAfterGetCurrRecord from the Best Practice section. That
  trigger is itself implicitly re-entered on every refresh, so placing
  CurrPage.Update(false) inside it can re-trigger the very loop the rule
  warns about. Only OnAction remains as the recommended location.

batch-number-series-instead-of-getnextno-per-row.md
- Scoped the lock/contention claim to gapless (Normal) series only.
  Added explicit statement that Allow Gaps series use NumberSequence
  sequences and do not hold the series-line lock, so the anti-pattern
  detection signal now excludes Allow Gaps series.

countapprox-for-progress-not-count.bad.al / .good.al
- Changed FindSet(true) to FindSet() in both samples. The loop is
  read-only; UpdLock is not needed and was misleading.

countapprox-for-progress-not-count.md
- Qualified the Count() cost claim: it is expensive only when no SIFT
  key covers all filtered fields (forcing a SELECT COUNT(*)); a filtered
  count with matching SIFT coverage is cheap. Added a parenthetical
  noting that SIFT coverage cannot be assumed for arbitrary filters.

dataaccessintent-readonly-on-analytical-objects.md
- Changed bc-version from [all] to ["16.."]. DataAccessIntent was
  introduced at runtime 5.0 / BC 16 and has no effect in earlier
  versions.
- Added precision to the supported objects: pages must be PageType=API
  with Editable=false; for queries, replica routing only applies when
  the query is exposed via OData/API, not for AL-to-AL calls.

httpclient-inside-write-transaction-holds-locks.good.al
- Replaced the Commit()-before-HttpClient pattern with a two-codeunit
  task-deferral pattern. The write completes inside the caller's
  transaction (locks released naturally when it ends); a TaskScheduler
  task runs the HTTP call in a separate session where no write-
  transaction lock is held.

httpclient-inside-write-transaction-holds-locks.md
- Changed Best Practice to recommend TaskScheduler/job queue deferral
  as the primary remedy.
- Added an explicit warning against Commit() as a generic remedy: it
  irrevocably commits all prior writes in the current transaction, so a
  subsequent failure cannot roll them back. Commit() is appropriate only
  at top-level entry points where partial persistence is intentional.

oncompanyopen-subscribers-must-not-do-io.good.al
- Added a ClientType guard so the subscriber exits immediately in
  background task sessions (OnAfterLogin fires there too, which would
  create an unbounded task chain without the guard).
- Added a TaskScheduler.TaskExists idempotency check to avoid queuing
  duplicate tasks on repeated logins.
- Fixed the error-fallback codeunit in CreateTask from a self-reference
  to 0 (no error codeunit).
- Added a 60-second delay (CurrentDateTime() + 60000) so the task does
  not compete with the login session itself.

prefer-related-table-over-extension-on-hot-ledgers.md
- Changed bc-version from [all] to ["23.."]. The companion-table join
  optimisation (single join per base table, automatic exclusion on List/
  OData pages with partial records) was introduced in v23.
- Scoped the "join is always paid" claim: since v23 the join is
  excluded on List/ListPart/OData pages when no extension field is
  loaded under partial-record semantics, but it is still paid on every
  posting path and any AL code that accesses an extension field.

skip-setloadfields-on-write-and-transferfields.bad.al / .good.al / .md
- Changed the example scenario from Modify(false) (which is actually
  valid with a partial record) to TransferFields+Insert into a temporary
  record, which is a documented full-load operation.
- Removed Modify from the list of operations that force a full load.
- Added an explicit note in the .md that Modify itself is not in the
  full-load list; SetLoadFields is safe to use before Modify(false).

use-dedicated-lookup-pages-not-full-lists.bad.al
- Added a separate CardPart page definition (50101) to replace the
  self-referencing FactBox part that referenced the same list page it
  was embedded in. A part cannot refer to its own container page.

validate-on-partial-record-forces-jit.good.al
- Restored SetLoadFields to the good sample so it exercises a partial
  record and the contrast with .bad.al is field selection, not the
  absence of the feature. The good sample loads both Name and Search
  Name (the field Name.OnValidate writes), while the bad sample loads
  only Name, causing a JIT reload of Search Name on every Validate call.

community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.md

@@ -17,7 +17,7 @@ application-area: [all]
 
 ## Best Practice
 
-Put display-only results in page variables assigned in `OnAfterGetRecord` without calling `Update`. If the page must refresh after an action, call `CurrPage.Update(false)` from `OnAction` or `OnAfterGetCurrRecord` once, not per row.
+Put display-only results in page variables assigned in `OnAfterGetRecord` without calling `Update`. If the page must refresh after an action, call `CurrPage.Update(false)` from `OnAction` once, not per row.
 
 See sample: `avoid-currpage-update-in-onaftergetrecord.good.al`.
 

community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.md

@@ -13,7 +13,7 @@ application-area: [all]
 
 ## Description
 
-`Codeunit "No. Series".GetNextNo` updates and locks the number-series line on every call. A tight `Insert` loop that asks for a number per row serializes every concurrent writer on that series — the classic SaaS posting bottleneck. Training data still copies the per-row C/AL `NoSeriesManagement` shape. Codeunit `"No. Series - Batch"` issues numbers in memory and writes the series line once via `SaveState`. `NumberSequence` is the alternative when gaps are acceptable.
+`Codeunit "No. Series".GetNextNo` on a **gapless (Normal)** series updates and locks the number-series line on every call. A tight `Insert` loop that asks for a number per row serializes every concurrent writer on that series — the classic SaaS posting bottleneck. Training data still copies the per-row C/AL `NoSeriesManagement` shape. Series configured with **Allow Gaps** instead obtain numbers through `NumberSequence` and do not hold the series-line lock between calls, so they are not affected by this pattern. Codeunit `"No. Series - Batch"` issues gapless numbers in memory and writes the series line once via `SaveState`.
 
 ## Best Practice
 
@@ -23,6 +23,6 @@ See sample: `batch-number-series-instead-of-getnextno-per-row.good.al`.
 
 ## Anti Pattern
 
-`NoSeries.GetNextNo(...)` inside `repeat ... Insert ... until Next() = 0`. Each iteration takes the series lock. The signal is `"No. Series"` (not `"No. Series - Batch"`) in a loop that inserts more than one row.
+`NoSeries.GetNextNo(...)` inside `repeat ... Insert ... until Next() = 0` where the series is **gapless** (Allow Gaps = false). Each iteration takes the series-line lock. The signal is `"No. Series"` (not `"No. Series - Batch"`) in a loop that inserts more than one row; do not flag the same pattern when the series has Allow Gaps enabled, as the `NumberSequence` path already avoids the lock.
 
 See sample: `batch-number-series-instead-of-getnextno-per-row.bad.al`.

community/knowledge/performance/countapprox-for-progress-not-count.bad.al

@@ -11,7 +11,7 @@ codeunit 50100 "CountApprox Progress Bad"
         // Exact Count() is a SELECT COUNT(*) just to drive a progress bar.
         Total := Customer.Count();
         Window.Open('Processing #1###### of #2######');
-        if Customer.FindSet(true) then
+        if Customer.FindSet() then
             repeat
                 Counter += 1;
                 Window.Update(1, Counter);

community/knowledge/performance/countapprox-for-progress-not-count.good.al

@@ -10,7 +10,7 @@ codeunit 50100 "CountApprox Progress Good"
         Customer.SetRange("Country/Region Code", 'US');
         Total := Customer.CountApprox();
         Window.Open('Processing #1###### of #2######');
-        if Customer.FindSet(true) then
+        if Customer.FindSet() then
             repeat
                 Counter += 1;
                 Window.Update(1, Counter);

community/knowledge/performance/countapprox-for-progress-not-count.md

@@ -13,7 +13,7 @@ application-area: [all]
 
 ## Description
 
-`Count()` asks SQL for an exact row count of the current filter. On a large table that is a `SELECT COUNT(*)` before any useful work starts — the usual cost of `Dialog.Open` with a percentage bar. `CountApprox()` exists for that UI case: it returns a cheap estimate (partition stats / metadata), accurate enough for a progress denominator. Agents default to `Count()` because the name matches "how many rows".
+`Count()` asks SQL for an exact row count of the current filter. When no SIFT key covers all filtered fields, this is a `SELECT COUNT(*)` against the data rows before any useful work starts — the usual cost of `Dialog.Open` with a percentage bar. (A filtered count on a table with a matching SIFT key is cheap, but SIFT coverage cannot be assumed for arbitrary filters.) `CountApprox()` exists for the progress-UI case: it returns a cheap estimate (partition stats / metadata), accurate enough for a progress denominator. Agents default to `Count()` because the name matches "how many rows".
 
 ## Best Practice
 
@@ -23,6 +23,6 @@ See sample: `countapprox-for-progress-not-count.good.al`.
 
 ## Anti Pattern
 
-`Total := Rec.Count(); Window.Open(...);` immediately before a `FindSet` over the same filter. The exact count is discarded after the bar finishes; the user paid a full scan to draw it.
+`Total := Rec.Count(); Window.Open(...);` immediately before a `FindSet` over the same filter. The exact count is discarded after the bar finishes; when the filter is not covered by a SIFT key, the user paid a full table scan just to draw the progress bar.
 
 See sample: `countapprox-for-progress-not-count.bad.al`.

community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.md

@@ -1,5 +1,5 @@
 ---
-bc-version: [all]
+bc-version: ["16.."]
 domain: performance
 keywords: [dataaccessintent, read-only, read-scale-out, report, api-page, query]
 technologies: [al]
@@ -13,11 +13,11 @@ application-area: [all]
 
 ## Description
 
-Reports, API pages, and queries that only read can run against a read replica when `DataAccessIntent = ReadOnly`. Without the property they hit the primary replica and compete with posting. Agents omit it because the default is read-write and the object "only reads" in AL. The replica routing is a metadata switch, not something the compiler infers from the absence of `Modify`.
+`DataAccessIntent` was introduced at runtime 5.0 (BC 16) and has no effect in earlier versions. Reports, API pages (`PageType = API` with `Editable = false`), and queries that only read can run against a read replica when `DataAccessIntent = ReadOnly`. For queries, replica routing only applies when the query is exposed via OData/API; running a query in AL code is unaffected. Without the property these objects hit the primary replica and compete with posting. Agents omit it because the default is read-write and the object "only reads" in AL. The replica routing is a metadata switch, not something the compiler infers from the absence of `Modify`.
 
 ## Best Practice
 
-On report, query, and API page objects that never write, set `DataAccessIntent = ReadOnly`. Keep the default on objects that insert, modify, or call a write codeunit from a processing-only report.
+On report objects and `PageType = API` pages with `Editable = false` that never write, set `DataAccessIntent = ReadOnly`. For query objects, set it when the query is consumed via OData or an API endpoint. Keep the default on objects that insert, modify, or call a write codeunit from a processing-only report.
 
 See sample: `dataaccessintent-readonly-on-analytical-objects.good.al`.
 

community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al

@@ -1,13 +1,26 @@
 codeunit 50100 "HttpClient Holds Locks Good"
 {
+    // Write completes inside the caller's transaction; HTTP deferred so locks are released with it.
     procedure SyncCustomerLastName(var Customer: Record Customer)
-    var
-        Client: HttpClient;
-        Response: HttpResponseMessage;
     begin
         Customer."Search Name" := Customer.Name;
         Customer.Modify(false);
-        Commit();
+        TaskScheduler.CreateTask(Codeunit::"Customer Sync Task", 0, true, CompanyName(), CurrentDateTime());
-        Client.Get(StrSubstNo('https://example.local/sync/%1', Customer."No."), Response);
+    end;
+}
+
+codeunit 50101 "Customer Sync Task"
+{
+    trigger OnRun()
+    var
+        Client: HttpClient;
+        Customer: Record Customer;
+        Response: HttpResponseMessage;
+    begin
+        // Separate session: no write-transaction lock is held during the HTTP call.
+        if Customer.FindSet() then
+            repeat
+                Client.Get(StrSubstNo('https://example.local/sync/%1', Customer."No."), Response);
+            until Customer.Next() = 0;
     end;
 }

community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md

@@ -17,7 +17,7 @@ The first database write opens an AL write transaction that the runtime holds un
 
 ## Best Practice
 
-Finish database writes and `Commit()` (or return from the write execution) before `HttpClient.Send`/`Get`/`Post`. If the call can be slow or retry, isolate it in a job queue or `TaskScheduler` task so UI and other sessions are not sitting on the writer's locks.
+Defer the HTTP call to a `TaskScheduler` task or job queue entry so it runs in a separate session after the write transaction has already ended. The database write completes and releases its locks naturally when the caller's transaction commits; the HTTP call then happens without holding any locks. Do **not** use `Commit()` as a general remedy: it irrevocably commits all prior writes in the current transaction, so any subsequent failure cannot roll them back. `Commit()` is appropriate only at top-level entry points where partial persistence is intentional and understood.
 
 See sample: `httpclient-inside-write-transaction-holds-locks.good.al`.
 

community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.good.al

@@ -3,8 +3,16 @@ codeunit 50100 "Login Subscriber IO Good"
     [EventSubscriber(ObjectType::Codeunit, Codeunit::"System Initialization", OnAfterLogin, '', false, false)]
     local procedure OnAfterLogin()
     begin
-        // Defer HTTP and heavy SQL to a job; session open must return immediately.
+        // Guard to interactive sessions only; background task sessions also raise OnAfterLogin.
-        TaskScheduler.CreateTask(Codeunit::"Login Subscriber IO Work", Codeunit::"Login Subscriber IO Work", true, CompanyName(), CurrentDateTime());
+        if not (Session.GetCurrentClientType() in [ClientType::Web, ClientType::Windows, ClientType::Desktop, ClientType::Tablet, ClientType::Phone]) then
+            exit;
+
+        // Idempotent: skip if a task for this codeunit is already queued.
+        if TaskScheduler.TaskExists(Codeunit::"Login Subscriber IO Work") then
+            exit;
+
+        // Defer the I/O work; CreateTask is the only write allowed on this path.
+        TaskScheduler.CreateTask(Codeunit::"Login Subscriber IO Work", 0, true, CompanyName(), CurrentDateTime() + 60000);
     end;
 }
 

community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.md

@@ -1,5 +1,5 @@
 ---
-bc-version: [all]
+bc-version: ["23.."]
 domain: performance
 keywords: [tableextension, companion-table, gl-entry, related-table, flowfield, hot-table]
 technologies: [al]
@@ -13,16 +13,17 @@ application-area: [all]
 
 ## Description
 
-Stored fields on a table extension live in companion storage that is joined when the base row is read. On hot tables — G/L Entry, Item Ledger Entry, Cust. Ledger Entry — that join is paid on posting, lists, and APIs even when the extra columns are unused. A related table keyed by the ledger `Entry No.`, optionally surfaced with a FlowField or FactBox, leaves the base read path alone. Agents extend G/L Entry because it is "where the posting already is".
+Since v23, all extensions on the same base table share at most one companion-table join, and the platform automatically excludes that join on List, ListPart, and OData pages when partial records are in effect and no extension field is loaded. However, the join is still paid on every posting path and any AL code that accesses an extension field — or that runs without partial-record semantics. On hot tables — G/L Entry, Item Ledger Entry, Cust. Ledger Entry — even a single access per posted row adds up at volume. A related table keyed by the ledger `Entry No.`, optionally surfaced with a FlowField or FactBox, leaves the base read path entirely untouched. Agents extend G/L Entry because it is "where the posting already is".
 
 ## Best Practice
 
-Put optional, sparse, or integration attributes in a related table with the ledger entry number as primary key. Show them from a FactBox or a FlowField. Use a tableextension stored field only when the value must appear as a native list column and is read on almost every access.
+Put optional, sparse, or integration attributes in a related table with the ledger entry number as primary key. Show them from a FactBox or a FlowField. 
+Use a tableextension stored field only when the value must appear as a native list column and is read on almost every access.
 
 See sample: `prefer-related-table-over-extension-on-hot-ledgers.good.al`.
 
 ## Anti Pattern
 
-`tableextension` on `"G/L Entry"` (or another posting table) that adds several stored `Text`/`Blob` fields used only by one integration. Every base-table read now joins those columns.
+`tableextension` on `"G/L Entry"` (or another posting table) that adds several stored `Text`/`Blob` fields used only by one integration. The companion join is paid on every posting and on any AL code path that loads extension fields, even when those columns are not needed for the current operation.
 
 See sample: `prefer-related-table-over-extension-on-hot-ledgers.bad.al`.

community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.bad.al

@@ -1,16 +1,16 @@
 codeunit 50100 "Skip LoadFields Write Bad"
 {
-    procedure UppercaseUsCustomerNames()
+    procedure CopyActiveCustomers(var TempCustomer: Record Customer temporary)
     var
         Customer: Record Customer;
     begin
-        // Partial load plus Modify forces a JIT full-row load per iteration.
+        // TransferFields requires all fields; partial load forces JIT per row.
-        Customer.SetLoadFields(Name);
+        Customer.SetLoadFields("No.", Name);
-        Customer.SetRange("Country/Region Code", 'US');
+        Customer.SetRange(Blocked, Customer.Blocked::" ");
-        if Customer.FindSet(true) then
+        if Customer.FindSet() then
             repeat
-                Customer.Name := UpperCase(Customer.Name);
+                TempCustomer.TransferFields(Customer);
-                Customer.Modify(false);
+                TempCustomer.Insert();
             until Customer.Next() = 0;
     end;
 }

community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.good.al

@@ -1,15 +1,15 @@
 codeunit 50100 "Skip LoadFields Write Good"
 {
-    procedure UppercaseUsCustomerNames()
+    procedure CopyActiveCustomers(var TempCustomer: Record Customer temporary)
     var
         Customer: Record Customer;
     begin
-        // Write path: load the full row. SetLoadFields would JIT on Modify.
+        // TransferFields needs all fields; omit SetLoadFields so the initial read loads the full row.
-        Customer.SetRange("Country/Region Code", 'US');
+        Customer.SetRange(Blocked, Customer.Blocked::" ");
-        if Customer.FindSet(true) then
+        if Customer.FindSet() then
             repeat
-                Customer.Name := UpperCase(Customer.Name);
+                TempCustomer.TransferFields(Customer);
-                Customer.Modify(false);
+                TempCustomer.Insert();
             until Customer.Next() = 0;
     end;
 }

community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.md

@@ -13,16 +13,16 @@ application-area: [all]
 
 ## Description
 
-`SetLoadFields` is a read optimization. `Insert`, `Modify`, `Delete`, `Rename`, `TransferFields`, and copy into a temporary record all require a fully loaded row. When those operations run on a partial record, the platform issues a just-in-time load of the missing fields. That extra round-trip costs more than loading the full row on the original `FindSet` or `Get`. Agents that apply `use-setloadfields-for-partial-records.md` to every loop therefore make write loops slower, not faster.
+`SetLoadFields` is a read optimization. The platform's [partial-record usage guidelines](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-partial-records#usage-guidelines) list the operations that require every field to already be present: `Insert`, `Delete`, `Rename`, `TransferFields`, and copying a record into a temporary table. When those operations run on a partial record, the platform issues a just-in-time load of the missing fields. That extra round-trip costs more than loading the full row on the original `FindSet` or `Get`. Note: `Modify` itself is **not** in this list — a `Modify(false)` that only touches loaded fields is safe with a partial record.
 
 ## Best Practice
 
-Use `SetLoadFields` only when the subsequent access is read-only. On a loop that writes the iterated record, or copies it with `TransferFields` / `Copy` onto a temporary record, omit `SetLoadFields` so the initial read already materializes every field those operations need.
+Omit `SetLoadFields` on loops whose body performs a documented full-load operation (`Insert`, `Delete`, `Rename`, `TransferFields`, or assignment into a temporary record) on the same record variable, so the initial read already materializes every field those operations need.
 
 See sample: `skip-setloadfields-on-write-and-transferfields.good.al`.
 
 ## Anti Pattern
 
-Calling `SetLoadFields` immediately before a `FindSet` whose body `Modify`s, `Delete`s, `Rename`s, or `TransferFields`s the same record. The review signal is partial-record setup on a record variable that is written or copied in the same iteration, not the mere presence of `SetLoadFields` on a read-only loop.
+Calling `SetLoadFields` immediately before a `FindSet` whose body performs `Delete`, `Rename`, `TransferFields`, or copies the record into a temporary table. The review signal is a partial-record setup on a record variable that feeds one of these documented full-load operations in the same iteration.
 
 See sample: `skip-setloadfields-on-write-and-transferfields.bad.al`.

community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.bad.al

@@ -37,7 +37,24 @@ page 50100 "Campaign Member List"
         }
         area(factboxes)
         {
-            part(Details; "Campaign Member List") { }
+            // Full list carries this FactBox on every dropdown open — expensive.
+            part(Details; "Campaign Member Details FB") { }
+        }
+    }
+}
+
+page 50101 "Campaign Member Details FB"
+{
+    PageType = CardPart;
+    SourceTable = "Campaign Member";
+
+    layout
+    {
+        area(content)
+        {
+            field("No."; Rec."No.") { }
+            field(Name; Rec.Name) { }
+            field("Balance (LCY)"; Rec."Balance (LCY)") { }
         }
     }
 }

community/knowledge/performance/validate-on-partial-record-forces-jit.good.al

@@ -4,10 +4,12 @@ codeunit 50100 "Validate Partial Rec Good"
     var
         Customer: Record Customer;
     begin
+        // Include every field that Name.OnValidate reads so the runtime never JIT-loads.
+        Customer.SetLoadFields(Name, "Search Name");
         Customer.SetRange("Country/Region Code", 'US');
         if Customer.FindSet(true) then
             repeat
-                Customer.Name := UpperCase(Customer.Name);
+                Customer.Validate(Name, UpperCase(Customer.Name));
                 Customer.Modify(false);
             until Customer.Next() = 0;
     end;