mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Merge remote-tracking branch 'origin/main' into fix-setloadfields-order-myth
# Conflicts: # community/knowledge/performance/call-setloadfields-before-filters.md Co-authored-by: JeremyVyska <35526546+JeremyVyska@users.noreply.github.com>
This commit is contained in:
commit
ee73a0c98d
474 changed files with 7604 additions and 1969 deletions
15
.github/scripts/validate_frontmatter.py
vendored
15
.github/scripts/validate_frontmatter.py
vendored
|
|
@ -145,10 +145,19 @@ def is_non_empty_list_of_str(value: Any) -> bool:
|
||||||
return isinstance(value, list) and len(value) > 0 and all(isinstance(v, str) and v for v in value)
|
return isinstance(value, list) and len(value) > 0 and all(isinstance(v, str) and v for v in value)
|
||||||
|
|
||||||
|
|
||||||
def expand_bc_version(value: Any) -> tuple[list[int] | None, str | None]:
|
def expand_bc_version(value: Any) -> tuple[list[int] | str | None, str | None]:
|
||||||
"""Return (expanded-list, error-message). One of the two is None."""
|
"""Return (expanded, error-message). One of the two is None.
|
||||||
|
|
||||||
|
For the universal sentinel ["all"], `expanded` is the string "all".
|
||||||
|
Otherwise it is the expanded list of version integers.
|
||||||
|
"""
|
||||||
if not isinstance(value, list) or not value:
|
if not isinstance(value, list) or not value:
|
||||||
return None, "must be a non-empty list"
|
return None, "must be a non-empty list"
|
||||||
|
# Case 0: universal sentinel
|
||||||
|
if len(value) == 1 and value[0] == "all":
|
||||||
|
return "all", None
|
||||||
|
if "all" in value:
|
||||||
|
return None, "'all' is mutually exclusive with explicit versions"
|
||||||
# Case 1: all integers
|
# Case 1: all integers
|
||||||
if all(isinstance(v, int) and not isinstance(v, bool) for v in value):
|
if all(isinstance(v, int) and not isinstance(v, bool) for v in value):
|
||||||
if any(v <= 0 for v in value):
|
if any(v <= 0 for v in value):
|
||||||
|
|
@ -162,7 +171,7 @@ def expand_bc_version(value: Any) -> tuple[list[int] | None, str | None]:
|
||||||
if start > end:
|
if start > end:
|
||||||
return None, f"range '{value[0]}' is not ascending"
|
return None, f"range '{value[0]}' is not ascending"
|
||||||
return list(range(start, end + 1)), None
|
return list(range(start, end + 1)), None
|
||||||
return None, "must be a list of integers or a single-element range shorthand like [26..28]"
|
return None, "must be [all], a list of integers, or a single-element range shorthand like [26..28]"
|
||||||
|
|
||||||
|
|
||||||
def headings_in_order(body: str) -> list[tuple[str, int]]:
|
def headings_in_order(body: str) -> list[tuple[str, int]]:
|
||||||
|
|
|
||||||
26
README.md
26
README.md
|
|
@ -1,9 +1,29 @@
|
||||||
|
# ⚠️ Warning
|
||||||
|
This project is under active development.
|
||||||
|
Large and potentially breaking changes are expected.
|
||||||
|
|
||||||
|
**Public preview will soon be announced.**
|
||||||
|
|
||||||
# BCQuality
|
# BCQuality
|
||||||
|
|
||||||
Quality skills and knowledge for Business Central development.
|
Quality skills and knowledge for Business Central development.
|
||||||
|
|
||||||
BCQuality is a curated knowledge base and skills library for Business Central. It provides structured, machine-readable guidance that development agents and tools can consume — establishing a consistent quality bar across tooling and teams.
|
BCQuality is a curated knowledge base and skills library for Business Central. It provides structured, machine-readable guidance that development agents and tools can consume — establishing a consistent quality bar across tooling and teams.
|
||||||
|
|
||||||
|
## What belongs here
|
||||||
|
|
||||||
|
BCQuality is a remedial knowledge base. A file exists because a capable LLM **would get something wrong, or miss something, without it** — not because the topic is important. The admission test for a knowledge file is one question:
|
||||||
|
|
||||||
|
> If this file did not exist, would a modern LLM reviewing or generating BC code make a mistake this file would have prevented?
|
||||||
|
|
||||||
|
If the answer is no — the advice is generic software-engineering guidance, or the LLM already knows the BC mechanic in question — the file does not belong here, regardless of how sound the content is. A file earns its place by encoding something BC-specific that LLMs demonstrably get wrong: a CodeCop rule number, a platform API whose semantics the training data gets backwards, a non-obvious ordering rule, a BC property whose default is a footgun.
|
||||||
|
|
||||||
|
Good fit: "`SetLoadFields` must be called before filters, not after" (non-obvious ordering rule). "`FindSet(true)` takes a LockTable and the two-parameter signature is obsolete" (subtle platform behaviour + outdated training data). "CodeCop AA0233 flags `FindFirst … Next` loops" (rule-specific).
|
||||||
|
|
||||||
|
Poor fit: "Use HTTPS instead of HTTP." "Don't hardcode secrets." "Keep transactions short." These are true but any capable LLM already applies them without prompting.
|
||||||
|
|
||||||
|
The practical consequence: when a code-review agent flags something it shouldn't have, or misses something it should have caught, the remedy is a new knowledge file. When it already behaves correctly on a topic, no file is needed.
|
||||||
|
|
||||||
## What's in this repo
|
## What's in this repo
|
||||||
|
|
||||||
BCQuality contains **knowledge** and **skills**. It does not contain agents. Agents that consume BCQuality ship with [AL-Go](https://github.com/microsoft/AL-Go) and other orchestrators.
|
BCQuality contains **knowledge** and **skills**. It does not contain agents. Agents that consume BCQuality ship with [AL-Go](https://github.com/microsoft/AL-Go) and other orchestrators.
|
||||||
|
|
@ -38,7 +58,7 @@ Skills define how agents consume knowledge. They come in three flavors:
|
||||||
|
|
||||||
READ and DO are read on demand — typically when the first dispatched action skill runs. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content.
|
READ and DO are read on demand — typically when the first dispatched action skill runs. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content.
|
||||||
|
|
||||||
- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/al-code-review.md`](microsoft/skills/al-code-review.md) (super-skill), which composes [`microsoft/skills/al-performance-review.md`](microsoft/skills/al-performance-review.md) and [`microsoft/skills/al-security-review.md`](microsoft/skills/al-security-review.md) (leaves).
|
- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes six leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI).
|
||||||
|
|
||||||
### Agent bootstrapping
|
### Agent bootstrapping
|
||||||
|
|
||||||
|
|
@ -52,7 +72,7 @@ Every knowledge file is a markdown file with mandatory YAML frontmatter. Files t
|
||||||
|
|
||||||
```yaml
|
```yaml
|
||||||
---
|
---
|
||||||
bc-version: [26..28] # BC versions this applies to
|
bc-version: [all] # or [26..28] for version-gated guidance
|
||||||
domain: performance # security | performance | ux | telemetry | ...
|
domain: performance # security | performance | ux | telemetry | ...
|
||||||
keywords: [query, filtering, partial] # free-text tags for retrieval
|
keywords: [query, filtering, partial] # free-text tags for retrieval
|
||||||
technologies: [al] # al | javascript | powershell | ...
|
technologies: [al] # al | javascript | powershell | ...
|
||||||
|
|
@ -98,6 +118,8 @@ Action skills follow a four-step pattern:
|
||||||
|
|
||||||
Every action skill produces output in a common format that orchestrators can consume without skill-specific parsing. The format is JSON and includes an `outcome` (so a clean run, a not-applicable skill, and a partial failure are all distinguishable), `findings` (what the skill observed), structured `references` back to the knowledge files that informed each finding, per-finding `confidence`, and a `suppressed` list recording any knowledge files overridden by layer precedence. This contract is defined in the Action Skill meta-skill so that orchestrators and action skills remain independently evolvable.
|
Every action skill produces output in a common format that orchestrators can consume without skill-specific parsing. The format is JSON and includes an `outcome` (so a clean run, a not-applicable skill, and a partial failure are all distinguishable), `findings` (what the skill observed), structured `references` back to the knowledge files that informed each finding, per-finding `confidence`, and a `suppressed` list recording any knowledge files overridden by layer precedence. This contract is defined in the Action Skill meta-skill so that orchestrators and action skills remain independently evolvable.
|
||||||
|
|
||||||
|
BCQuality is an **additive** knowledge layer: it augments the agent's review judgement, it does not replace it. Super-skills (such as `al-code-review`) run a self-review pass alongside their sub-skills and surface concerns the agent identified on its own, marked with `from-sub-skill: "agent"` and an empty `references: []` so consumers can render them distinctly from knowledge-backed findings. See [agent-consumption.md](agent-consumption.md) and [`skills/do.md`](skills/do.md) for the full contract.
|
||||||
|
|
||||||
The meta-skills in `/skills/` define this pattern. Every concrete action skill follows it.
|
The meta-skills in `/skills/` define this pattern. Every concrete action skill follows it.
|
||||||
|
|
||||||
For the end-to-end flow — from orchestrator trigger through to how output reaches developers — see [agent-consumption.md](agent-consumption.md).
|
For the end-to-end flow — from orchestrator trigger through to how output reaches developers — see [agent-consumption.md](agent-consumption.md).
|
||||||
|
|
|
||||||
14
SECURITY.md
Normal file
14
SECURITY.md
Normal file
|
|
@ -0,0 +1,14 @@
|
||||||
|
<!-- BEGIN MICROSOFT SECURITY.MD V1.0.0 BLOCK -->
|
||||||
|
|
||||||
|
## Security
|
||||||
|
|
||||||
|
Microsoft takes the security of our software products and services seriously, which
|
||||||
|
includes all source code repositories in our GitHub organizations.
|
||||||
|
|
||||||
|
**Please do not report security vulnerabilities through public GitHub issues.**
|
||||||
|
|
||||||
|
For security reporting information, locations, contact information, and policies,
|
||||||
|
please review the latest guidance for Microsoft repositories at
|
||||||
|
[https://aka.ms/SECURITY.md](https://aka.ms/SECURITY.md).
|
||||||
|
|
||||||
|
<!-- END MICROSOFT SECURITY.MD BLOCK -->
|
||||||
|
|
@ -35,7 +35,7 @@ The agent reads `/skills/entry.md` and runs it against the task context. Entry a
|
||||||
The dispatch record names one or more action skills and the subset of inputs each should receive. If the outcome is `no-match` or `failed`, the agent returns the record to the orchestrator unchanged.
|
The dispatch record names one or more action skills and the subset of inputs each should receive. If the outcome is `no-match` or `failed`, the agent returns the record to the orchestrator unchanged.
|
||||||
|
|
||||||
### 4. Agent invokes each dispatched action skill
|
### 4. Agent invokes each dispatched action skill
|
||||||
Action skills live inside the layers — `/microsoft/skills/`, `/community/skills/`, `/custom/skills/` — so their authority is carried by their location. For a PR review, Entry typically dispatches `microsoft/skills/al-code-review.md`. The agent reads the file and executes it.
|
Action skills live inside the layers — `/microsoft/skills/`, `/community/skills/`, `/custom/skills/` — so their authority is carried by their location. For a PR review, Entry typically dispatches `microsoft/skills/review/al-code-review.md`. The agent reads the file and executes it.
|
||||||
|
|
||||||
### 5. Action skill executes the four-step pattern
|
### 5. Action skill executes the four-step pattern
|
||||||
|
|
||||||
|
|
@ -66,6 +66,17 @@ The orchestrator parses this **without skill-specific logic**. This is the point
|
||||||
### 7. Orchestrator integrates
|
### 7. Orchestrator integrates
|
||||||
The orchestrator turns findings into PR comments, build gates, or IDE diagnostics, and links the references back to the knowledge files so the PR author — human or agent — can read the guidance.
|
The orchestrator turns findings into PR comments, build gates, or IDE diagnostics, and links the references back to the knowledge files so the PR author — human or agent — can read the guidance.
|
||||||
|
|
||||||
|
## Knowledge-backed and agent findings
|
||||||
|
|
||||||
|
BCQuality is an **additive** knowledge layer. The agent surfaces two kinds of findings, both shaped to the same DO output contract:
|
||||||
|
|
||||||
|
- **Knowledge-backed findings** carry one or more entries in `references[]` pointing at BCQuality knowledge files. Their `id` is the primary file's repo-relative path. These are produced by leaf sub-skills and rolled up by super-skills.
|
||||||
|
- **Agent findings** are surfaced by a super-skill from its own self-review pass when no BCQuality knowledge file backs the concern. They are tagged with `from-sub-skill: "agent"`, carry an empty `references: []`, use a slug `id` prefixed `agent:`, and have `confidence` capped at `medium`. Their `message` is self-contained because there is no knowledge-file footer to fall back on.
|
||||||
|
|
||||||
|
Before a super-skill emits an agent finding, it validates the candidate against the BCQuality knowledge already loaded for the task: a matching file upgrades the candidate to a knowledge-backed finding (and merges or deduplicates against the relevant sub-skill output); a contradicting file suppresses the candidate. Only candidates with no BCQuality coverage become agent findings.
|
||||||
|
|
||||||
|
Orchestrators MAY render the two kinds differently — for example, by labelling agent findings or routing them to a separate review domain — and MAY apply independent severity floors. The `from-sub-skill: "agent"` marker is the contract.
|
||||||
|
|
||||||
## Why this architecture
|
## Why this architecture
|
||||||
|
|
||||||
- **Entry is the only hardcoded thing.** Orchestrators ship with one convention — *"invoke `/skills/entry.md` first"* — and nothing else. New action skills and new knowledge files are picked up automatically because Entry discovers them at dispatch time.
|
- **Entry is the only hardcoded thing.** Orchestrators ship with one convention — *"invoke `/skills/entry.md` first"* — and nothing else. New action skills and new knowledge files are picked up automatically because Entry discovers them at dispatch time.
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [singleinstance, subscriber, event, memory, session]
|
keywords: [singleinstance, subscriber, event, memory, session]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Avoid growing globals in SingleInstance subscribers
|
# Avoid growing globals in SingleInstance subscribers
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [maintainsiftindex, sift, calcsums, flowfield, write-cost]
|
keywords: [maintainsiftindex, sift, calcsums, flowfield, write-cost]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Choose MaintainSIFTIndex by read-write ratio
|
# Choose MaintainSIFTIndex by read-write ratio
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [setloadfields, case, conditional, branch, field-loading]
|
keywords: [setloadfields, case, conditional, branch, field-loading]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Load common fields before branching on case
|
# Load common fields before branching on case
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [setloadfields, primary-key, reference, existence-check, memory]
|
keywords: [setloadfields, primary-key, reference, existence-check, memory]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Load only primary key fields for reference work
|
# Load only primary key fields for reference work
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [setloadfields, filter, field-exclusion, index]
|
keywords: [setloadfields, filter, field-exclusion, index]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Omit filter-only fields from SetLoadFields
|
# Omit filter-only fields from SetLoadFields
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [case, branch, frequency, control-flow, hot-path]
|
keywords: [case, branch, frequency, control-flow, hot-path]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Order case branches by frequency
|
# Order case branches by frequency
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass]
|
keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Use DeleteAll for filtered bulk deletion
|
# Use DeleteAll for filtered bulk deletion
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [dataclassification, gdpr, privacy, euii, compliance]
|
keywords: [dataclassification, gdpr, privacy, euii, compliance]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,20 +9,18 @@ application-area: [all]
|
||||||
|
|
||||||
# Classify every field with DataClassification
|
# Classify every field with DataClassification
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
Every field on every AL table and table extension must carry an explicit `DataClassification` property. The value drives GDPR tooling, data-subject requests, retention policies, and audit reporting — all of which rely on the field metadata to know what data to include, anonymize, or delete. A field with no `DataClassification` defaults to `ToBeClassified`, which is a compliance gap, not a neutral state.
|
Every field on every AL table and table extension must have a resolved `DataClassification` value, either declared directly on the field or inherited from a table-level default. The value drives GDPR tooling, data-subject requests, retention policies, and audit reporting — all of which rely on the field metadata to know what data to include, anonymize, or delete. A field with no field-level property and no table-level default resolves to `ToBeClassified`, which is a compliance gap, not a neutral state.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
Choose the narrowest value that accurately describes the field's content: `EndUserIdentifiableInformation` for data that directly identifies a person, `EndUserPseudonymousIdentifiers` for indirect identifiers, `CustomerContent` for business operational data, `SystemMetadata` for system-generated housekeeping, `AccountData` for tenant/billing, `OrganizationIdentifiableInformation` for organization-level identifiers. When uncertain between two values, pick the stronger protection.
|
Choose the narrowest value that accurately describes the field's content: `EndUserIdentifiableInformation` for data that directly identifies a person, `EndUserPseudonymousIdentifiers` for indirect identifiers, `CustomerContent` for business operational data, `SystemMetadata` for system-generated housekeeping, `AccountData` for tenant/billing, `OrganizationIdentifiableInformation` for organization-level identifiers. Use a table-level default for homogeneous tables, and override individual fields whose content differs from that default. When uncertain between two values, pick the stronger protection.
|
||||||
|
|
||||||
See sample: `classify-every-field-with-dataclassification.good.al`.
|
See sample: `classify-every-field-with-dataclassification.good.al`.
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
Leaving `DataClassification = ToBeClassified` on a field, or omitting the property entirely (which resolves to the same default). Code in this state fails compliance audits and breaks the subject-access-request and retention tooling that depends on the property being set correctly.
|
Leaving `DataClassification = ToBeClassified` on a field, omitting classification when the table has no default, or relying on a table-level default that understates a field's actual content. Code in this state fails compliance audits and breaks the subject-access-request and retention tooling that depends on the property being set correctly.
|
||||||
|
|
||||||
See sample: `classify-every-field-with-dataclassification.bad.al`.
|
See sample: `classify-every-field-with-dataclassification.bad.al`.
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [permissionset, includedpermissionsets, assignable, composition, role]
|
keywords: [permissionset, includedpermissionsets, assignable, composition, role]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Compose permission sets with IncludedPermissionSets
|
# Compose permission sets with IncludedPermissionSets
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [entitlement, permissionset, license, clipping, sandbox-drift]
|
keywords: [entitlement, permissionset, license, clipping, sandbox-drift]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Do not grant rights beyond a user's entitlement
|
# Do not grant rights beyond a user's entitlement
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [istemporary, deleteall, modifyall, safeguard, precondition]
|
keywords: [istemporary, deleteall, modifyall, safeguard, precondition]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Guard bulk operations with IsTemporary
|
# Guard bulk operations with IsTemporary
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [oauth2, api-key, authentication, httpclient, token-refresh]
|
keywords: [oauth2, api-key, authentication, httpclient, token-refresh]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Prefer OAuth2 over API keys for external HTTP calls
|
# Prefer OAuth2 over API keys for external HTTP calls
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: security
|
domain: security
|
||||||
keywords: [temporary-table, data-protection, permission, cleanup]
|
keywords: [temporary-table, data-protection, permission, cleanup]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
|
|
@ -9,7 +9,7 @@ application-area: [all]
|
||||||
|
|
||||||
# Protect sensitive data in temporary tables
|
# Protect sensitive data in temporary tables
|
||||||
|
|
||||||
> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,10 +0,0 @@
|
||||||
tableextension 50118 "Perf Sample SIFTKey" extends "Cust. Ledger Entry"
|
|
||||||
{
|
|
||||||
keys
|
|
||||||
{
|
|
||||||
key(PerfSampleOpenByCustomer; "Customer No.", Open, "Posting Date")
|
|
||||||
{
|
|
||||||
SumIndexFields = "Remaining Amt. (LCY)";
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
@ -1,27 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [sift, sumindexfields, flowfield, key, aa0232]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Add SIFT keys for FlowField aggregations
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
CodeCop rule AA0232 checks that FlowFields backed by CalcSums or aggregation CalcFormula are supported by a key whose SumIndexFields include the summed field and whose key prefix matches the formula's filter fields. Without a SIFT key the platform falls back to a full aggregation on every read — typically invisible in development and catastrophic in production.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
For each Sum-style FlowField, ensure the source table has a key whose leading fields match the FlowField's CalcFormula WHERE clause and whose SumIndexFields list includes the summed field. Table extensions adding new FlowFields are responsible for adding the supporting key.
|
|
||||||
|
|
||||||
See sample: `add-sift-keys-for-flowfields.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Declaring a FlowField on a hot table without checking whether a supporting SIFT key exists ships a latent scan into every list page and report that touches the field.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,14 @@
|
||||||
|
report 50221 "Perf Sample AddLoadFields Bad"
|
||||||
|
{
|
||||||
|
dataset
|
||||||
|
{
|
||||||
|
// No AddLoadFields: every Cust. Ledger Entry column ships per row, even though
|
||||||
|
// only three columns feed the layout.
|
||||||
|
dataitem(CustLedgerEntry; "Cust. Ledger Entry")
|
||||||
|
{
|
||||||
|
column(CustomerNo; "Customer No.") { }
|
||||||
|
column(PostingDate; "Posting Date") { }
|
||||||
|
column(Amount; Amount) { }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -1,8 +1,8 @@
|
||||||
report 50112 "Perf Sample AddLoadFields Good"
|
report 50220 "Perf Sample AddLoadFields Good"
|
||||||
{
|
{
|
||||||
dataset
|
dataset
|
||||||
{
|
{
|
||||||
dataitem(Cust; "Cust. Ledger Entry")
|
dataitem(CustLedgerEntry; "Cust. Ledger Entry")
|
||||||
{
|
{
|
||||||
column(CustomerNo; "Customer No.") { }
|
column(CustomerNo; "Customer No.") { }
|
||||||
column(PostingDate; "Posting Date") { }
|
column(PostingDate; "Posting Date") { }
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [report, addloadfields, onpredataitem, dataitem, partial-record, layout]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# In reports, declare the fields the layout needs with AddLoadFields
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Reports iterate dataitems on potentially large source tables and pipe rows into a layout. The partial-record optimization is the same idea as `use-setloadfields-for-partial-records.md`, but the API is different: per the upstream guidance, "for reports, use `AddLoadFields()` in `OnPreDataItem` trigger to add fields needed by the layout." `AddLoadFields` is additive — call it for each field the layout consumes — and runs once per dataitem before iteration begins.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
In each dataitem's `OnPreDataItem` trigger, list the columns the layout binds to via `AddLoadFields(<field>, <field>, ...)`. The platform then materializes only those columns per row. Treat the layout column list as the spec: every column the layout uses must be added; columns the layout does not use should not be added.
|
||||||
|
|
||||||
|
See sample: `addloadfields-in-report-onpredataitem.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Relying on the dataitem's default to load every field. On a report bound to a ledger-scale table this transfers an entire row per iteration, of which the layout reads a fraction.
|
||||||
|
|
||||||
|
See sample: `addloadfields-in-report-onpredataitem.bad.al`.
|
||||||
|
|
@ -0,0 +1,22 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [admin-page, migration, wizard, hybrid, permissions, lower-severity]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Admin and migration pages tolerate lower performance discipline
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Some pages run rarely and against small datasets, and the upstream guidance explicitly calls for treating them as lower severity. Per the review checklist, "Admin/migration pages (`Admin`, `Setup`, `Wizard`, `Migration`, `HybridBC14`, `HybridSL`, `HybridGP` namespaces, `Permissions`/`PermissionSet` pages) are infrequently used with small datasets — apply lower severity." The same logic covers one-time wizards and tenant-bootstrap routines: the code path runs a handful of times in the lifetime of a tenant, against a bounded dataset, by an administrator.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When triaging a finding on an admin, migration, or wizard page, downgrade severity relative to the same finding on a hot business path. A `FindSet` loop without `SetLoadFields` on a migration page that processes setup records once per tenant is a different finding than the same loop on a posting routine that runs thousands of times a day. Note this context explicitly in the review so the call site is not "fixed" twice with diminishing returns.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Treating a migration wizard's per-row loop with the same urgency as the same loop in `Sales-Post`. The fix cost is the same; the production benefit is not. Bulk-rewriting an admin page to use `ModifyAll` and partial records buys nothing the user will perceive.
|
||||||
|
|
@ -1,7 +1,10 @@
|
||||||
codeunit 50101 "Perf Sample FilterBeforeFind Bad"
|
codeunit 50229 "Perf Sample FilterEarly Bad"
|
||||||
{
|
{
|
||||||
procedure ProcessUsCustomers(var Customer: Record Customer)
|
procedure ProcessUSCustomers()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
begin
|
begin
|
||||||
|
// Reads every customer in the table, discards the non-US ones in AL.
|
||||||
if Customer.FindSet() then
|
if Customer.FindSet() then
|
||||||
repeat
|
repeat
|
||||||
if Customer."Country/Region Code" = 'US' then
|
if Customer."Country/Region Code" = 'US' then
|
||||||
|
|
@ -11,6 +14,5 @@ codeunit 50101 "Perf Sample FilterBeforeFind Bad"
|
||||||
|
|
||||||
local procedure ProcessCustomer(var Customer: Record Customer)
|
local procedure ProcessCustomer(var Customer: Record Customer)
|
||||||
begin
|
begin
|
||||||
// per-customer work
|
|
||||||
end;
|
end;
|
||||||
}
|
}
|
||||||
|
|
@ -1,6 +1,8 @@
|
||||||
codeunit 50100 "Perf Sample FilterBeforeFind Good"
|
codeunit 50228 "Perf Sample FilterEarly Good"
|
||||||
{
|
{
|
||||||
procedure ProcessUsCustomers(var Customer: Record Customer)
|
procedure ProcessUSCustomers()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
begin
|
begin
|
||||||
Customer.SetRange("Country/Region Code", 'US');
|
Customer.SetRange("Country/Region Code", 'US');
|
||||||
if Customer.FindSet() then
|
if Customer.FindSet() then
|
||||||
|
|
@ -11,6 +13,5 @@ codeunit 50100 "Perf Sample FilterBeforeFind Good"
|
||||||
|
|
||||||
local procedure ProcessCustomer(var Customer: Record Customer)
|
local procedure ProcessCustomer(var Customer: Record Customer)
|
||||||
begin
|
begin
|
||||||
// per-customer work
|
|
||||||
end;
|
end;
|
||||||
}
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [setrange, setfilter, filter, loop, early, dataset]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Apply SetRange/SetFilter before iterating, not as an if-test inside the loop
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A `SetRange` or `SetFilter` placed before `FindSet` narrows the result set at the database. The same condition expressed as an `if` inside the loop body filters in AL, after every row has crossed the boundary. Per the upstream guidance, "apply `SetRange`/`SetFilter` as early as possible to reduce dataset" and "more specific filters = better performance." On a production-scale table the difference is the difference between scanning a subset and scanning the whole table.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Move every predicate that can be expressed as an equality or range filter into a `SetRange` or `SetFilter` ahead of the find. Combine with `SetCurrentKey` to choose a key whose first fields match the filter (see `setcurrentkey-aligns-key-with-filters.md`). The loop body should then contain only the work that depends on per-row state.
|
||||||
|
|
||||||
|
See sample: `apply-filters-before-iterating.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`if Customer.FindSet() then repeat if Customer."Country/Region Code" = 'US' then ProcessCustomer(Customer); until Customer.Next() = 0;` — the loop pays for every row in the table and discards the non-matching ones in AL. The intent is the same as a `SetRange("Country/Region Code", 'US')` ahead of the find, but the cost is not.
|
||||||
|
|
||||||
|
See sample: `apply-filters-before-iterating.bad.al`.
|
||||||
|
|
@ -0,0 +1,14 @@
|
||||||
|
codeunit 50215 "Perf Sample GuardBeforeGet Bad"
|
||||||
|
{
|
||||||
|
procedure ResolveAllocation(var PurchaseLine: Record "Purchase Line")
|
||||||
|
var
|
||||||
|
PurchaseHeader: Record "Purchase Header";
|
||||||
|
begin
|
||||||
|
// Wasted lookup when the line has no allocation account: the procedure
|
||||||
|
// exits below, but the header was already fetched.
|
||||||
|
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");
|
||||||
|
if PurchaseLine."Selected Alloc. Account No." = '' then
|
||||||
|
exit;
|
||||||
|
// ...
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,12 @@
|
||||||
|
codeunit 50214 "Perf Sample GuardBeforeGet Good"
|
||||||
|
{
|
||||||
|
procedure ResolveAllocation(var PurchaseLine: Record "Purchase Line")
|
||||||
|
var
|
||||||
|
PurchaseHeader: Record "Purchase Header";
|
||||||
|
begin
|
||||||
|
if PurchaseLine."Selected Alloc. Account No." = '' then
|
||||||
|
exit;
|
||||||
|
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");
|
||||||
|
// ...
|
||||||
|
end;
|
||||||
|
}
|
||||||
26
microsoft/knowledge/performance/apply-guards-before-get.md
Normal file
26
microsoft/knowledge/performance/apply-guards-before-get.md
Normal file
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [get, guard, early-exit, conditional, lookup, wasted-query]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Apply early-exit guards before calling Get
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A `Get` (or any other database call) executed before a guard that may exit the procedure does a round-trip the procedure never uses. Per the upstream guidance, "Flag `Get()` calls that execute before a guard condition that may exit early — the DB lookup is wasted." The fix is structural: order the procedure body so cheap checks (parameter validation, in-memory field comparisons, enum tests) run first, and the database call runs only after the guards pass.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Read the procedure top-to-bottom and place every condition that can short-circuit ahead of every database call. The check `if SomeNo = '' then exit;` belongs above `Header.Get(...)`, not below. Each guard moved upward saves one wasted query on the path that exits.
|
||||||
|
|
||||||
|
See sample: `apply-guards-before-get.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`Record.Get(...)` at the top of a procedure followed by `if SomeField = '' then exit;`. The code reads top-down as "load the record, then decide whether we needed it" — exactly the order that wastes the query. The pattern is easy to introduce when guards are added later, defensively, without re-checking call ordering.
|
||||||
|
|
||||||
|
See sample: `apply-guards-before-get.bad.al`.
|
||||||
|
|
@ -1,18 +0,0 @@
|
||||||
codeunit 50117 "Perf Sample CalcFieldsInLoop Bad"
|
|
||||||
{
|
|
||||||
procedure ProcessLargeLines(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line")
|
|
||||||
begin
|
|
||||||
SalesLine.SetRange("Document Type", SalesHeader."Document Type");
|
|
||||||
SalesLine.SetRange("Document No.", SalesHeader."No.");
|
|
||||||
if SalesLine.FindSet() then
|
|
||||||
repeat
|
|
||||||
SalesHeader.CalcFields(Amount);
|
|
||||||
if SalesHeader.Amount > 1000 then
|
|
||||||
ProcessLine(SalesLine);
|
|
||||||
until SalesLine.Next() = 0;
|
|
||||||
end;
|
|
||||||
|
|
||||||
local procedure ProcessLine(var SalesLine: Record "Sales Line")
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,18 +0,0 @@
|
||||||
codeunit 50116 "Perf Sample CalcFieldsInLoop Good"
|
|
||||||
{
|
|
||||||
procedure ProcessLargeLines(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line")
|
|
||||||
begin
|
|
||||||
SalesHeader.CalcFields(Amount);
|
|
||||||
SalesLine.SetRange("Document Type", SalesHeader."Document Type");
|
|
||||||
SalesLine.SetRange("Document No.", SalesHeader."No.");
|
|
||||||
if SalesLine.FindSet() then
|
|
||||||
repeat
|
|
||||||
if SalesHeader.Amount > 1000 then
|
|
||||||
ProcessLine(SalesLine);
|
|
||||||
until SalesLine.Next() = 0;
|
|
||||||
end;
|
|
||||||
|
|
||||||
local procedure ProcessLine(var SalesLine: Record "Sales Line")
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [calcfields, flowfield, loop, n-plus-one]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Do not call CalcFields inside loops
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
CalcFields evaluates one or more FlowFields for the current record by issuing a separate SQL aggregation. Called inside a loop over a record set, it becomes an N+1 problem: one aggregate per row. For any non-trivial set on a ledger-entry-backed FlowField this is orders of magnitude slower than the equivalent batched query.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Move CalcFields out of the iteration. If the total is what you need, use CalcSums on the filtered parent set. If row-by-row FlowField values are needed, reshape the computation so the aggregate runs once — for example by joining against a temporary table populated in a single batched query.
|
|
||||||
|
|
||||||
See sample: `avoid-calcfields-in-loops.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Calling CalcFields inside `repeat ... until Next() = 0` on a hot parent record is the textbook N+1 pattern. Even a modest parent set size (hundreds of rows) turns into thousands of round-trips.
|
|
||||||
|
|
||||||
See sample: `avoid-calcfields-in-loops.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -1,15 +1,14 @@
|
||||||
codeunit 50129 "Perf Sample CommitInLoop Bad"
|
codeunit 50129 "Perf Sample CommitInLoop Bad"
|
||||||
{
|
{
|
||||||
procedure ReleaseAllOrders()
|
procedure NormalizeCustomerNames()
|
||||||
var
|
var
|
||||||
SalesHeader: Record "Sales Header";
|
Customer: Record Customer;
|
||||||
begin
|
begin
|
||||||
SalesHeader.SetRange(Status, SalesHeader.Status::Open);
|
if Customer.FindSet(true) then
|
||||||
if SalesHeader.FindSet() then
|
|
||||||
repeat
|
repeat
|
||||||
SalesHeader.Status := SalesHeader.Status::Released;
|
Customer.Name := UpperCase(Customer.Name);
|
||||||
SalesHeader.Modify();
|
Customer.Modify();
|
||||||
Commit();
|
Commit();
|
||||||
until SalesHeader.Next() = 0;
|
until Customer.Next() = 0;
|
||||||
end;
|
end;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,21 @@
|
||||||
|
codeunit 50128 "Perf Sample CommitInLoop Good"
|
||||||
|
{
|
||||||
|
procedure NormalizeCustomerNames()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
RowsInChunk: Integer;
|
||||||
|
ChunkSize: Integer;
|
||||||
|
begin
|
||||||
|
ChunkSize := 500;
|
||||||
|
if Customer.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
Customer.Name := UpperCase(Customer.Name);
|
||||||
|
Customer.Modify();
|
||||||
|
RowsInChunk += 1;
|
||||||
|
if RowsInChunk >= ChunkSize then begin
|
||||||
|
Commit();
|
||||||
|
RowsInChunk := 0;
|
||||||
|
end;
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -1,7 +1,7 @@
|
||||||
---
|
---
|
||||||
bc-version: [26..28]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [commit, loop, transaction, lock]
|
keywords: [commit, loop, transaction, lock, checkpoint, codeunit-run]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
countries: [w1]
|
countries: [w1]
|
||||||
application-area: [all]
|
application-area: [all]
|
||||||
|
|
@ -9,15 +9,17 @@ application-area: [all]
|
||||||
|
|
||||||
# Do not Commit inside loops
|
# Do not Commit inside loops
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
> Contributions welcome — open a PR to refine or extend this article.
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
Commit ends the current transaction. Calling it inside a loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically. It also interferes with the platform's ability to batch write operations. The original motivation — releasing locks during a long batch — is better served by splitting the batch into explicit checkpoints that each process a bounded number of rows.
|
Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that each process N rows.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
If the batch is large enough that a single transaction is untenable, process it in checkpoints driven by an outer loop that each time picks up the next N rows. Commit once per checkpoint at a clearly defined safe boundary, not inside the per-row loop.
|
If the batch is large enough that a single transaction is untenable, process it in checkpoints driven by an outer loop that each time picks up the next N rows. Commit once per checkpoint at a clearly defined safe boundary, not inside the per-row loop. Wrapping each chunk in `Codeunit.Run` gives the same effect with native rollback on failure — see `codeunit-run-as-atomic-sub-operation.md`.
|
||||||
|
|
||||||
|
See sample: `avoid-commit-inside-loops.good.al`.
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,14 +0,0 @@
|
||||||
codeunit 50105 "Perf Sample AvoidFindFirstNext Bad"
|
|
||||||
{
|
|
||||||
procedure EmitAllItems(var Item: Record Item)
|
|
||||||
begin
|
|
||||||
if Item.FindFirst() then
|
|
||||||
repeat
|
|
||||||
EmitItem(Item);
|
|
||||||
until Item.Next() = 0;
|
|
||||||
end;
|
|
||||||
|
|
||||||
local procedure EmitItem(var Item: Record Item)
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,27 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [findfirst, findlast, get, next, aa0233]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Do not pair FindFirst, FindLast, or Get with Next
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
CodeCop rule AA0233 flags loops that start with FindFirst, FindLast, or Get and then call Next. FindFirst and FindLast retrieve a single row and reposition the cursor; calling Next after them forces the platform to re-seek and stream the rest of the set, which is slower than the correct FindSet pattern and signals intent incorrectly to reviewers and the optimizer.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Choose the Find variant that matches the operation: FindSet for full iteration, FindFirst or FindLast when you want exactly one row, Get when the primary key is known. Never call Next after FindFirst, FindLast, or Get.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Writing `if Rec.FindFirst() then repeat ... until Rec.Next() = 0` is the canonical AA0233 offender. The loop wastes bandwidth and obscures the author's intent.
|
|
||||||
|
|
||||||
See sample: `avoid-findfirst-with-next.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,15 @@
|
||||||
|
codeunit 50253 "Perf Sample NPlus1 Bad"
|
||||||
|
{
|
||||||
|
procedure SumStdCost(var BOMLine: Record "BOM Component") TotalCost: Decimal
|
||||||
|
var
|
||||||
|
Item: Record Item;
|
||||||
|
begin
|
||||||
|
if BOMLine.FindSet() then
|
||||||
|
repeat
|
||||||
|
// Full-row Item.Get per BOM line — no partial loading, no caching.
|
||||||
|
Item.Get(BOMLine."No.");
|
||||||
|
if Item."Costing Method" = Item."Costing Method"::Standard then
|
||||||
|
TotalCost += Item."Standard Cost" * BOMLine."Quantity per";
|
||||||
|
until BOMLine.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,15 @@
|
||||||
|
codeunit 50252 "Perf Sample NPlus1 Good"
|
||||||
|
{
|
||||||
|
procedure SumStdCost(var BOMLine: Record "BOM Component") TotalCost: Decimal
|
||||||
|
var
|
||||||
|
Item: Record Item;
|
||||||
|
begin
|
||||||
|
Item.SetLoadFields("Costing Method", "Standard Cost");
|
||||||
|
if BOMLine.FindSet() then
|
||||||
|
repeat
|
||||||
|
if Item.Get(BOMLine."No.") then
|
||||||
|
if Item."Costing Method" = Item."Costing Method"::Standard then
|
||||||
|
TotalCost += Item."Standard Cost" * BOMLine."Quantity per";
|
||||||
|
until BOMLine.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [n-plus-one, get, findfirst, loop, inner-lookup, large-table]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Avoid Get / FindFirst inside a loop on a large inner table
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A `Get` or `FindFirst` against a different record inside a loop body produces one database round-trip per iteration — the classic N+1 pattern. Per the upstream guidance, "Flag when a `Get()`/`FindFirst()` is called inside a loop for each record — this creates N+1 database round-trips." The cost only matters when the inner table is meaningful: lookups against temporary tables, singleton setup tables, enum-mapping tables, permission objects, or Role IDs are bounded and safe. The pattern to catch is the inner lookup that hits a production-scale table for every outer row.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When the loop needs values from another record, lift the lookup out of the loop if the rows can be collected up front, or apply `SetLoadFields` so each inner read transfers only the columns the loop actually uses (see `use-setloadfields-for-partial-records.md`). When the inner record is small or bounded, leave the call site alone — the rule targets large-table inner lookups specifically.
|
||||||
|
|
||||||
|
See sample: `avoid-get-inside-loop-on-large-table.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Iterating BOM lines and calling `Item.Get(BOMLine."No.")` per row to read a costing method, with no `SetLoadFields` on `Item`. Each iteration issues one query against Item (~800k rows) and pulls the entire row to read two fields. The fix is `Item.SetLoadFields("Costing Method", "Standard Cost");` ahead of the loop — still N reads, but each one transfers only the needed columns.
|
||||||
|
|
||||||
|
See sample: `avoid-get-inside-loop-on-large-table.bad.al`.
|
||||||
|
|
@ -0,0 +1,22 @@
|
||||||
|
codeunit 50255 "Perf Sample RecRef Bad"
|
||||||
|
{
|
||||||
|
procedure ProcessAllCustomerNames()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
RecRef: RecordRef;
|
||||||
|
FldRef: FieldRef;
|
||||||
|
begin
|
||||||
|
RecRef.Open(Database::Customer);
|
||||||
|
if RecRef.FindSet() then
|
||||||
|
repeat
|
||||||
|
// Table and field are fixed at compile time, but every iteration
|
||||||
|
// pays dynamic resolution cost.
|
||||||
|
FldRef := RecRef.Field(Customer.FieldNo(Name));
|
||||||
|
ProcessName(Format(FldRef.Value));
|
||||||
|
until RecRef.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure ProcessName(Name: Text)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,16 @@
|
||||||
|
codeunit 50254 "Perf Sample RecRef Good"
|
||||||
|
{
|
||||||
|
procedure ProcessAllCustomerNames()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
ProcessName(Customer.Name);
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure ProcessName(Name: Text[100])
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [recordref, fieldref, hot-loop, typed-record, metadata]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Avoid RecordRef / FieldRef in hot loops when a typed record fits
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`RecordRef` and `FieldRef` are slower than direct typed record access — the platform resolves the table and field at runtime instead of at compile time. The trade-off is intentional: per the upstream guidance, "RecordRef/FieldRef operations are slower than direct record access, but many features REQUIRE them for generic metadata iteration (permission checks, field copying, dynamic field access)." The rule, then, is not "never use them" but "only flag when used inside a clearly unbounded hot loop (10k+ iterations) where a typed alternative exists."
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Use `RecordRef`/`FieldRef` for genuinely generic code — permission checks, field copying, table-agnostic export. When the loop target is known at compile time and the loop iterates a large number of rows, declare the typed record and access fields directly; the saved per-iteration overhead is measurable at the volumes the rule targets.
|
||||||
|
|
||||||
|
See sample: `avoid-recordref-in-hot-loop.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`RecRef.Open(Database::Customer); if RecRef.FindSet() then repeat FldRef := RecRef.Field(Customer.FieldNo(Name)); ProcessName(FldRef.Value); until RecRef.Next() = 0;` — the table is fixed at compile time, the field is fixed at compile time, and the loop pays the dynamic-resolution cost on every iteration. The direct `Customer.Name` form does the same work without the lookup.
|
||||||
|
|
||||||
|
See sample: `avoid-recordref-in-hot-loop.bad.al`.
|
||||||
|
|
@ -0,0 +1,21 @@
|
||||||
|
page 50217 "Perf Sample Redundant Bad"
|
||||||
|
{
|
||||||
|
PageType = ListPart;
|
||||||
|
SourceTable = "Assembly Line";
|
||||||
|
|
||||||
|
var
|
||||||
|
AssemblyLineRec: Record "Assembly Line";
|
||||||
|
ShowWarning: Boolean;
|
||||||
|
|
||||||
|
trigger OnAfterGetRecord()
|
||||||
|
begin
|
||||||
|
// Redundant: the platform already fetched the row into Rec.
|
||||||
|
AssemblyLineRec.Get("Document Type", "Document No.", "Line No.");
|
||||||
|
ShowWarning := CheckAvailability(AssemblyLineRec);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure CheckAvailability(var AssemblyLine: Record "Assembly Line"): Boolean
|
||||||
|
begin
|
||||||
|
exit(AssemblyLine.Quantity > 0);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
page 50216 "Perf Sample Redundant Good"
|
||||||
|
{
|
||||||
|
PageType = ListPart;
|
||||||
|
SourceTable = "Assembly Line";
|
||||||
|
|
||||||
|
var
|
||||||
|
ShowWarning: Boolean;
|
||||||
|
|
||||||
|
trigger OnAfterGetRecord()
|
||||||
|
begin
|
||||||
|
ShowWarning := CheckAvailability(Rec);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure CheckAvailability(var AssemblyLine: Record "Assembly Line"): Boolean
|
||||||
|
begin
|
||||||
|
exit(AssemblyLine.Quantity > 0);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [get, onaftergetrecord, redundant, page-trigger, rec, already-loaded]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Do not Get the record the page already loaded
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A list or card page's `OnAfterGetRecord` trigger fires *because* the platform has already fetched a row into `Rec`. Calling `Get` for that same row inside the trigger repeats the read the platform just did. Per the upstream guidance, this is "redundant — record already fetched by page runtime"; the correction is "use `Rec` directly — already loaded." The waste compounds on list pages, where the trigger runs once per row displayed.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Inside page triggers — `OnAfterGetRecord`, `OnAfterGetCurrRecord`, validation triggers — read from `Rec` (or the trigger's record parameter). The platform exposes the freshly loaded record there for exactly this purpose. Reach for `Get` only when the trigger needs a *different* record than the one being displayed.
|
||||||
|
|
||||||
|
See sample: `avoid-redundant-get-when-record-already-loaded.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`AssemblyLineRec.Get("Document Type", "Document No.", "Line No.");` at the top of `OnAfterGetRecord`, when the trigger is on the `Assembly Line` page itself and `Rec` already holds that row. The pattern often appears when a helper that expects a record parameter is invoked from a page trigger and the author writes a `Get` to "freshen" `Rec` rather than passing `Rec` through.
|
||||||
|
|
||||||
|
See sample: `avoid-redundant-get-when-record-already-loaded.bad.al`.
|
||||||
|
|
@ -1,11 +0,0 @@
|
||||||
codeunit 50127 "Perf Sample UserInTxn Bad"
|
|
||||||
{
|
|
||||||
procedure ArchiveSalesHeader(var SalesHeader: Record "Sales Header")
|
|
||||||
begin
|
|
||||||
SalesHeader.Status := SalesHeader.Status::Released;
|
|
||||||
SalesHeader.Modify();
|
|
||||||
if not Confirm('Archive document %1?', false, SalesHeader."No.") then
|
|
||||||
exit;
|
|
||||||
SalesHeader.Delete(true);
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,14 +0,0 @@
|
||||||
codeunit 50126 "Perf Sample UserInTxn Good"
|
|
||||||
{
|
|
||||||
procedure ArchiveSalesHeader(var SalesHeader: Record "Sales Header")
|
|
||||||
begin
|
|
||||||
if not Confirm('Archive document %1?', false, SalesHeader."No.") then
|
|
||||||
exit;
|
|
||||||
DoArchive(SalesHeader);
|
|
||||||
end;
|
|
||||||
|
|
||||||
local procedure DoArchive(var SalesHeader: Record "Sales Header")
|
|
||||||
begin
|
|
||||||
// only Insert/Modify/Delete calls happen here; no prompts
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [confirm, strmenu, message, transaction, dialog]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Do not prompt the user inside a write transaction
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
Confirm, StrMenu, Message, and any other user-facing dialog pauses execution while the transaction is still open. During that pause every lock held by the transaction blocks other sessions. A user who walks away from the screen can suspend business-critical tables for an unbounded period.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Gather every user decision before the writing phase begins. Once the decisions are known, run the transaction end-to-end without prompts.
|
|
||||||
|
|
||||||
See sample: `avoid-user-interaction-in-transactions.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Calling Confirm or StrMenu from inside an OnInsert, OnModify, or OnDelete trigger — or from any code path that has already started modifying records — blocks on user input while holding locks.
|
|
||||||
|
|
||||||
See sample: `avoid-user-interaction-in-transactions.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
codeunit 50239 "Perf Sample PromptInTxn Bad"
|
||||||
|
{
|
||||||
|
procedure PostOrder(DocNo: Code[20])
|
||||||
|
var
|
||||||
|
SalesHeader: Record "Sales Header";
|
||||||
|
PostConfirmQst: Label 'Post this order?';
|
||||||
|
begin
|
||||||
|
SalesHeader.LockTable();
|
||||||
|
SalesHeader.Get(SalesHeader."Document Type"::Order, DocNo);
|
||||||
|
// Lock held while the dialog is on screen — minutes or hours.
|
||||||
|
if Confirm(PostConfirmQst) then
|
||||||
|
PostSalesOrder(SalesHeader);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure PostSalesOrder(var SalesHeader: Record "Sales Header")
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
codeunit 50238 "Perf Sample PromptInTxn Good"
|
||||||
|
{
|
||||||
|
procedure PostOrder(DocNo: Code[20])
|
||||||
|
var
|
||||||
|
SalesHeader: Record "Sales Header";
|
||||||
|
PostConfirmQst: Label 'Post this order?';
|
||||||
|
begin
|
||||||
|
if not Confirm(PostConfirmQst) then
|
||||||
|
exit;
|
||||||
|
SalesHeader.LockTable();
|
||||||
|
SalesHeader.Get(SalesHeader."Document Type"::Order, DocNo);
|
||||||
|
PostSalesOrder(SalesHeader);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure PostSalesOrder(var SalesHeader: Record "Sales Header")
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [confirm, strmenu, dialog, transaction, lock, user-interaction]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Do not hold locks while waiting for the user
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A `Confirm`, `StrMenu`, modal page, or other user prompt issued from inside a write transaction stalls the transaction — and therefore every lock it holds — until the user responds. Per the upstream guidance, "Avoid user interactions (Confirm, StrMenu) inside transactions — they hold locks while waiting for user input." The wait is bounded only by the user; meanwhile other sessions block on whatever this transaction has acquired.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Sequence the operation so user confirmation happens *before* any database write that takes a lock the prompt holds open. The shape is: ask the user → if confirmed, acquire locks and post. `if Confirm(...) then begin SalesHeader.LockTable(); SalesHeader.Get(DocNo); PostSalesOrder(SalesHeader); end;` keeps the lock window down to the work itself.
|
||||||
|
|
||||||
|
See sample: `avoid-user-prompts-inside-transactions.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`SalesHeader.LockTable(); SalesHeader.Get(DocNo); if Confirm('Post this order?') then ...;` — the lock is held for as long as the dialog is up. A user who steps away to lunch holds the lock for an hour, and every other session that touches that row blocks for the duration.
|
||||||
|
|
||||||
|
See sample: `avoid-user-prompts-inside-transactions.bad.al`.
|
||||||
|
|
@ -0,0 +1,15 @@
|
||||||
|
codeunit 50223 "Perf Sample CalcSums Bad"
|
||||||
|
{
|
||||||
|
procedure TotalRemaining(CustomerNo: Code[20]) Total: Decimal
|
||||||
|
var
|
||||||
|
CustLedgerEntry: Record "Cust. Ledger Entry";
|
||||||
|
begin
|
||||||
|
CustLedgerEntry.SetRange("Customer No.", CustomerNo);
|
||||||
|
// One SQL query per row over a 10M-row ledger.
|
||||||
|
if CustLedgerEntry.FindSet() then
|
||||||
|
repeat
|
||||||
|
CustLedgerEntry.CalcFields("Remaining Amount");
|
||||||
|
Total += CustLedgerEntry."Remaining Amount";
|
||||||
|
until CustLedgerEntry.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,11 @@
|
||||||
|
codeunit 50222 "Perf Sample CalcSums Good"
|
||||||
|
{
|
||||||
|
procedure TotalRemaining(CustomerNo: Code[20]) Total: Decimal
|
||||||
|
var
|
||||||
|
CustLedgerEntry: Record "Cust. Ledger Entry";
|
||||||
|
begin
|
||||||
|
CustLedgerEntry.SetRange("Customer No.", CustomerNo);
|
||||||
|
CustLedgerEntry.CalcSums("Remaining Amount");
|
||||||
|
Total := CustLedgerEntry."Remaining Amount";
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [calcfields, calcsums, loop, flowfield, n-plus-one, aggregation]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Use CalcSums to aggregate, not CalcFields inside a loop
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`CalcFields` materializes FlowField values for one record. Each call against a persistent table is "a separate SQL query"; running it inside a `repeat ... until Next() = 0` over a large table issues one query per row on top of the iteration itself. `CalcSums` answers the same aggregation question — "give me the sum of this FlowField over the filtered set" — as a single SQL statement. Per the upstream guidance, `CalcFields` inside loops on large persistent tables is "a performance problem"; the aggregation form is `CalcSums()`.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When the procedure totals a FlowField (or several) across a filtered set, set the filters, then call `CalcSums("Field 1", "Field 2", ...)`. The platform issues one query; the result is read off the record's FlowField slot. Single `CalcFields` outside loops is fine, and `CalcFields` on the current row in a page's `OnAfterGetRecord` or in `OnValidate` is the standard pattern — those are per-action, not per-row over a large set.
|
||||||
|
|
||||||
|
See sample: `calcsums-instead-of-calcfields-in-loop.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`if CustLedgerEntry.FindSet() then repeat CustLedgerEntry.CalcFields("Remaining Amount"); Total += CustLedgerEntry."Remaining Amount"; until CustLedgerEntry.Next() = 0;` — exactly the upstream-flagged shape. The iteration is the cheap part; the per-row `CalcFields` is what scales linearly with table size.
|
||||||
|
|
||||||
|
See sample: `calcsums-instead-of-calcfields-in-loop.bad.al`.
|
||||||
|
|
@ -0,0 +1,12 @@
|
||||||
|
codeunit 50144 "Perf Sample AtomicSub Bad"
|
||||||
|
{
|
||||||
|
procedure ApplyDiscountToSelection(var Customer: Record Customer)
|
||||||
|
begin
|
||||||
|
if Customer.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
Customer."Customer Price Group" := 'VIP';
|
||||||
|
Customer.Modify(true);
|
||||||
|
Commit();
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,29 @@
|
||||||
|
codeunit 50142 "Perf Sample AtomicSub Good"
|
||||||
|
{
|
||||||
|
procedure ApplyDiscountToSelection(var Customer: Record Customer)
|
||||||
|
var
|
||||||
|
ApplyOne: Codeunit "Perf Sample Apply Discount";
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
ClearLastError();
|
||||||
|
if not ApplyOne.Run(Customer) then
|
||||||
|
LogSkipped(Customer."No.", GetLastErrorText());
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure LogSkipped(CustomerNo: Code[20]; ErrorText: Text)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
||||||
|
codeunit 50143 "Perf Sample Apply Discount"
|
||||||
|
{
|
||||||
|
TableNo = Customer;
|
||||||
|
|
||||||
|
trigger OnRun()
|
||||||
|
begin
|
||||||
|
Rec.Validate("Customer Price Group", 'VIP');
|
||||||
|
Rec.Modify(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [codeunit-run, atomic, rollback, transaction, sub-transaction, try-pattern, implicit-commit]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Use Codeunit.Run to bound an atomic sub-operation
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`Codeunit.Run(ID)` is the AL-idiomatic way to run a unit of work as an atomic sub-operation with its own transactional boundary. When the return value is captured — `if Codeunit.Run(MyCodeunit) then ...` — the runtime treats the codeunit as a unit: on successful completion it performs an implicit commit of the codeunit's database changes; on error it rolls those changes back and the caller receives `false`. Per the platform reference, "any changes done to the database will be committed at the end of the codeunit, unless an error occurs." The caller decides how to react — compensate, surface an error, continue — without having to manage transactions by hand.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When a piece of work must either complete fully or have no effect, put it in its own codeunit and invoke it via `Codeunit.Run`, capturing the return. Use `if not Codeunit.Run(X) then Error(...)` to abort and unwind; use the plain boolean branch to react to failure without aborting the caller. This replaces the SQL-style `BEGIN TRAN / COMMIT / ROLLBACK` habit with a pattern the AL runtime implements natively. Do not confuse `Codeunit.Run` with `[TryFunction]` — both catch errors, but only `Codeunit.Run` rolls back database changes on failure (see `use-tryfunction-for-error-catching-not-rollback.md`). Note that if the caller is already in a write transaction, the platform requires a `Commit()` before `Codeunit.Run` — the sub-operation cannot nest inside an open transaction (see `codeunit-run-requires-prior-commit-inside-transaction.md`).
|
||||||
|
|
||||||
|
See sample: `codeunit-run-as-atomic-sub-operation.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Inlining the work in the caller and sprinkling `Commit()` to simulate sub-transaction boundaries. The caller's enclosing transaction is fused to the sub-work; any Commit between checkpoints survives subsequent errors, and any errors after a Commit cannot be cleanly unwound. Per-row Commits (see `avoid-commit-inside-loops.md`) are a frequent symptom.
|
||||||
|
|
||||||
|
See sample: `codeunit-run-as-atomic-sub-operation.bad.al`.
|
||||||
|
|
@ -0,0 +1,27 @@
|
||||||
|
codeunit 50147 "Perf Sample OpenTxnRun Bad"
|
||||||
|
{
|
||||||
|
procedure ApplyDiscountToSelection(var Customer: Record Customer)
|
||||||
|
var
|
||||||
|
ApplyOne: Codeunit "Perf Sample OpenTxnRun Apply";
|
||||||
|
RunLog: Record "Custom Run Log";
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
RunLog.Init();
|
||||||
|
RunLog."Customer No." := Customer."No.";
|
||||||
|
RunLog.Insert();
|
||||||
|
if not ApplyOne.Run(Customer) then;
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
||||||
|
codeunit 50148 "Perf Sample OpenTxnRun Apply"
|
||||||
|
{
|
||||||
|
TableNo = Customer;
|
||||||
|
|
||||||
|
trigger OnRun()
|
||||||
|
begin
|
||||||
|
Rec.Validate("Customer Price Group", 'VIP');
|
||||||
|
Rec.Modify(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,37 @@
|
||||||
|
codeunit 50145 "Perf Sample DeferredLog Good"
|
||||||
|
{
|
||||||
|
procedure ApplyDiscountToSelection(var Customer: Record Customer)
|
||||||
|
var
|
||||||
|
ApplyOne: Codeunit "Perf Sample DeferredLog Apply";
|
||||||
|
FailedCustomerNos: List of [Code[20]];
|
||||||
|
FailureReasons: List of [Text];
|
||||||
|
Index: Integer;
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
ClearLastError();
|
||||||
|
if not ApplyOne.Run(Customer) then begin
|
||||||
|
FailedCustomerNos.Add(Customer."No.");
|
||||||
|
FailureReasons.Add(GetLastErrorText());
|
||||||
|
end;
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
|
||||||
|
for Index := 1 to FailedCustomerNos.Count() do
|
||||||
|
WriteFailureLog(FailedCustomerNos.Get(Index), FailureReasons.Get(Index));
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure WriteFailureLog(CustomerNo: Code[20]; Reason: Text)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
||||||
|
codeunit 50146 "Perf Sample DeferredLog Apply"
|
||||||
|
{
|
||||||
|
TableNo = Customer;
|
||||||
|
|
||||||
|
trigger OnRun()
|
||||||
|
begin
|
||||||
|
Rec.Validate("Customer Price Group", 'VIP');
|
||||||
|
Rec.Modify(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [codeunit-run, commit, write-transaction, nesting, loop, runtime-error]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Commit before Codeunit.Run when the caller already holds a write transaction
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`Codeunit.Run` cannot nest inside an open write transaction. Per the platform reference, "If you're already in a transaction you must commit first before calling `Codeunit.Run`." The platform enforces this at runtime: the first call dies with an error, not at compile time. The rule most often surfaces in a loop that pairs outer-scope writes — progress records, audit log entries, failure markers — with a per-item `Codeunit.Run`: the first outer write opens a transaction, the subsequent `Codeunit.Run` throws. `[CommitBehavior]` does not silence this, because the implicit commit inside `Codeunit.Run` is exempt from the attribute: "The `CommitBehavior` only applies to explicit commits, not implicit commits done as part of [Codeunit.Run]." `[TryFunction]` is not a substitute either: a try method catches errors but does not open its own rollback boundary (see `use-tryfunction-for-error-catching-not-rollback.md`).
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
For the `Codeunit.Run` atomic-sub-operation pattern (see `codeunit-run-as-atomic-sub-operation.md`) to work in a loop, keep the outer scope **read-only**. Move per-iteration writes — progress updates, logging, audit entries — into the sub-codeunit so they commit or roll back together with the per-item work. If logging must live outside the atomic boundary, defer it: collect failure info in memory during the loop (a `List of [Text]`, a temporary record, local variables) and write it in one pass after the loop ends, when no outer write transaction is open.
|
||||||
|
|
||||||
|
See sample: `codeunit-run-requires-prior-commit-inside-transaction.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Inserting `Commit()` before each `Codeunit.Run` to silence the runtime error. The error goes away, but the outer scope now commits per iteration — the behavior `avoid-commit-inside-loops.md` exists to warn against. Attempting to silence the implicit commit inside the sub-codeunit with `[CommitBehavior(CommitBehavior::Ignore)]` also fails: the attribute does not apply to `Codeunit.Run`'s implicit commit. Conditioning the Commit on `Database.IsInWriteTransaction()` (runtime 11.0+) is another version of the same trap — the method has legitimate uses for diagnostics and library code that genuinely cannot control its caller, but branching production flow on runtime transaction state typically signals unclear ownership that would be better fixed by restructuring the caller so transaction state is predictable.
|
||||||
|
|
||||||
|
See sample: `codeunit-run-requires-prior-commit-inside-transaction.bad.al`.
|
||||||
|
|
@ -0,0 +1,10 @@
|
||||||
|
codeunit 50235 "Perf Sample LockReadOnly Bad"
|
||||||
|
{
|
||||||
|
procedure GetStatus(var AgentStatus: Record "Agent Status"): Boolean
|
||||||
|
begin
|
||||||
|
// Read-only path, yet every caller's transaction now acquires UPDLOCK
|
||||||
|
// on Agent Status for the remainder of the transaction.
|
||||||
|
AgentStatus.LockTable();
|
||||||
|
exit(AgentStatus.Get());
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,14 @@
|
||||||
|
codeunit 50234 "Perf Sample LockReadOnly Good"
|
||||||
|
{
|
||||||
|
procedure GetStatus(var AgentStatus: Record "Agent Status"): Boolean
|
||||||
|
begin
|
||||||
|
if AgentStatus.Get() then
|
||||||
|
exit(true);
|
||||||
|
AgentStatus.LockTable();
|
||||||
|
if not AgentStatus.Get() then begin
|
||||||
|
AgentStatus.Init();
|
||||||
|
AgentStatus.Insert();
|
||||||
|
end;
|
||||||
|
exit(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [locktable, read-only, helper, contention, transaction]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Do not LockTable in a read-only procedure
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`LockTable` is a transaction-wide signal: from the call onward, every read against that table in the same transaction acquires `UPDLOCK`. Per the upstream guidance, "`LockTable()` before Modify/Insert/Delete in the same procedure is the correct pattern" — locking the read against the write that follows is what the call exists for. The anti-pattern is "`LockTable()` in read-only procedures — unnecessary lock contention": the procedure never writes, but the lock cost is paid by everyone sharing the transaction.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Reserve `LockTable` for the read directly before a `Modify`, `Insert`, or `Delete` that depends on the read value. If a helper is sometimes called for reading and sometimes for writing, split it into separate read and write paths and call `LockTable` only on the write path. For read-only existence checks or lookups, the right primitive is `ReadIsolation` (see `prefer-readisolation-over-locktable-for-reads.md`).
|
||||||
|
|
||||||
|
See sample: `do-not-locktable-in-read-only-procedure.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
A pure getter that opens with `Rec.LockTable();`. Every caller's transaction now acquires `UPDLOCK` on that table for every subsequent read until commit. The contention shows up as blocking on unrelated sessions whose own code path looks innocent — the locker is invisible to the blocked reader.
|
||||||
|
|
||||||
|
See sample: `do-not-locktable-in-read-only-procedure.bad.al`.
|
||||||
|
|
@ -0,0 +1,17 @@
|
||||||
|
page 50247 "Perf Sample WriteScroll Bad"
|
||||||
|
{
|
||||||
|
PageType = List;
|
||||||
|
SourceTable = Customer;
|
||||||
|
|
||||||
|
trigger OnAfterGetRecord()
|
||||||
|
begin
|
||||||
|
// One DB write per row displayed, every time the user scrolls.
|
||||||
|
Rec."Reminder Terms Code" := CalcReminderTerms();
|
||||||
|
Rec.Modify();
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure CalcReminderTerms(): Code[10]
|
||||||
|
begin
|
||||||
|
exit('');
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
page 50246 "Perf Sample WriteScroll Good"
|
||||||
|
{
|
||||||
|
PageType = List;
|
||||||
|
SourceTable = Customer;
|
||||||
|
|
||||||
|
var
|
||||||
|
ShowWarning: Boolean;
|
||||||
|
|
||||||
|
trigger OnAfterGetRecord()
|
||||||
|
begin
|
||||||
|
ShowWarning := CalcWarning();
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure CalcWarning(): Boolean
|
||||||
|
begin
|
||||||
|
exit(false);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [page-trigger, onaftergetrecord, modify, display, scroll, db-write]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Do not Modify inside OnAfterGetRecord
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A list page's `OnAfterGetRecord` fires once per visible row, every time the user scrolls, sorts, or refreshes. A `Modify` inside that trigger means a database write per row displayed. Per the upstream guidance, "`Modify()` here means a DB write on every scroll. Use page variables for display-only state instead." `OnAfterGetCurrRecord` (single record on selection), `OnOpenPage`, and `OnInit` fire once or at much lower frequency and tolerate one-time setup logic.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When the trigger needs to compute display-only state per row, write the result into a page variable (a global on the page object) rather than back to the database. Reserve `Modify` for triggers that fire on an explicit user action — `OnAction`, validation triggers, `OnQueryClosePage` — where one action maps to one write.
|
||||||
|
|
||||||
|
See sample: `do-not-modify-in-onaftergetrecord.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`trigger OnAfterGetRecord() begin Rec."Warning Flag" := CalcWarning(); Rec.Modify(); end;` — on a list page over a moderately sized table, scrolling through fifty rows produces fifty writes. The page feels slow, the table accumulates churn, and the warning flag — which is recomputed on every refresh anyway — never needed persistence.
|
||||||
|
|
||||||
|
See sample: `do-not-modify-in-onaftergetrecord.bad.al`.
|
||||||
|
|
@ -0,0 +1,12 @@
|
||||||
|
page 50249 "Perf Sample TempAPI Bad"
|
||||||
|
{
|
||||||
|
PageType = API;
|
||||||
|
APIPublisher = 'perf';
|
||||||
|
APIGroup = 'sample';
|
||||||
|
APIVersion = 'v1.0';
|
||||||
|
EntityName = 'outboxEmail';
|
||||||
|
EntitySetName = 'outboxEmails';
|
||||||
|
SourceTable = "Sent Email";
|
||||||
|
// SourceTableTemporary removed — every request now hits SQL.
|
||||||
|
DelayedInsert = true;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,12 @@
|
||||||
|
page 50248 "Perf Sample TempAPI Good"
|
||||||
|
{
|
||||||
|
PageType = API;
|
||||||
|
APIPublisher = 'perf';
|
||||||
|
APIGroup = 'sample';
|
||||||
|
APIVersion = 'v1.0';
|
||||||
|
EntityName = 'outboxEmail';
|
||||||
|
EntitySetName = 'outboxEmails';
|
||||||
|
SourceTable = "Sent Email";
|
||||||
|
SourceTableTemporary = true;
|
||||||
|
DelayedInsert = true;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [sourcetabletemporary, api-page, temporary, persistent, in-memory]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Removing SourceTableTemporary on an API page switches it from in-memory to persistent
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`SourceTableTemporary = true` on a page makes the page's record buffer in-memory only — reads and writes do not touch SQL. The same applies to `TableType = Temporary` on a record. Removing either turns operations that were memory accesses into database round-trips. Per the upstream guidance, the change is "potentially increasing DB load for high-volume paths (API pages, background tasks)" — and on API pages especially, the change is invisible at the page definition but visible at production scale.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
If a page or record was declared temporary on purpose — to buffer payloads, accept synthetic rows, or expose computed data through an API surface without persisting it — keep it temporary. When removing the property looks necessary, audit the call sites first: a temporary API page is often consumed by integrations that issue many calls per minute, and the round-trip cost is paid per call. If persistence is genuinely required, weigh storage and lock cost against alternatives (a regular table the API page reads from, an event-driven write).
|
||||||
|
|
||||||
|
See sample: `do-not-remove-sourcetabletemporary-from-api-page.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Dropping `SourceTableTemporary = true` from an API page to "simplify" it, without revisiting the access pattern. The page begins issuing real SQL on every request; locks now contend with other writers; bulk integrations slow proportionally. The same trap exists for a record that was `TableType = Temporary` and gets demoted to a persistent table to make a debugger view easier.
|
||||||
|
|
||||||
|
See sample: `do-not-remove-sourcetabletemporary-from-api-page.bad.al`.
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [filter, setrange, setfilter, findset, scan]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Filter before you find
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
Every call to FindSet, Find, or FindFirst on an unfiltered record variable scans the entire table. On hot tables (ledger entries, value entries, sales invoice lines) a production dataset can easily be millions of rows, so the cost of forgetting a filter is orders of magnitude worse than the cost of applying one.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Apply SetRange or SetFilter to narrow the record set before calling FindSet or Find. The filters should match a key on the table (see set-current-key-to-match-filters). When iterating rows that belong to a parent record, set all key-field filters before the find call — never inside the repeat loop.
|
|
||||||
|
|
||||||
See sample: `filter-before-find.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Calling FindSet with no filters and then discarding rows inside the loop with an if-statement forces the platform to read every row of the table before your code even runs.
|
|
||||||
|
|
||||||
See sample: `filter-before-find.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,25 @@
|
||||||
|
codeunit 50237 "Perf Sample FindSetTrue Bad"
|
||||||
|
{
|
||||||
|
procedure NormalizeNames()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
// Read takes a shared lock; the Modify then needs to upgrade — that gap
|
||||||
|
// is the deadlock window FindSet(true) was designed to close.
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
Customer.Name := UpperCase(Customer.Name);
|
||||||
|
Customer.Modify();
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
procedure ReadOnlyOverlocked()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
// No Modify in the loop, yet every row is read under UpdLock.
|
||||||
|
if Customer.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,23 @@
|
||||||
|
codeunit 50236 "Perf Sample FindSetTrue Good"
|
||||||
|
{
|
||||||
|
procedure NormalizeNames()
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
if Customer.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
Customer.Name := UpperCase(Customer.Name);
|
||||||
|
Customer.Modify();
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
procedure SumBalances() Total: Decimal
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
Total += Customer."Balance (LCY)";
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [findset, updlock, readisolation, locking, modify, obsolete-syntax]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# FindSet(true) applies UpdLock on the read; the two-parameter form is obsolete
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`FindSet()` and `FindSet(false)` are read-only — no locking. Per the upstream guidance, `FindSet(true)` "signifies the intent is to modify records" and "sets `ReadIsolation::UpdLock` on the record before finding rows." That is exactly the right shape when the loop body modifies each row: the read takes the same lock the modification will need, avoiding the deadlock window between an unlocked read and a later upgrade. The older two-parameter form `FindSet(ForUpdate, UpdateKey)` is obsolete — only the single-parameter signature should appear in new code.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Use `FindSet(true)` only when the loop body genuinely modifies the iterated rows; use `FindSet()` (or `FindSet(false)`) when the loop only reads. Do not write `FindSet(true, true)` or `FindSet(true, false)` — the two-parameter form is the obsolete signature.
|
||||||
|
|
||||||
|
See sample: `findset-true-applies-updlock-on-read.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`FindSet(true)` on a loop that does not modify the iterated rows takes an `UpdLock` the work does not need; competing readers and writers stall against a lock the loop never uses. The mirror anti-pattern is `FindSet()` (no parameter) on a loop that *does* modify each row — the read takes a shared lock, the `Modify` then needs to upgrade, and the gap between them is a deadlock candidate.
|
||||||
|
|
||||||
|
See sample: `findset-true-applies-updlock-on-read.bad.al`.
|
||||||
|
|
@ -0,0 +1,15 @@
|
||||||
|
tableextension 50226 "Perf Sample SIFT Bad Cust" extends Customer
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
// No SIFT key on Detailed Cust. Ledg. Entry for (Customer No.) with
|
||||||
|
// "Debit Amount" in SumIndexFields — the sum falls back to row-by-row
|
||||||
|
// aggregation over a ledger-scale table.
|
||||||
|
field(50226; "Perf Sample Total Debit"; Decimal)
|
||||||
|
{
|
||||||
|
FieldClass = FlowField;
|
||||||
|
CalcFormula = sum("Detailed Cust. Ledg. Entry"."Debit Amount"
|
||||||
|
where("Customer No." = field("No.")));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,23 @@
|
||||||
|
tableextension 50224 "Perf Sample SIFT Good Ext" extends "Detailed Cust. Ledg. Entry"
|
||||||
|
{
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PerfSampleByCustomer; "Customer No.", "Posting Date")
|
||||||
|
{
|
||||||
|
SumIndexFields = "Debit Amount";
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
tableextension 50225 "Perf Sample SIFT Good Cust" extends Customer
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(50225; "Perf Sample Total Debit"; Decimal)
|
||||||
|
{
|
||||||
|
FieldClass = FlowField;
|
||||||
|
CalcFormula = sum("Detailed Cust. Ledg. Entry"."Debit Amount"
|
||||||
|
where("Customer No." = field("No.")));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [flowfield, sumindexfields, sift, key, calcformula, aa0232]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# A FlowField needs a source-table key that covers its CalcFormula
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A FlowField is computed by SQL on demand. CodeCop AA0232 — "FlowFields should be indexed with SumIndexFields on corresponding keys" — captures the indexing requirement: the source table must declare a key that includes the fields the `CalcFormula` filters on, with the aggregated field listed in that key's `SumIndexFields`. When that alignment is in place, the platform answers `CalcFields`/`CalcSums` from SIFT; without it, the same query falls back to a row-by-row aggregation on what is often a ledger-scale table. Per the upstream guidance, "Missing SIFT indices cause performance issues on List pages."
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When introducing or changing a FlowField, walk the `CalcFormula`'s `WHERE` clause field by field and verify the source table has a key whose key fields cover those filters, with the aggregated field in `SumIndexFields`. The same applies when the destination side of the FlowField filter is a list-page column: the page filter triggers the FlowField on every visible row, and only SIFT keeps that affordable.
|
||||||
|
|
||||||
|
See sample: `flowfield-source-key-needs-sumindexfields.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
A `sum` FlowField against a large source table with no matching SIFT key. Each calculation aggregates rows directly; on a ledger-sized source the FlowField becomes the slowest column on every page that displays it. Pointing an existing FlowField's `CalcFormula` at a larger source table without verifying the new source's keys is the same trap a step removed — the upstream review guidance flags it as "CalcFormula changed to larger source table".
|
||||||
|
|
||||||
|
See sample: `flowfield-source-key-needs-sumindexfields.bad.al`.
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
codeunit 50257 "Perf Sample EventGuard Bad"
|
||||||
|
{
|
||||||
|
[EventSubscriber(ObjectType::Table, Database::"Sales Line", 'OnAfterValidateEvent', 'Quantity', false, false)]
|
||||||
|
local procedure OnAfterValidateQuantity(var Rec: Record "Sales Line")
|
||||||
|
var
|
||||||
|
Item: Record Item;
|
||||||
|
begin
|
||||||
|
// Item.Get fires on every Quantity edit — including lines whose Type is
|
||||||
|
// not Item. No cheap guard, no SetLoadFields.
|
||||||
|
Item.Get(Rec."No.");
|
||||||
|
if Item."Item Category Code" <> '' then
|
||||||
|
RecalculatePrice(Rec, Item);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure RecalculatePrice(var SalesLine: Record "Sales Line"; var Item: Record Item)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,19 @@
|
||||||
|
codeunit 50256 "Perf Sample EventGuard Good"
|
||||||
|
{
|
||||||
|
[EventSubscriber(ObjectType::Table, Database::"Sales Line", 'OnAfterValidateEvent', 'Quantity', false, false)]
|
||||||
|
local procedure OnAfterValidateQuantity(var Rec: Record "Sales Line")
|
||||||
|
var
|
||||||
|
Item: Record Item;
|
||||||
|
begin
|
||||||
|
if Rec.Type <> Rec.Type::Item then
|
||||||
|
exit;
|
||||||
|
Item.SetLoadFields("Item Category Code");
|
||||||
|
if Item.Get(Rec."No.") then
|
||||||
|
if Item."Item Category Code" <> '' then
|
||||||
|
RecalculatePrice(Rec, Item);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure RecalculatePrice(var SalesLine: Record "Sales Line"; var Item: Record Item)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [event-subscriber, guard, db-call, frequently-fired, validate]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Guard event subscribers with cheap checks before any database call
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Event subscribers fire on every event matching their signature — for `OnAfterValidateEvent` on a hot field like `Sales Line.Quantity`, that is every quantity edit by every user. Per the upstream guidance, "Keep event subscriber code lightweight" and "Avoid database operations in frequently-fired events — guard with cheap checks first." A `Get` or `FindFirst` at the top of such a subscriber pays a database round-trip on every fire, including the calls for which the subscriber's work was not needed.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Open the subscriber with an in-memory predicate that filters out the calls the subscriber does not handle — record type, document type, status, parameter-passed flags. Only after the cheap guard passes should the body issue a database call, and only with `SetLoadFields` for the columns the body actually reads.
|
||||||
|
|
||||||
|
See sample: `guard-event-subscribers-before-db-call.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`[EventSubscriber(...'OnAfterValidateEvent', 'Quantity', ...)] local procedure ... var Item: Record Item; begin Item.Get(Rec."No."); if Item.HasCustomPricing() then ...;` — `Item.Get` runs on every quantity change, including changes to lines whose `Type` is not `Item`. A pre-check `if Rec.Type <> Rec.Type::Item then exit;` ahead of the `Get` removes most of the calls.
|
||||||
|
|
||||||
|
See sample: `guard-event-subscribers-before-db-call.bad.al`.
|
||||||
|
|
@ -1,12 +0,0 @@
|
||||||
codeunit 50140 "Perf Sample Subscriber Bad"
|
|
||||||
{
|
|
||||||
[EventSubscriber(ObjectType::Table, Database::"Sales Line", 'OnAfterValidateEvent', 'No.', false, false)]
|
|
||||||
local procedure HeavyWorkOnSalesLineNo(var Rec: Record "Sales Line"; var xRec: Record "Sales Line")
|
|
||||||
var
|
|
||||||
HttpClient: HttpClient;
|
|
||||||
HttpResponse: HttpResponseMessage;
|
|
||||||
begin
|
|
||||||
// synchronous external call on a hot event
|
|
||||||
HttpClient.Get('https://example.com/validate?no=' + Rec."No.", HttpResponse);
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,27 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [event, subscriber, publisher, extension]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Keep event subscribers lightweight
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
Event subscribers run synchronously on the publisher's thread. If a subscriber does heavy work — a database query, a web service call, a layout render — every caller of the publisher pays that cost. Subscribers on hot events (OnAfterValidate on common fields, OnBeforeInsert on ledger-entry-like tables) can multiply a small per-call cost into a system-wide regression.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Keep subscribers small: guard early with inexpensive checks, defer heavy work to a task queue or a background session, and cache results across invocations when the data is stable.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Calling an external web service, running a report, or iterating a large table from inside an event subscriber on a hot publisher makes every operation on that publisher as slow as the heaviest subscriber.
|
|
||||||
|
|
||||||
See sample: `keep-event-subscribers-lightweight.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -1,18 +0,0 @@
|
||||||
codeunit 50128 "Perf Sample TxnScope Bad"
|
|
||||||
{
|
|
||||||
procedure ImportCustomers(var Source: List of [Text])
|
|
||||||
var
|
|
||||||
Customer: Record Customer;
|
|
||||||
HttpClient: HttpClient;
|
|
||||||
HttpResponse: HttpResponseMessage;
|
|
||||||
Row: Text;
|
|
||||||
begin
|
|
||||||
foreach Row in Source do begin
|
|
||||||
// external call inside the write transaction
|
|
||||||
HttpClient.Get('https://example.com/validate?row=' + Row, HttpResponse);
|
|
||||||
Customer.Init();
|
|
||||||
// ... populate from Row ...
|
|
||||||
Customer.Insert(true);
|
|
||||||
end;
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,22 +0,0 @@
|
||||||
codeunit 50123 "Perf Sample TxnScope Good"
|
|
||||||
{
|
|
||||||
procedure ImportCustomers(var Source: List of [Text])
|
|
||||||
var
|
|
||||||
Prepared: Record Customer temporary;
|
|
||||||
Customer: Record Customer;
|
|
||||||
begin
|
|
||||||
// read, validate, and shape outside the transaction
|
|
||||||
PrepareRows(Source, Prepared);
|
|
||||||
|
|
||||||
// transaction starts here: only Insert/Modify calls
|
|
||||||
if Prepared.FindSet() then
|
|
||||||
repeat
|
|
||||||
Customer := Prepared;
|
|
||||||
Customer.Insert(true);
|
|
||||||
until Prepared.Next() = 0;
|
|
||||||
end;
|
|
||||||
|
|
||||||
local procedure PrepareRows(var Source: List of [Text]; var Prepared: Record Customer temporary)
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [transaction, lock, scope, contention]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Keep transaction scope short
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
Every write operation runs inside a transaction that holds locks until the transaction ends. Long transactions increase blocking, deadlocks, and timeouts for other sessions. The same work split across narrower transactions typically completes faster under load because it holds locks for less time.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Perform data reads, calculations, and external integrations outside the transaction whenever possible. Enter the writing phase with all inputs computed, execute the minimum set of Insert, Modify, and Delete calls, and exit. If you have a long-running batch, split it into checkpoints at safe boundaries (see avoid-commit-inside-loops).
|
|
||||||
|
|
||||||
See sample: `keep-transaction-scope-short.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Opening a transaction, then performing external web-service calls, heavy report runs, or user-facing dialogs while the locks are held, suspends every other session that needs the same rows for as long as the external operation takes.
|
|
||||||
|
|
||||||
See sample: `keep-transaction-scope-short.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
table 50227 "Perf Sample FA Journal Tmpl"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; Name; Code[10]) { }
|
||||||
|
field(40; "No. of Lines"; Integer)
|
||||||
|
{
|
||||||
|
FieldClass = FlowField;
|
||||||
|
// Source key below has MaintainSQLIndex = false: SIFT cannot
|
||||||
|
// function, so this COUNT runs without a SQL index.
|
||||||
|
CalcFormula = count("FA Journal Line"
|
||||||
|
where("Journal Template Name" = field(Name)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
tableextension 50228 "Perf Sample FA Jnl Line Ext" extends "FA Journal Line"
|
||||||
|
{
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PerfSampleByTemplate; "Journal Template Name", "Journal Batch Name")
|
||||||
|
{
|
||||||
|
MaintainSQLIndex = false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,24 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [maintainsqlindex, key, sift, flowfield, sum, count, table-scan]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# MaintainSQLIndex = false on a key disables SIFT for FlowFields that depend on it
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`MaintainSQLIndex = false` on a key tells the platform not to materialize that key as a SQL index. Per the upstream guidance, when a FlowField's source key carries that property, "SIFT cannot function, COUNT/SUM will table-scan." The flag is sometimes set to save write-path cost on a rarely-queried key, but if a `CalcFormula` aggregates through that exact key, the FlowField loses its index — every `CalcFields`/`CalcSums`/list-page filter that triggers it runs without one.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When changing a key property to `MaintainSQLIndex = false`, find every FlowField whose `CalcFormula` filters on that key and verify another key covers the same fields. When adding a FlowField whose source table has only a `MaintainSQLIndex = false` key for its filter columns, add a fully-indexed key (or accept that the FlowField cannot ride SIFT and reshape the design — see `flowfield-source-key-needs-sumindexfields.md`).
|
||||||
|
|
||||||
|
See sample: `maintainsqlindex-false-breaks-flowfield-sift.bad.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
A FlowField whose `CalcFormula`'s `WHERE` columns line up with a key that has `MaintainSQLIndex = false`. The schema looks correct — the key exists, the SumIndexFields are listed — but at runtime the platform has no SQL index to use, and the aggregation table-scans on every invocation.
|
||||||
|
|
@ -1,10 +0,0 @@
|
||||||
codeunit 50107 "Perf Sample OnlyFetchUsed Bad"
|
|
||||||
{
|
|
||||||
procedure CustomerHasEntries(CustomerNo: Code[20]): Boolean
|
|
||||||
var
|
|
||||||
CustLedgerEntry: Record "Cust. Ledger Entry";
|
|
||||||
begin
|
|
||||||
CustLedgerEntry.SetRange("Customer No.", CustomerNo);
|
|
||||||
exit(CustLedgerEntry.FindSet());
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,10 +0,0 @@
|
||||||
codeunit 50106 "Perf Sample OnlyFetchUsed Good"
|
|
||||||
{
|
|
||||||
procedure CustomerHasEntries(CustomerNo: Code[20]): Boolean
|
|
||||||
var
|
|
||||||
CustLedgerEntry: Record "Cust. Ledger Entry";
|
|
||||||
begin
|
|
||||||
CustLedgerEntry.SetRange("Customer No.", CustomerNo);
|
|
||||||
exit(not CustLedgerEntry.IsEmpty());
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [findset, get, aa0175, wasted-fetch, read]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Only fetch records you use
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
CodeCop rule AA0175 flags code that retrieves a record and then does not use it. Every Find, FindSet, FindFirst, FindLast, or Get has a cost: the platform reads rows from SQL, materializes them, and transports them to the AL runtime. A call whose result is never read is wasted work, and on hot tables that work is never free.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Retrieve a record only when you need one or more of its field values. When you only need to know whether at least one row matches a filter, use IsEmpty (see use-isempty-for-existence-checks). When you only need a subset of fields, use SetLoadFields (see use-setloadfields-for-partial-records).
|
|
||||||
|
|
||||||
See sample: `only-fetch-records-you-use.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Calling FindSet or Get and then ignoring the result, or using it only as a boolean existence test, performs the full fetch and throws the data away.
|
|
||||||
|
|
||||||
See sample: `only-fetch-records-you-use.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,13 @@
|
||||||
|
codeunit 50209 "Perf Sample FindSetNext Bad"
|
||||||
|
{
|
||||||
|
procedure SumCustomerBalances() Total: Decimal
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
// AA0233: FindFirst paired with Next — single-row API used to iterate.
|
||||||
|
if Customer.FindFirst() then
|
||||||
|
repeat
|
||||||
|
Total += Customer."Balance (LCY)";
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,18 @@
|
||||||
|
codeunit 50208 "Perf Sample FindSetNext Good"
|
||||||
|
{
|
||||||
|
procedure SumCustomerBalances() Total: Decimal
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
if Customer.FindSet() then
|
||||||
|
repeat
|
||||||
|
Total += Customer."Balance (LCY)";
|
||||||
|
until Customer.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
procedure GetFirstUSCustomer(var Customer: Record Customer): Boolean
|
||||||
|
begin
|
||||||
|
Customer.SetRange("Country/Region Code", 'US');
|
||||||
|
exit(Customer.FindFirst());
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,26 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [findset, findfirst, findlast, get, next, repeat-until, aa0181, aa0233]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Use FindSet with repeat..Next; do not pair FindFirst/FindLast/Get with Next
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Two CodeCop rules carve out the loop pattern. AA0181 says `FindSet()`/`Find()` "must be used with `Next()` method" — these are the multi-row APIs that the runtime sets up for forward iteration. AA0233 says do "NOT use `FindFirst()`/`FindLast()`/`Get()` with `Next()`" — these are single-row APIs, and iterating from them "wastes CPU and bandwidth." Both rules together define one boundary: choose `FindSet` when the body iterates; choose `FindFirst`, `FindLast`, or `Get` when the body uses exactly one record.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When the body executes `repeat ... until Next() = 0;`, open the iteration with `FindSet()`. When the body needs one record and does not call `Next`, use `FindFirst`, `FindLast`, or — if the full primary key is known — `Get` (see `use-get-instead-of-findfirst-on-full-primary-key.md`). The choice is per call site, not a global preference.
|
||||||
|
|
||||||
|
See sample: `pair-findset-with-next-loop.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`if Customer.FindFirst() then repeat ... until Customer.Next() = 0;` — AA0233 flags this. The single-row API does not prepare the runtime for iteration, so the loop pays a cost the FindSet path does not. The mirror anti-pattern is calling `FindSet` to read a single record (see `use-isempty-for-existence-check.md` when only existence is required).
|
||||||
|
|
||||||
|
See sample: `pair-findset-with-next-loop.bad.al`.
|
||||||
|
|
@ -0,0 +1,21 @@
|
||||||
|
codeunit 50240 "Perf Sample Trigger Param Good"
|
||||||
|
{
|
||||||
|
procedure BulkFlagOrders(var SalesHeader: Record "Sales Header")
|
||||||
|
begin
|
||||||
|
if SalesHeader.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
SalesHeader."Job Queue Status" := SalesHeader."Job Queue Status"::"Scheduled for Posting";
|
||||||
|
// Trigger has nothing to add for a status flip in this code path.
|
||||||
|
SalesHeader.Modify(false);
|
||||||
|
until SalesHeader.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
procedure CreateOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20])
|
||||||
|
begin
|
||||||
|
SalesHeader.Init();
|
||||||
|
SalesHeader."Document Type" := SalesHeader."Document Type"::Order;
|
||||||
|
SalesHeader."Sell-to Customer No." := CustomerNo;
|
||||||
|
// OnInsert allocates the No.-Series number — the trigger is required.
|
||||||
|
SalesHeader.Insert(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,24 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [insert, modify, delete, trigger, run-trigger, write-parameters]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Pass false to Insert/Modify/Delete when the table triggers do not need to fire
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`Insert(true)`, `Modify(true)`, and `Delete(true)` run the table's `OnInsert`/`OnModify`/`OnDelete` trigger; the `(false)` form skips it. Per the upstream guidance, the trigger form should be used "only when needed" — every row whose write fires a trigger pays that cost, even when the trigger has nothing useful to add for the current call site. For tight bulk write paths the difference compounds linearly with row count.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Reach for the `(false)` form when the calling code already enforces the invariants the trigger would, or when the trigger is empty for the current table/extension. Use `(true)` when the trigger does work the caller depends on (number-series allocation, validation, cascading writes). Decide per call, not by code style: a default of "always `true`" makes bulk writes pay for triggers they did not need, and a default of "always `false`" silently skips validation the trigger was put there to enforce.
|
||||||
|
|
||||||
|
See sample: `pass-false-to-insert-when-trigger-not-needed.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Looping over thousands of rows and calling `Modify(true)` on each, when the table's `OnModify` trigger does nothing relevant for the operation. The trigger cost is paid per row; the user-visible behavior is identical to the `(false)` form. The mirror is using `(false)` for an operation that depends on trigger-side defaulting and silently producing rows that fail downstream validation.
|
||||||
|
|
@ -0,0 +1,22 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: performance
|
||||||
|
keywords: [dictionary, temporary-table, lookup, o-of-1, key-lookup]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Prefer a Dictionary over a temporary table for pure lookups
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
A temporary table supports a full record API — filters, iteration, multi-field keys — but a pure key→value lookup pays for plumbing it does not use. Per the upstream guidance, "if a temporary table record is ONLY used as a lookup table, it is faster to use a dictionary which supports O(1) lookups instead of O(lg n) for temporary tables." The Dictionary type has no record machinery to traverse; the key hash answers the lookup directly.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
When the use of a temp record is "set a key, see if the row exists, read a single value", switch to `Dictionary of [Key, Value]`. Use the temp-table form when the use genuinely needs filtering, iteration in a specific order, or a multi-field key. Compatibility with code that expects a `Record` parameter is a real reason to keep the temp table; performance alone, on a pure lookup, is not.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
A temp `Record` declared, populated row by row, then queried with `SetRange(KeyField, X); if Find('=') then Value := Rec.ValueField;`. The lookup hashes the key behind the scenes and does the same work a `Dictionary` would, plus the per-row record overhead. The pattern often appears because the author originally needed iteration and the iteration was later removed without revisiting the data structure.
|
||||||
|
|
@ -1,20 +0,0 @@
|
||||||
codeunit 50135 "Perf Sample RecordRef Bad"
|
|
||||||
{
|
|
||||||
procedure BlockCustomer(CustomerNo: Code[20])
|
|
||||||
var
|
|
||||||
RecRef: RecordRef;
|
|
||||||
PkRef: KeyRef;
|
|
||||||
NoRef: FieldRef;
|
|
||||||
BlockedRef: FieldRef;
|
|
||||||
begin
|
|
||||||
RecRef.Open(Database::Customer);
|
|
||||||
PkRef := RecRef.KeyIndex(1);
|
|
||||||
NoRef := PkRef.FieldIndex(1);
|
|
||||||
NoRef.SetRange(CustomerNo);
|
|
||||||
if not RecRef.FindFirst() then
|
|
||||||
exit;
|
|
||||||
BlockedRef := RecRef.Field(54);
|
|
||||||
BlockedRef.Value(2);
|
|
||||||
RecRef.Modify(true);
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,12 +0,0 @@
|
||||||
codeunit 50134 "Perf Sample RecordRef Good"
|
|
||||||
{
|
|
||||||
procedure BlockCustomer(CustomerNo: Code[20])
|
|
||||||
var
|
|
||||||
Customer: Record Customer;
|
|
||||||
begin
|
|
||||||
if not Customer.Get(CustomerNo) then
|
|
||||||
exit;
|
|
||||||
Customer.Blocked := Customer.Blocked::All;
|
|
||||||
Customer.Modify(true);
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
|
|
@ -1,29 +0,0 @@
|
||||||
---
|
|
||||||
bc-version: [26..28]
|
|
||||||
domain: performance
|
|
||||||
keywords: [recordref, fieldref, dynamic, reflection]
|
|
||||||
technologies: [al]
|
|
||||||
countries: [w1]
|
|
||||||
application-area: [all]
|
|
||||||
---
|
|
||||||
|
|
||||||
# Prefer direct record access over RecordRef where possible
|
|
||||||
|
|
||||||
> **Seed article.** Converted from an existing performance-review prompt to bootstrap the BCQuality performance corpus. Domain stewards should expand, restructure, and refine as needed.
|
|
||||||
|
|
||||||
## Description
|
|
||||||
|
|
||||||
RecordRef and FieldRef are the platform's reflection API: they work across tables the compiler does not know at authoring time. That flexibility costs per-operation overhead — every field access goes through a lookup — and loses compile-time type checking. For operations where the table is known, a strongly-typed Record variable is simpler and faster.
|
|
||||||
|
|
||||||
## Best Practice
|
|
||||||
|
|
||||||
Use Record variables for code paths that target a known table. Reach for RecordRef and FieldRef only when the table is genuinely dynamic (generic export/import, field-agnostic utilities, cross-table integrations).
|
|
||||||
|
|
||||||
See sample: `prefer-direct-record-over-recordref.good.al`.
|
|
||||||
|
|
||||||
## Anti Pattern
|
|
||||||
|
|
||||||
Using RecordRef as a habit, even when the target table is hardcoded two lines earlier, costs performance and hides intent from reviewers.
|
|
||||||
|
|
||||||
See sample: `prefer-direct-record-over-recordref.bad.al`.
|
|
||||||
|
|
||||||
|
|
@ -1,10 +0,0 @@
|
||||||
codeunit 50130 "Perf Sample GetVsFind Good"
|
|
||||||
{
|
|
||||||
procedure CustomerName(CustomerNo: Code[20]): Text[100]
|
|
||||||
var
|
|
||||||
Customer: Record Customer;
|
|
||||||
begin
|
|
||||||
if Customer.Get(CustomerNo) then
|
|
||||||
exit(Customer.Name);
|
|
||||||
end;
|
|
||||||
}
|
|
||||||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Add table
Add a link
Reference in a new issue