Merge pull request #132 from microsoft/gggdttt-refine-self-improvement-guidance

Refine self-improvement review guidance

microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al

@@ -3,12 +3,24 @@ codeunit 50129 "Perf Sample CommitInLoop Bad"
     procedure NormalizeCustomerNames()
     var
         Customer: Record Customer;
+        LastCustomerNo: Code[20];
+        ProcessedCount: Integer;
     begin
+        Customer.SetFilter("No.", '>%1', LastCustomerNo);
         if Customer.FindSet(true) then
             repeat
                 Customer.Name := UpperCase(Customer.Name);
                 Customer.Modify();
-                Commit();
+
+                // LastCustomerNo exists only in memory, so a retry cannot exclude
+                // work that was already committed.
+                LastCustomerNo := Customer."No.";
+                ProcessedCount += 1;
+
+                // This still opened a FindSet over the complete remaining tail;
+                // periodic commits do not turn retrieval into bounded TOP X.
+                if ProcessedCount mod 500 = 0 then
+                    Commit();
             until Customer.Next() = 0;
     end;
 }

microsoft/knowledge/performance/avoid-commit-inside-loops.good.al

@@ -16,11 +16,22 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
 {
     procedure NormalizeCustomerNames()
     var
+        NormalizeState: Record "Perf Normalize State";
         LastCustomerNo: Code[20];
     begin
-        // The outer loop owns checkpoints; the per-row loop contains no Commit.
-        while NormalizeNextChunk(LastCustomerNo) do
+        if not NormalizeState.Get('CUSTOMER') then begin
+            NormalizeState.Init();
+            NormalizeState.Code := 'CUSTOMER';
+            NormalizeState.Insert();
+        end;
+        LastCustomerNo := NormalizeState."Last Customer No.";
+
+        while NormalizeNextChunk(LastCustomerNo) do begin
+            // Persist progress in the same transaction as the completed chunk.
+            NormalizeState."Last Customer No." := LastCustomerNo;
+            NormalizeState.Modify();
             Commit();
+        end;
     end;
 
     local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
@@ -58,3 +69,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
         exit(true);
     end;
 }
+
+table 50128 "Perf Normalize State"
+{
+    fields
+    {
+        field(1; Code; Code[10]) { }
+        field(2; "Last Customer No."; Code[20]) { }
+    }
+
+    keys
+    {
+        key(PK; Code) { Clustered = true; }
+    }
+}

microsoft/knowledge/performance/avoid-commit-inside-loops.md

@@ -13,16 +13,18 @@ application-area: [all]
 
 ## Description
 
-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 select an exact list of at most N keys and process only those rows.
+Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`).
+
+A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next).
 
 ## Best Practice
 
-If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
+If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
 
 See sample: `avoid-commit-inside-loops.good.al`.
 
 ## Anti Pattern
 
-Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
+Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
 
 See sample: `avoid-commit-inside-loops.bad.al`.

microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md

@@ -15,12 +15,12 @@ application-area: [all]
 
 ## Best Practice
 
-Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`).
+Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics.
 
 See sample: `prefer-modifyall-over-per-row-modify.good.al`.
 
 ## Anti Pattern
 
-A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
+A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
 
 See sample: `prefer-modifyall-over-per-row-modify.bad.al`.

microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md

@@ -1,7 +1,7 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber]
+keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress]
 technologies: [al]
 countries: [w1]
 application-area: [all]
@@ -11,12 +11,12 @@ application-area: [all]
 
 ## Description
 
-`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL.
+`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call.
 
 ## Best Practice
 
-Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls.
+Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API.
 
 ## Anti Pattern
 
-Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows.
+Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes.

microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md

@@ -1,7 +1,7 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass]
+keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass, security-filtering, media, companion-fields]
 technologies: [al]
 countries: [w1]
 application-area: [all]
@@ -13,16 +13,16 @@ application-area: [all]
 
 ## Description
 
-`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied. It is not guaranteed to stay one statement. The base table `OnDelete` trigger is skipped, but table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run. Extension event subscribers, global delete triggers, and media fields can also require row processing. `DeleteAll(true)` runs the base table `OnDelete` trigger as well and has no performance advantage over `Delete(true)` in a loop.
+`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied, but it is not guaranteed to stay one statement. Microsoft documents that `DeleteAll` [reverts to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. Setting `RunTrigger` to false skips the base table `OnDelete` trigger, but [table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method#remarks).
 
 ## Best Practice
 
-Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and installed extensions, subscribers, global triggers, and media fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately.
+Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and that trigger code, related subscribers, security filtering, media fields, and companion fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately.
 
 See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`.
 
 ## Anti Pattern
 
-Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking table extensions and subscribers.
+Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic or fallback condition. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking the documented fallback conditions.
 
 See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`.