mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Extract 55 knowledge articles from BC review-agent prompt
Adds 55 articles (plus 76 code samples) spanning four new domains and two existing domains, extracted from the internal Business Central review-agent prompt. Content was filtered against BCQuality's remedial-knowledge premise: each article encodes BC-specific behaviour, a CodeCop rule, a platform API semantic, or an anti-false-positive guideline that a capable LLM would otherwise get wrong. New domains: - privacy (11 articles): DataClassification inheritance semantics, the StrSubstNo-defeats-Error-telemetry-classification pitfall, Privacy Notice consent for outgoing requests, anti-false-positives for pages and in-memory data. - upgrade (11 articles): upgrade-codeunit structure, upgrade-tag lifecycle and registration, protected DB reads, DataTransfer for large datasets, InitValue semantics, enum-ordinal preservation, obsolete-workflow, first-install detection. - ui (9 articles): caption capitalization by phrase type, tooltip voice, teaching-tip vs tooltip, tour-tip conventions, character limits, banned terms, ampersand handling, title punctuation. - style (11 articles): label-suffix convention, API page naming, temporary-variable prefix, label properties (Comment/Locked), named invocations, FieldCaption in user messages, OptionCaption pairing, Error-parameter passing, `this` keyword, required parentheses, file naming. Gaps in existing domains: - performance (11 articles): production-scale table catalog (no row counts, per internal-data concern), anti-false-positive for bounded tables, guard-before-Get ordering, redundant-Get-in-OnAfterGetRecord, LockTable in read-only helpers, combined ModifyAll passes, writes in OnAfterGetRecord, SetLoadFields heuristics, temporary-table regressions, FlowField source-table widening, MaintainSQLIndex disabling SIFT. - security (2 articles): environment-specific hardcoded GUIDs, ValidateTableRelation=false on user input. Intentionally excluded: specific production P95 row-count numbers (aggregated internal telemetry); rewritten as categorical guidance on which tables to treat as production-scale without publishing sizes. All articles use `bc-version: [all]` (applies to every BC version, per the new schema sentinel). Validator passes with 0 errors / 0 warnings.
This commit is contained in:
Jesper Schulz-Wedde
parent
9a4198eb28
commit
e570d6113f
131 changed files with 2799 additions and 0 deletions
|
|
@ -0,0 +1,13 @@
|
|||
codeunit 51207 "Perf Sample CombineMA Bad"
|
||||
{
|
||||
procedure UpdateTolerance(DocumentNo: Code[20]; ToleranceAmount: Decimal)
|
||||
var
|
||||
CustLedgerEntry: Record "Cust. Ledger Entry";
|
||||
begin
|
||||
CustLedgerEntry.SetRange("Document No.", DocumentNo);
|
||||
CustLedgerEntry.SetRange(Open, true);
|
||||
// Two scans over the same filtered rows on a 10M-row ledger table.
|
||||
CustLedgerEntry.ModifyAll("Accepted Payment Tolerance", ToleranceAmount);
|
||||
CustLedgerEntry.ModifyAll("Accepted Pmt. Disc. Tolerance", false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 51206 "Perf Sample CombineMA Good"
|
||||
{
|
||||
procedure UpdateTolerance(DocumentNo: Code[20]; ToleranceAmount: Decimal)
|
||||
var
|
||||
CustLedgerEntry: Record "Cust. Ledger Entry";
|
||||
begin
|
||||
CustLedgerEntry.SetRange("Document No.", DocumentNo);
|
||||
CustLedgerEntry.SetRange(Open, true);
|
||||
if CustLedgerEntry.FindSet(true) then
|
||||
repeat
|
||||
CustLedgerEntry."Accepted Payment Tolerance" := ToleranceAmount;
|
||||
CustLedgerEntry."Accepted Pmt. Disc. Tolerance" := false;
|
||||
CustLedgerEntry.Modify(false);
|
||||
until CustLedgerEntry.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [modifyall, bulk-update, filter, scan, recordset]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Combine multiple ModifyAll calls on the same recordset into a single pass
|
||||
|
||||
## Description
|
||||
|
||||
`ModifyAll(Field, Value)` issues a SQL UPDATE against every row matching the record variable's current filters, setting one field. Calling it twice on the same filtered recordset — once per field to update — produces two separate UPDATE statements, each of which has to re-locate the matching rows through the index. On a ledger-entry-scale table with ten million rows and a filter that matches a thousand, the overhead is not a doubling of the update cost but a doubling of the more expensive row-location cost. A single `FindSet(true)` + set-by-set assignment + `Modify(false)` completes both field changes in one pass.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When more than one field needs to change on the same filtered recordset, iterate once with `FindSet(true)` and assign all fields per row. Reserve ModifyAll for the case where a single field change covers the whole update. If the filter set is truly huge and the trigger behaviour differs between fields, consider splitting with concrete evidence — otherwise the single-pass loop wins.
|
||||
|
||||
See sample: `combine-multiple-modifyall-calls.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Applying `SetRange` against `CustLedgerEntry` on `"Document No."` and then calling `ModifyAll("Accepted Payment Tolerance", ...)` followed by `ModifyAll("Accepted Pmt. Disc. Tolerance", false)` — two scans over the same filtered rows. On Cust. Ledger Entry with production-scale data the redundant second scan is the dominant cost.
|
||||
|
||||
See sample: `combine-multiple-modifyall-calls.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [setup-table, temporary, bounded-table, metadata, migration, false-positive]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not flag performance on inherently bounded tables
|
||||
|
||||
## Description
|
||||
|
||||
Several categories of Business Central tables are so small, so rarely accessed, or so in-memory that performance heuristics that make sense on Item Ledger Entry produce noise when applied to them. Temporary records (`TableType = Temporary`, `SourceTableTemporary = true`) live in memory and any access pattern is fast. Singleton setup tables (`Sales & Receivables Setup`, `General Ledger Setup`, `*Setup` tables generally) hold one row per company. Small bounded tables — enum mappings, permission objects, Role IDs — count in the dozens. System metadata tables (`TableMetadata`, `Field`, `AllObjWithCaption`) are bounded by the object catalog. Admin, Migration, Setup, Wizard, and Hybrid* pages are used infrequently with small datasets.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Skip performance findings on these categories unless the code is specifically pathological (unbounded loop that multiplies cost non-linearly). A missing SetLoadFields on a singleton Setup table is not a finding. A Count on a 30-row permission mapping is not a finding. An admin page that iterates a bounded list once per invocation is not a finding. Reserving reviewer attention for the tables where it matters is half the value of the heuristics — noise on bounded tables trains authors to ignore the signal.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Flagging `SalesReceivablesSetup.Get()` followed by `SetLoadFields()` on a handful of fields as "missing partial record optimization". Flagging a `FindSet` + loop on `Role ID` mapping because the loop has no SetCurrentKey. Flagging a Migration codeunit for writing many records, when the entire migration runs once per customer. All three burn author attention on cases that are not regressions.
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
pageextension 51209 "Perf Sample NoModifyOAGR Bad" extends "Customer List"
|
||||
{
|
||||
trigger OnAfterGetRecord()
|
||||
begin
|
||||
// Every scroll writes to the database. Every OnModify subscriber on
|
||||
// Customer fires alongside. Write volume scales with mouse-wheel speed.
|
||||
Rec."Last Warning Flag" := CalcWarning();
|
||||
Rec.Modify();
|
||||
end;
|
||||
|
||||
local procedure CalcWarning(): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,35 @@
|
|||
page 51208 "Perf Sample NoModifyOAGR Good"
|
||||
{
|
||||
PageType = List;
|
||||
SourceTable = Customer;
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
repeater(Group)
|
||||
{
|
||||
field("No."; Rec."No.") { ApplicationArea = All; }
|
||||
field(WarningFlag; ShowWarning)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Warning';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnAfterGetRecord()
|
||||
begin
|
||||
// Page-local variable. No database write per row.
|
||||
ShowWarning := CalcWarning(Rec);
|
||||
end;
|
||||
|
||||
var
|
||||
ShowWarning: Boolean;
|
||||
|
||||
local procedure CalcWarning(var Customer: Record Customer): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [onaftergetrecord, modify, page, trigger, write-per-scroll]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not Modify records inside OnAfterGetRecord
|
||||
|
||||
## Description
|
||||
|
||||
`OnAfterGetRecord` fires for every row the page or repeater renders. On a list page the user scrolls through, the trigger runs hundreds of times per second. A `Modify()` call inside the trigger writes to the database for every row scrolled past — the user's mouse wheel generates the write storm, and the effect compounds with every other subscriber that reacts to the OnModify event. The database activity is usually invisible to the author in development, because the list page loads ten rows; on a production tenant scrolling through thousands of rows, the page becomes the top source of write volume.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Derive display-only state into a page-level variable and bind that variable to the field control instead of writing to `Rec`. If the computed value is genuinely a stored attribute of the record, compute it once at the authoring site (OnValidate, OnInsert) and display the stored value on the list — do not recompute and rewrite on every render.
|
||||
|
||||
See sample: `do-not-modify-records-in-onaftergetrecord.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An OnAfterGetRecord body that assigns a computed value to `Rec."Warning Flag"` and calls `Rec.Modify()` so the flag persists. The write fires per scroll, per user, per second — and every subscriber on the Rec's OnModify fires alongside.
|
||||
|
||||
See sample: `do-not-modify-records-in-onaftergetrecord.bad.al`.
|
||||
|
|
@ -0,0 +1,34 @@
|
|||
page 51203 "Perf Sample ReGetRec Bad"
|
||||
{
|
||||
PageType = List;
|
||||
SourceTable = "Assembly Line";
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
repeater(Group)
|
||||
{
|
||||
field("No."; Rec."No.") { ApplicationArea = All; }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnAfterGetRecord()
|
||||
var
|
||||
AssemblyLineRec: Record "Assembly Line";
|
||||
begin
|
||||
// Redundant Get. The page runtime already loaded this row into Rec.
|
||||
// At list-page scale this fires hundreds of times per scroll.
|
||||
AssemblyLineRec.Get(Rec."Document Type", Rec."Document No.", Rec."Line No.");
|
||||
ShowWarning := CheckAvailability(AssemblyLineRec);
|
||||
end;
|
||||
|
||||
var
|
||||
ShowWarning: Boolean;
|
||||
|
||||
local procedure CheckAvailability(var AssemblyLine: Record "Assembly Line"): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
page 51202 "Perf Sample ReGetRec Good"
|
||||
{
|
||||
PageType = List;
|
||||
SourceTable = "Assembly Line";
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
repeater(Group)
|
||||
{
|
||||
field("No."; Rec."No.") { ApplicationArea = All; }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnAfterGetRecord()
|
||||
begin
|
||||
// Rec already holds the current row's values; no Get needed.
|
||||
ShowWarning := CheckAvailability(Rec);
|
||||
end;
|
||||
|
||||
var
|
||||
ShowWarning: Boolean;
|
||||
|
||||
local procedure CheckAvailability(var AssemblyLine: Record "Assembly Line"): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [onaftergetrecord, get, rec, page-runtime, redundant-fetch]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not re-Get the current record inside OnAfterGetRecord
|
||||
|
||||
## Description
|
||||
|
||||
The page runtime loads the current record before firing `OnAfterGetRecord` — `Rec` already holds the row's values when the trigger body runs. Calling `Rec.Get(...)` (or any equivalent Get against the same key) inside the trigger issues a second database round-trip for data the runtime just fetched. On a list page that displays hundreds of rows during a scroll, this turns into hundreds of wasted round-trips per user interaction. The same concern applies to `OnAfterGetCurrRecord` on card and document pages, though the impact is smaller because the trigger fires per selection rather than per row.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Read from `Rec` directly. When a helper method needs a different record, pass `Rec` as an argument or let the helper fetch its own lookup once; do not re-Get the current row. If the code truly needs a fresh value because it was modified by another session, design the refresh explicitly — document it in a comment — rather than paying the cost on every trigger fire.
|
||||
|
||||
See sample: `do-not-re-get-rec-inside-onaftergetrecord.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An `OnAfterGetRecord` trigger body that starts with `AssemblyLineRec.Get("Document Type", "Document No.", "Line No.")` for the same keys the page runtime has already used — the Get restates what `Rec` already holds. Replace with a direct call against `Rec` (`CheckAvailability(Rec)`).
|
||||
|
||||
See sample: `do-not-re-get-rec-inside-onaftergetrecord.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [flowfield, calcformula, regression, source-table, sift]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not retarget a FlowField's CalcFormula to a larger source table
|
||||
|
||||
## Description
|
||||
|
||||
A FlowField's CalcFormula is evaluated every time the field is read — every time the page renders, every CalcFields call, every list page filter that references the field. Changing the CalcFormula's source table from a smaller, bounded, or already-filtered table to a larger unfiltered one multiplies the per-read cost. A common shape is the refactor from "Posted X" to "X" — the unposted line table is typically an order of magnitude larger and carries rows that the original FlowField never considered. The change compiles and may look like a simple scope widening; the performance impact is not visible until production load.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a FlowField CalcFormula changes source table, evaluate the before/after row counts, ensure a SIFT key exists on the new source that matches the formula's filters (see `add-sift-keys-for-flowfields`), and verify no existing callers rely on the tighter scope. If the widening is intentional, the corresponding SIFT keys on the new source must ship in the same PR.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Changing a `sum("Posted Expense Report Line"."Amount" where(...))` formula to `sum("Expense Report Line"."Amount" where(...))` without touching the source table's keys. Every list page and dashboard that reads the FlowField now aggregates over the unposted table too, almost always without a supporting SIFT key.
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 51201 "Perf Sample GuardBeforeGet Bad"
|
||||
{
|
||||
procedure HandleLine(var PurchaseLine: Record "Purchase Line")
|
||||
var
|
||||
PurchaseHeader: Record "Purchase Header";
|
||||
begin
|
||||
// Get fires on every call — including the ones that exit immediately below.
|
||||
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");
|
||||
|
||||
if PurchaseLine."Selected Alloc. Account No." = '' then
|
||||
exit;
|
||||
|
||||
// Work with PurchaseHeader.
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 51200 "Perf Sample GuardBeforeGet Good"
|
||||
{
|
||||
procedure HandleLine(var PurchaseLine: Record "Purchase Line")
|
||||
var
|
||||
PurchaseHeader: Record "Purchase Header";
|
||||
begin
|
||||
// Cheap in-memory check first. Get only when the subsequent code needs the header.
|
||||
if PurchaseLine."Selected Alloc. Account No." = '' then
|
||||
exit;
|
||||
|
||||
if not PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.") then
|
||||
exit;
|
||||
|
||||
// Work with PurchaseHeader.
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [get, guard, early-exit, wasted-fetch, conditional]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Place guard conditions before Get, not after
|
||||
|
||||
## Description
|
||||
|
||||
A `Record.Get(Key)` is a database round-trip. When the call site also contains an early-exit condition that may fire before the fetched record is used, the order of the two matters: `Get` first followed by a guard that may exit means every call pays the round-trip, including the calls that immediately return. Flipping the order — evaluate the guard first, `Get` only when needed — costs nothing in the happy path and turns the wasted round-trip into zero work on the exit path. The savings compound on hot tables and on code paths entered many times per user action.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Evaluate cheap, in-memory conditions first. Only issue the `Get` (or `FindFirst`, `FindLast`) when the subsequent code actually needs the record's values. For complex procedures with multiple exit conditions, sort them cheapest-first: in-memory checks, then single-record lookups, then set iteration.
|
||||
|
||||
See sample: `guard-before-get-not-after.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No."); if PurchaseLine."Selected Alloc. Account No." = '' then exit;` — the Get fires on every call; the exit discards the result for every call where `Selected Alloc. Account No.` is blank.
|
||||
|
||||
See sample: `guard-before-get-not-after.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [sourcetabletemporary, tabletype, temporary, api-page, persistence, regression]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not remove SourceTableTemporary or TableType = Temporary without understanding the impact
|
||||
|
||||
## Description
|
||||
|
||||
`SourceTableTemporary = true` on a page, and `TableType = Temporary` on a table, mean the underlying record operates in memory — Insert/Modify/Delete mutate the session buffer, not the database. Removing either property converts the same operations to real SQL writes. On an API page that external callers hit at high frequency, on a background task that processes thousands of records, or on a UI page that composes an in-memory list for display, the change from temporary to persistent can turn a lightweight operation into a major source of database load. The refactor is easy to propose ("why is this temporary?") and expensive to regret.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a diff removes `SourceTableTemporary = true` or `TableType = Temporary`, require justification explaining why persistence is now required and what paths still write. Review the callers for unexpected new writes, transaction scope, trigger fires, and contention. Keep the property unless the change genuinely needs persistence; an unused-looking temporary table on a bounded page is usually there for a reason.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A cleanup PR that deletes `SourceTableTemporary = true` from an API page "because the source table already exists". The API now writes to the real table on every call, every consumer's requests reach the database, and the incidental side-effects in the source table's triggers start firing across tenants.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [maintainsqlindex, sift, sumindexfields, flowfield, calcsums, key]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# MaintainSQLIndex = false on a key disables SIFT for the FlowFields that depend on it
|
||||
|
||||
## Description
|
||||
|
||||
SIFT relies on the underlying SQL index being maintained by the platform. Setting `MaintainSQLIndex = false` on a key drops the SQL index without dropping the AL key declaration — the key compiles, FlowFields that reference its SumIndexFields compile, and CalcSums calls against matching filters compile. At runtime, however, the SIFT optimization silently cannot engage, and every aggregate falls back to a table scan. The symptom is a FlowField whose read time degrades linearly with row count, with no code-level signal pointing at the key property as the cause.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Keep `MaintainSQLIndex = true` (the default) on any key whose SumIndexFields back a FlowField or that callers use with CalcSums. When a key is genuinely unused and the SQL index cost is the concern, remove the key entirely rather than leaving it in place with `MaintainSQLIndex = false`. If the FlowField is still needed, pick a different key that is maintained.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A source-table key declared with `SumIndexFields` and `MaintainSQLIndex = false`, with a FlowField referencing those sum fields. The FlowField appears to work in development against small datasets and becomes a full table scan on production-scale data, with no error message and no obvious culprit in the code under review.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [setloadfields, heuristics, narrow-table, short-loop, diminishing-returns]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# SetLoadFields pays off at scale; skip it on narrow tables and short loops
|
||||
|
||||
## Description
|
||||
|
||||
`SetLoadFields` reduces the number of columns the platform hydrates per record. It delivers real savings on wide tables with blob, media, or many text fields when the iteration touches a small subset. Below certain thresholds the accounting flips the other way: narrow tables (fewer than ~10 fields) save almost nothing per row, and short loops (fewer than ~10 iterations) amortize the narrowing over too few fetches to outweigh the extra code and the specification-and-access-set coupling that future edits have to maintain. Recommending SetLoadFields on every Find/Get call produces low-value churn and invites the opposite mistake — listing a field in SetLoadFields and then forgetting to access it, which triggers a second round-trip to load the missing field.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Reach for SetLoadFields when the table is wide (10+ fields, especially with blobs) AND the code path reads a small subset AND the iteration or fetch count is material. When in doubt on a short loop over a narrow table, leave SetLoadFields out; the complexity cost is not earned. The filter-only-field rule from `omit-filter-only-fields-from-setloadfields` still applies: fields used only in filters stay out of the list.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A 5-row loop over a 6-field setup table prefaced by `Rec.SetLoadFields(...)`. The author has added two lines of code, coupled the loop to a field specification that needs to be updated on every schema change, and saved nanoseconds. The same pattern applied mechanically to every Find call in a codebase produces hundreds of diffs that do not move the performance needle.
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 51205 "Perf Sample LockTable Bad"
|
||||
{
|
||||
procedure GetOrCreate(var AgentStatus: Record "Integer"): Boolean
|
||||
begin
|
||||
// Every caller takes an exclusive lock, even the ones that only read.
|
||||
// Under load the helper becomes the dominant contention point.
|
||||
AgentStatus.LockTable();
|
||||
if not AgentStatus.Get(1) then begin
|
||||
AgentStatus.Number := 1;
|
||||
AgentStatus.Insert();
|
||||
end;
|
||||
exit(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,17 @@
|
|||
codeunit 51204 "Perf Sample LockTable Good"
|
||||
{
|
||||
procedure GetOrCreate(var AgentStatus: Record "Integer"): Boolean
|
||||
begin
|
||||
// Read path: no lock.
|
||||
if AgentStatus.Get(1) then
|
||||
exit(true);
|
||||
|
||||
// Write path: lock only when we are about to insert.
|
||||
AgentStatus.LockTable();
|
||||
if not AgentStatus.Get(1) then begin
|
||||
AgentStatus.Number := 1;
|
||||
AgentStatus.Insert();
|
||||
end;
|
||||
exit(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [locktable, read-only, write-path, contention, helper]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Split read-only and write paths so LockTable runs only when needed
|
||||
|
||||
## Description
|
||||
|
||||
LockTable takes an exclusive write lock on the affected table for the remainder of the transaction. In a helper that is called from many read-only sites and a few write sites, placing LockTable unconditionally at the top serializes every reader on every other reader's lock — the helper becomes a system-wide contention point. The correct shape is a conditional structure: try the read-only path first, and only fall through to LockTable when the code genuinely needs to modify the table.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Factor the helper so readers return immediately without a lock and only writers reach the LockTable call. A common pattern: attempt `Rec.Get()` first; if it returns the row, exit with the value; otherwise LockTable and proceed with the Insert. Document the pattern in a comment on the helper so callers understand why the LockTable is inside a branch.
|
||||
|
||||
See sample: `split-read-only-and-write-paths-to-avoid-locktable.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `GetOrCreate` helper that unconditionally calls `Rec.LockTable()` at the top, then Gets the row, then returns it. Every reader now blocks every other reader even though none of them intend to write. Under load the helper becomes the dominant bottleneck.
|
||||
|
||||
See sample: `split-read-only-and-write-paths-to-avoid-locktable.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [ledger-entry, production-scale, hot-table, item-ledger, gl-entry, sales-invoice-line]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Treat ledger-entry and line-type tables as production-scale when reviewing performance
|
||||
|
||||
## Description
|
||||
|
||||
A handful of Business Central tables grow to millions of rows in production tenants: Item Ledger Entry, Value Entry, G/L Entry, VAT Entry, Customer Ledger Entry, Vendor Ledger Entry, Sales Invoice Line, Purchase Invoice Line, Detailed Cust. Ledg. Entry, Detailed Vendor Ledg. Entry, and equivalent line-type tables. Master-data tables like Customer, Vendor, and Item typically reach the high hundreds of thousands. A performance review that treats these tables with the same latitude as setup tables or small reference lists under-reports real regressions; the same filter-or-key mistake that is invisible on a 50-row table is a full table scan over millions of rows on these.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a code change touches any of the above tables, demand concrete performance reasoning before accepting it: an appropriate key selection, a SetLoadFields narrowing, filters that use the key prefix, no N+1 inside the iteration. A finding on one of these tables should almost never be downgraded from High to Low on the grounds that "the operation looks small" — at production scale the operation is never small.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Applying review heuristics uniformly to all tables. A missing SetCurrentKey on a Setup table changes nothing; the same mistake on Item Ledger Entry turns a list page into a multi-second load. The asymmetry is the whole point of the catalog — knowing which tables warrant the stricter read.
|
||||
Loading…
Add table
Add a link
Reference in a new issue