From a26329b2a0fedfc495f5aa888369d4481369f36d Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 30 Jun 2026 20:41:34 +0530 Subject: [PATCH 1/3] ci(postgres): teach the parity guide the DISTINCT row-count trap + refactor faithfulness Two review lessons from the post-merge net-diff/whole-repo re-audit of the SQL-dialect classes: - Section 3 (row-count trap) now covers SELECT DISTINCT too: adding the ORDER BY column to the select to satisfy Postgres grows the DISTINCT key and changes the MariaDB row count when the column is not single-valued per distinct row -- sort in Python instead. - New section 6: a 'refactor' / raw-SQL->qb conversion is not automatically 1:1. Diff the WHERE/predicate and the resulting row set, not just the SELECT shape -- a conversion that widens a filter (e.g. posting_datetime > X gaining an OR (== X AND creation > ...) branch under a sql->qb refactor) changes the rows touched on both engines and hides under a refactor label. Co-Authored-By: Claude Opus 4.8 --- .github/POSTGRES_COMPATIBILITY.md | 35 +++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md index 6cac07afc24..19d0cee1862 100644 --- a/.github/POSTGRES_COMPATIBILITY.md +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -45,7 +45,9 @@ Flag a changed query that uses any of these: - **`HAVING` referencing a `SELECT` alias** — PostgreSQL rejects output-column aliases in `HAVING` (regardless of whether the query has a `GROUP BY`; MariaDB allows them). Repeat the underlying expression in `HAVING`, or move a non-aggregate predicate into `WHERE`. -- **`SELECT DISTINCT … ORDER BY `** — add the expr to the select. +- **`SELECT DISTINCT … ORDER BY `** — add the expr to the select + **only if it is single-valued per distinct row**; otherwise it grows the `DISTINCT` key and the + MariaDB row count (see §3) — drop the SQL `ORDER BY` and sort in Python instead. - **Single-quoted column alias** `AS 'x'` — PostgreSQL reads `'x'` as a string literal. Use an unquoted (or double-quoted) alias. - **`varchar | varchar`** (bitwise OR misused as a coalesce) — errors on PostgreSQL. Use @@ -119,7 +121,7 @@ These don't error, so a one-engine CI stays green. Flag them: --- -## 3. The `GROUP BY` row-count trap (the single most important rule) +## 3. The row-count trap — `GROUP BY` **and** `DISTINCT` (the single most important rule) When making a loose `GROUP BY` PostgreSQL-valid, **do not add a non-functionally-dependent column to the `GROUP BY` just to satisfy PostgreSQL** — that turns one group row into N and @@ -140,6 +142,14 @@ versa) to make a number "more correct" — that changes the MariaDB value. The w MariaDB's prior one-value-per-group output; a different aggregate is a product change, out of scope for a portability fix. +**The same trap applies to `SELECT DISTINCT`.** To satisfy PostgreSQL's "an `ORDER BY` expr must +appear in the select list under `DISTINCT`" 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), exactly as adding a non-FD column to `GROUP BY` does. +Add it only when it is functionally dependent on the existing select columns; otherwise drop the +SQL `ORDER BY` and **sort in Python** (`key=str.casefold`, per §2) so the distinct row set is +unchanged. + --- ## 4. False positives — do NOT flag these @@ -170,10 +180,27 @@ These are auto-handled by the framework and are **not** breaks: --- +## 6. Refactors and raw-SQL→ORM conversions are not automatically 1:1 + +A commit labeled a **refactor** or a **raw-`frappe.db.sql` → `frappe.qb`/ORM conversion** is meant +to preserve behaviour — but it easily doesn't, and the change passes 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 +changes the rows touched on **both** engines and is a regression hiding under a "refactor" label. + +Real example: an `UPDATE` whose bound was `posting_datetime > X` gained an +`OR (posting_datetime == X AND creation > args.creation)` branch during a "`sql` → `qb` refactor", +widening the rows updated on both engines. Even when such a change is a deliberate bug-fix it must +be called out and tested — it is **not** the no-op the refactor label implies. Confirm the +converted query touches exactly the same rows with the same values MariaDB produced before. + +--- + ## How to review -For every changed query: does it (a) use a construct from §1 (would error on PostgreSQL), or -(b) match a divergence in §2/§3 (different result across engines)? If so, comment with the +For every changed query: does it (a) use a construct from §1 (would error on PostgreSQL), +(b) match a divergence in §2/§3 (different result across engines), or (c) change the row set under +a refactor/conversion label (§6)? If so, comment with the portable fix and confirm it leaves **MariaDB output unchanged**. Skip the §4 false positives. Prefer a comment that names the rule (e.g. "loose GROUP BY — Max()-wrap, don't add to GROUP BY: splits the row count") so the fix is unambiguous. From e79c24791fa686ac3d9914b096df5091ff9dabb3 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 30 Jun 2026 20:41:35 +0530 Subject: [PATCH 2/3] ci(greptile): flag the DISTINCT row-count trap and refactor-smuggled row-set changes Mirror the two new POSTGRES_COMPATIBILITY.md rules into the greptile instructions so the bot flags them on changed queries: (1) adding an ORDER BY column to a SELECT DISTINCT grows the distinct key and the MariaDB row count unless it is functionally dependent; (2) a refactor / raw-frappe.db.sql->qb conversion can silently change the WHERE/row set on both engines -- review the predicate, not just the query shape. Co-Authored-By: Claude Opus 4.8 --- .greptile/config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.greptile/config.json b/.greptile/config.json index ed875670346..12036fe99b2 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'. 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. Do NOT suggest changing a Max()-wrapped column to Sum() to make a number more correct, that changes MariaDB's value. 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'. 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.", "customContext": { "files": [ { From f560767eb0e074d76499eaae43cc27c7461dc6e8 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 30 Jun 2026 20:58:52 +0530 Subject: [PATCH 3/3] =?UTF-8?q?ci(postgres):=20include=20=C2=A76=20in=20th?= =?UTF-8?q?e=20How-to-review=20closing=20summary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The closing paragraph named only the §2/§3 semantic divergences as static-checker-invisible; §6 (refactor/conversion row-set changes) is equally invisible and belongs there too. (greptile nit.) Co-Authored-By: Claude Opus 4.8 --- .github/POSTGRES_COMPATIBILITY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md index 19d0cee1862..6f2445d8d44 100644 --- a/.github/POSTGRES_COMPATIBILITY.md +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -206,5 +206,5 @@ Prefer a comment that names the rule (e.g. "loose GROUP BY — Max()-wrap, don't splits the row count") so the fix is unambiguous. The static pre-commit checker (`.github/helper/postgres_compat.py`) catches the *mechanical* -§1 breaks; the **semantic** §2/§3 divergences are exactly what a reviewer (and this guide) must -cover, because no static check can see them. +§1 breaks; the **semantic** §2/§3 divergences and the §6 refactor/conversion row-set changes are +exactly what a reviewer (and this guide) must cover, because no static check can see them.