From 0922d85bf05829b9bbdfde960ccc7a51acf5fa6b Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 5 Jul 2026 19:00:47 +0530 Subject: [PATCH] docs: catalog second-order GROUP BY traps (wrap-is-the-bug classes) A full audit of the loose-GROUP-BY fixes found four recurring mistakes in the fixes themselves: incoherent Max/Min pairs over coupled columns, NULL-skipping Max on discriminators, Sum(x)*Max(y) fabricated arithmetic, and wrong-bound picks. Add them to the compatibility catalog (new 3.1) and to the Greptile review instructions so future PRs get flagged. --- .github/POSTGRES_COMPATIBILITY.md | 28 ++++++++++++++++++++++++++++ .greptile/config.json | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md index 6b08f81fdb4..9dee4cb0f0d 100644 --- a/.github/POSTGRES_COMPATIBILITY.md +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -150,6 +150,34 @@ Add it only when it is functionally dependent on the existing select columns; ot SQL `ORDER BY` and **sort in Python** (`key=str.casefold`, per §2) so the distinct row set is unchanged. +### 3.1 Second-order traps — when the `Max()`/`Min()` wrap itself is the bug + +The wrap is only a no-op when the column is provably single-valued per group (**"`Max()` means +provably constant"**). When the column can genuinely vary, the wrap is a decision, and a full +audit of these fixes found four recurring mistakes: + +- **Incoherent pair** — two semantically-coupled columns (a flag + a link: + `is_phantom_item` + `bom_no`; a discriminator + its value) aggregated with *independent* + `Max()`/`Min()` can pair values from **different rows** — a chimera row that never existed. + MariaDB's loose pick was at least row-coherent. Fix: group by the pair (when consumers + tolerate the extra rows), or select one **representative row** (`Min(child.name)` subquery + + join-back) so every column comes from the same line. +- **NULL-skipping** — `MAX`/`MIN` ignore NULLs, so `Max()` over a mostly-NULL discriminator + (an `original_item`-style column) *deterministically* returns the non-NULL value where + MariaDB could return NULL — deterministically wrong where the old behavior was only + intermittently wrong. Flag it wherever "no value" is a meaningful state (fallback gates, + dict keys). +- **Fabricated arithmetic** — `Sum(x) * Max(y)` where `y` varies within the group invents a + number no row ever had (and `Max` biases it upward) — poisonous when it feeds validation, + budgets, valuation, or GL/stock values. Fix per-row: `Sum(x * y)`. +- **Wrong bound** — where the value has a semantic, pick the bound deliberately: + `Min(schedule_date)` for a "required by", `Min(idx)` for first-line ordering, a qty-weighted + average for a rate. A blind `Max` can understate urgency or overstate a figure. + +Review heuristic: **if choosing between `Max` and `Min` would change the answer, the column is +not functionally dependent** — wrapping either is the wrong fix. Group by it, restructure, or +pick a bound for a stated reason, and cover the varying-group case with a test. + --- ## 4. False positives — do NOT flag these diff --git a/.greptile/config.json b/.greptile/config.json index 3c69e6e63fe..e3492ac7c0d 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -7,7 +7,7 @@ "frappe/frappe" ] }, - "instructions": "ERPNext runs on both MariaDB and PostgreSQL from one codebase, but the PostgreSQL test job is label-gated and may not run on this PR, so review every new or changed database query (raw frappe.db.sql, frappe.qb, frappe.get_all/get_list/get_value, and report SQL) for cross-engine compatibility. PRIME RULE: MariaDB output must never change; PostgreSQL is bent to match MariaDB, never the reverse, so a change to the value, row count, or ordering MariaDB produced is a regression even if it looks more correct (the only accepted change is replacing an arbitrary/undefined result with a deterministic one, row count preserved, and it should be called out). Flag a changed query that (1) would ERROR on PostgreSQL: loose GROUP BY (selecting/ordering a column neither grouped nor aggregated -- including an aggregate like Sum()/Count() selected next to bare columns with NO .groupby() at all), MySQL-only functions (TIMESTAMP(date,time), TIMEDIFF, STR_TO_DATE, DATE_FORMAT, DATE_ADD/DATE_SUB, GROUP_CONCAT, PERIOD_DIFF, SQL IF()), .rlike()/RLIKE (frappe rewrites REGEXP->~* on PostgreSQL but does NOT translate RLIKE; use .regexp()), .like()/LIKE on a NON-text column such as idx/docstatus (bigint ILIKE has no operator; Cast_(col,'varchar') first), CAST AS CHAR / Cast(x,'char') (bare CHAR is character(1) on PostgreSQL and truncates multi-digit values; use 'varchar'), UPDATE..JOIN, HAVING on a SELECT alias, SELECT DISTINCT with an ORDER BY expr not in the select list, single-quoted column aliases, varchar bitwise OR, capital-cased identifiers used as fieldnames in get_value(dt,dn,'Status') or get_all(dt,fields=['Account']) (PostgreSQL matches the quoted identifier case-sensitively; use the stored lower-case name), a Python bool written to a Check/int column via set_value/db_set/qb.update().set() instead of 1/0, or IfNull/Coalesce of a typed column with a different-typed literal such as IfNull(date_col, 0) -> COALESCE(date, integer) (PostgreSQL: 'COALESCE types date and integer cannot be matched'; the common IfNull(date,0) != 0 / == 0 presence test should be date_col.isnotnull() / .isnull(), else coalesce to a same-type default), or division by a possibly-zero divisor (Sum(a)/Sum(b) or x/col where the data can drive the divisor to 0 -- MariaDB returns NULL for division by zero but PostgreSQL raises 'division by zero' and aborts the query, so wrap the divisor in NullIf(divisor, 0)); or (2) would SILENTLY DIVERGE across engines: case-sensitive ==/.isin()/Strpos on USER-ENTERED free-text columns such as Data/Small Text/Long Text but NOT on Link/Select/name columns where exact-case matching is intended (PostgreSQL is case-sensitive, use Lower() both sides), lowercasing a value used as a document-name lookup, empty-string vs NULL in Concat/Concat_ws (MariaDB CONCAT(x,NULL) is NULL but PostgreSQL CONCAT drops the NULL, so a label like Concat('MFG-', nullable_date) leaks a bare 'MFG-' on PostgreSQL -- guard with Case/Coalesce/NullIf), NULL ordering (PostgreSQL sorts NULLs last) in ORDER BY..LIMIT 1, integer division (int/int truncates on PostgreSQL; multiply by 100.0 or make a literal a float, e.g. col/1440 -> col/1440.0), get_all(distinct=True, order_by=...) (frappe DROPS the ORDER BY for distinct queries on PostgreSQL, so sort in Python with key=str.casefold), an engine-specific function rewrite that does not match MariaDB on edge cases, or UnixTimestamp(date)/date-to-epoch math that is timezone-dependent (a strict epoch <= now bound is flaky on PostgreSQL). Also flag CATCH-AND-CONTINUE inserts: on PostgreSQL a failed insert aborts the WHOLE transaction (InFailedSqlTransaction), so code that swallows a duplicate/unique error and keeps going in the same transaction must wrap the fallible insert in frappe.db.savepoint(name) + rollback(save_point=name), unless it re-throws with no DB call before the throw or the insert uses ignore_if_duplicate=True or autoname='hash'. When RECOVERING the poisoned txn, prefer a SCOPED savepoint over a full frappe.db.rollback(): a full rollback discards rows the handler already created before the failure -- which MariaDB keeps -- so it is a silent MariaDB regression. 'The bg job / whitelist entrypoint owns the txn' does NOT make a full rollback safe if it did multiple inserts in a loop first; a full rollback is safe only when it immediately re-throws/raises, has nothing successful before it (single op), or the batch is meant to be atomic (a partial result is invalid -> rollback + mark Failed is correct). Otherwise use a per-iteration/per-record savepoint, and keep the function's success/None return contract (don't return a value for a doc that was just rolled back). GROUP BY ROW-COUNT TRAP (most important): to make a loose GROUP BY PostgreSQL-valid, do NOT add a non-functionally-dependent column (the classic traps are the child/row primary key or an editable per-row field) to GROUP BY because that splits one row into N and changes the MariaDB row count; Max()/Min()-wrap it instead (row count preserved, value arbitrary to deterministic). Judge functional dependence by the SOURCE TABLE: a column from a master joined on the group key is FD and safe in GROUP BY, but a descriptive field on the transaction table (e.g. t1.supplier_name, t1.territory) is NOT FD and must be wrapped. The SAME row-count trap applies to SELECT DISTINCT: to satisfy PostgreSQL's ORDER-BY-expr-must-appear-in-the-select rule, do NOT blindly add the ordered column to the select -- if it is not single-valued per existing distinct row the DISTINCT key grows and MariaDB returns MORE rows (a regression); add it only if functionally dependent on the existing select columns, otherwise drop the SQL ORDER BY and sort in Python (key=str.casefold). Do NOT suggest changing a Max()-wrapped column to Sum() to make a number more correct, that changes MariaDB's value. REFACTOR / CONVERSION FAITHFULNESS: a commit labeled a 'refactor' or a raw-frappe.db.sql->frappe.qb/ORM conversion is meant to preserve behaviour but easily does not, and the change slips past the static checker and a one-engine green run -- diff the WHERE/predicate, the JOIN/ON conditions and the resulting ROW SET, not just the SELECT shape. A conversion that silently widens or narrows the filter (e.g. a 'posting_datetime > X' bound gaining an OR (posting_datetime == X AND creation > args.creation) branch under a sql->qb refactor) changes the rows touched on BOTH engines and is a regression hiding under a refactor label; call it out and require a test even if it is a deliberate bug-fix. DO NOT FLAG these false positives: .like()/['like'] on a TEXT column (already ILIKE on PostgreSQL -- but DO flag it on a non-text/integer column, see above), raw ifnull/backticks/LOCATE/REGEXP/.regexp() inside frappe.db.sql (auto-translated by the framework -- but RLIKE/.rlike() is NOT translated, see above), or an ORDER BY..LIMIT 1 tie where adding a tiebreaker would change MariaDB's current pick. Full catalog with examples and portable fixes is in .github/POSTGRES_COMPATIBILITY.md.", + "instructions": "ERPNext runs on both MariaDB and PostgreSQL from one codebase, but the PostgreSQL test job is label-gated and may not run on this PR, so review every new or changed database query (raw frappe.db.sql, frappe.qb, frappe.get_all/get_list/get_value, and report SQL) for cross-engine compatibility. PRIME RULE: MariaDB output must never change; PostgreSQL is bent to match MariaDB, never the reverse, so a change to the value, row count, or ordering MariaDB produced is a regression even if it looks more correct (the only accepted change is replacing an arbitrary/undefined result with a deterministic one, row count preserved, and it should be called out). Flag a changed query that (1) would ERROR on PostgreSQL: loose GROUP BY (selecting/ordering a column neither grouped nor aggregated -- including an aggregate like Sum()/Count() selected next to bare columns with NO .groupby() at all), MySQL-only functions (TIMESTAMP(date,time), TIMEDIFF, STR_TO_DATE, DATE_FORMAT, DATE_ADD/DATE_SUB, GROUP_CONCAT, PERIOD_DIFF, SQL IF()), .rlike()/RLIKE (frappe rewrites REGEXP->~* on PostgreSQL but does NOT translate RLIKE; use .regexp()), .like()/LIKE on a NON-text column such as idx/docstatus (bigint ILIKE has no operator; Cast_(col,'varchar') first), CAST AS CHAR / Cast(x,'char') (bare CHAR is character(1) on PostgreSQL and truncates multi-digit values; use 'varchar'), UPDATE..JOIN, HAVING on a SELECT alias, SELECT DISTINCT with an ORDER BY expr not in the select list, single-quoted column aliases, varchar bitwise OR, capital-cased identifiers used as fieldnames in get_value(dt,dn,'Status') or get_all(dt,fields=['Account']) (PostgreSQL matches the quoted identifier case-sensitively; use the stored lower-case name), a Python bool written to a Check/int column via set_value/db_set/qb.update().set() instead of 1/0, or IfNull/Coalesce of a typed column with a different-typed literal such as IfNull(date_col, 0) -> COALESCE(date, integer) (PostgreSQL: 'COALESCE types date and integer cannot be matched'; the common IfNull(date,0) != 0 / == 0 presence test should be date_col.isnotnull() / .isnull(), else coalesce to a same-type default), or division by a possibly-zero divisor (Sum(a)/Sum(b) or x/col where the data can drive the divisor to 0 -- MariaDB returns NULL for division by zero but PostgreSQL raises 'division by zero' and aborts the query, so wrap the divisor in NullIf(divisor, 0)); or (2) would SILENTLY DIVERGE across engines: case-sensitive ==/.isin()/Strpos on USER-ENTERED free-text columns such as Data/Small Text/Long Text but NOT on Link/Select/name columns where exact-case matching is intended (PostgreSQL is case-sensitive, use Lower() both sides), lowercasing a value used as a document-name lookup, empty-string vs NULL in Concat/Concat_ws (MariaDB CONCAT(x,NULL) is NULL but PostgreSQL CONCAT drops the NULL, so a label like Concat('MFG-', nullable_date) leaks a bare 'MFG-' on PostgreSQL -- guard with Case/Coalesce/NullIf), NULL ordering (PostgreSQL sorts NULLs last) in ORDER BY..LIMIT 1, integer division (int/int truncates on PostgreSQL; multiply by 100.0 or make a literal a float, e.g. col/1440 -> col/1440.0), get_all(distinct=True, order_by=...) (frappe DROPS the ORDER BY for distinct queries on PostgreSQL, so sort in Python with key=str.casefold), an engine-specific function rewrite that does not match MariaDB on edge cases, or UnixTimestamp(date)/date-to-epoch math that is timezone-dependent (a strict epoch <= now bound is flaky on PostgreSQL). Also flag CATCH-AND-CONTINUE inserts: on PostgreSQL a failed insert aborts the WHOLE transaction (InFailedSqlTransaction), so code that swallows a duplicate/unique error and keeps going in the same transaction must wrap the fallible insert in frappe.db.savepoint(name) + rollback(save_point=name), unless it re-throws with no DB call before the throw or the insert uses ignore_if_duplicate=True or autoname='hash'. When RECOVERING the poisoned txn, prefer a SCOPED savepoint over a full frappe.db.rollback(): a full rollback discards rows the handler already created before the failure -- which MariaDB keeps -- so it is a silent MariaDB regression. 'The bg job / whitelist entrypoint owns the txn' does NOT make a full rollback safe if it did multiple inserts in a loop first; a full rollback is safe only when it immediately re-throws/raises, has nothing successful before it (single op), or the batch is meant to be atomic (a partial result is invalid -> rollback + mark Failed is correct). Otherwise use a per-iteration/per-record savepoint, and keep the function's success/None return contract (don't return a value for a doc that was just rolled back). GROUP BY ROW-COUNT TRAP (most important): to make a loose GROUP BY PostgreSQL-valid, do NOT add a non-functionally-dependent column (the classic traps are the child/row primary key or an editable per-row field) to GROUP BY because that splits one row into N and changes the MariaDB row count; Max()/Min()-wrap it instead (row count preserved, value arbitrary to deterministic). Judge functional dependence by the SOURCE TABLE: a column from a master joined on the group key is FD and safe in GROUP BY, but a descriptive field on the transaction table (e.g. t1.supplier_name, t1.territory) is NOT FD and must be wrapped. The SAME row-count trap applies to SELECT DISTINCT: to satisfy PostgreSQL's ORDER-BY-expr-must-appear-in-the-select rule, do NOT blindly add the ordered column to the select -- if it is not single-valued per existing distinct row the DISTINCT key grows and MariaDB returns MORE rows (a regression); add it only if functionally dependent on the existing select columns, otherwise drop the SQL ORDER BY and sort in Python (key=str.casefold). Do NOT suggest changing a Max()-wrapped column to Sum() to make a number more correct, that changes MariaDB's value. SECOND-ORDER GROUP BY TRAPS (the Max()/Min() wrap itself can be the bug -- a wrap is only a no-op when the column is provably single-valued per group): (a) INCOHERENT PAIR: two semantically-coupled columns (a flag + a link like is_phantom_item + bom_no, a discriminator + its value) aggregated with INDEPENDENT Max()/Min() can pair values from DIFFERENT rows into a chimera row that never existed (MariaDB's loose pick was at least row-coherent) -- when a consumer uses the two values together (recursion into the link gated by the flag, dict keys, link+flag display) require grouping by the pair or a single representative-row subquery (Min(child.name) + join back). (b) NULL-SKIPPING: Max/Min ignore NULLs, so Max() over a mostly-NULL discriminator deterministically prefers the non-NULL value where MariaDB could return NULL -- flag when 'no value' is a meaningful state (fallback gates like 'if x:', dict keys, status decisions). (c) FABRICATED ARITHMETIC: Sum(x) * Max(y) -- or Python arithmetic combining a Sum'd and a Max'd column from the same grouped query -- where y can vary within the group invents a value no row ever had and Max biases it upward; require per-row Sum(x*y) when it feeds validation, budgets, valuation, or GL/stock values. (d) WRONG BOUND: when the aggregated value has a semantic, the bound must be chosen deliberately (Min(schedule_date) for a 'required by' date, Min(idx) for first-line ordering, a qty-weighted average for a rate); a blind Max can understate urgency or overstate a figure. Heuristic: if switching Max<->Min would change the answer, the column is NOT functionally dependent and wrapping either is the wrong fix -- group by it, restructure, or pick a bound for a stated reason with a test. REFACTOR / CONVERSION FAITHFULNESS: a commit labeled a 'refactor' or a raw-frappe.db.sql->frappe.qb/ORM conversion is meant to preserve behaviour but easily does not, and the change slips past the static checker and a one-engine green run -- diff the WHERE/predicate, the JOIN/ON conditions and the resulting ROW SET, not just the SELECT shape. A conversion that silently widens or narrows the filter (e.g. a 'posting_datetime > X' bound gaining an OR (posting_datetime == X AND creation > args.creation) branch under a sql->qb refactor) changes the rows touched on BOTH engines and is a regression hiding under a refactor label; call it out and require a test even if it is a deliberate bug-fix. DO NOT FLAG these false positives: .like()/['like'] on a TEXT column (already ILIKE on PostgreSQL -- but DO flag it on a non-text/integer column, see above), raw ifnull/backticks/LOCATE/REGEXP/.regexp() inside frappe.db.sql (auto-translated by the framework -- but RLIKE/.rlike() is NOT translated, see above), or an ORDER BY..LIMIT 1 tie where adding a tiebreaker would change MariaDB's current pick. Full catalog with examples and portable fixes is in .github/POSTGRES_COMPATIBILITY.md.", "customContext": { "files": [ {