From 4a690c86d231bc70cb754b4f159309452fb996ea Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 22 Jun 2026 09:29:19 +0530 Subject: [PATCH] =?UTF-8?q?ci(greptile):=20teach=20the=20review=20bot=20to?= =?UTF-8?q?=20enforce=20MariaDB=E2=86=94PostgreSQL=20parity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PostgreSQL server-test job is label-gated, so until it is required the Greptile PR-review bot is the always-on guard against cross-engine breaks. Extend .greptile/config.json with `instructions` (and a `customContext` reference to a new guide) so every review flags new/changed queries that would error on PostgreSQL or silently diverge from MariaDB, under the prime rule that MariaDB output must not change. - .github/POSTGRES_COMPATIBILITY.md — the catalogue the bot (and contributors) follow: hard breaks (loose GROUP BY, MySQL-only funcs, UPDATE..JOIN, HAVING-on-alias, DISTINCT+ORDER BY, single-quoted alias, varchar bitwise OR, capital identifiers, set_value(Check,bool)), silent divergences (text case-sensitivity, name-lookup case, empty-string↔NULL, NULL ordering, ORDER BY..LIMIT 1 tiebreakers, integer division, distinct-drops-ORDER-BY-on-PG + casefold sorting, function-rewrite parity, UnixTimestamp TZ), the GROUP BY row-count trap (Max()-wrap vs add-to-GROUP-BY; FD-by-source-table), the InFailedSqlTransaction/savepoint rule, and the false positives NOT to flag (.like→ILIKE, ifnull/backtick/LOCATE/REGEXP auto-translation, MariaDB-changing tiebreakers). - Existing disabledLabels and frappe/frappe context are preserved. --- .github/POSTGRES_COMPATIBILITY.md | 151 ++++++++++++++++++++++++++++++ .greptile/config.json | 12 +++ 2 files changed, 163 insertions(+) create mode 100644 .github/POSTGRES_COMPATIBILITY.md diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md new file mode 100644 index 00000000000..952ea92b564 --- /dev/null +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -0,0 +1,151 @@ +# PostgreSQL compatibility — review guide + +ERPNext targets **both MariaDB and PostgreSQL from a single codebase**. The full server +test suite passes on both, but the PostgreSQL CI job is **label-gated** (it does not run on +every PR), so until it is required this guide is the always-on guard. Greptile loads it as +review context (`.greptile/config.json`). + +When reviewing a PR, flag any **new or changed query** (raw `frappe.db.sql`, `frappe.qb`, +`frappe.get_all/get_list/get_value`, report SQL) that would **error on PostgreSQL** or +**return different results on the two engines**. + +## The one rule that governs everything + +**MariaDB behaviour must not change; PostgreSQL is brought into line with MariaDB — never the +reverse.** A "fix" that changes the value, row count, or ordering MariaDB produced is a +regression, even if the new behaviour looks more correct. The only accepted MariaDB-output +change is replacing a genuinely *undefined/arbitrary* result with a deterministic one (row +count preserved) — and that should be called out explicitly. + +There are two failure modes to watch for: +1. **Hard breaks** — PostgreSQL raises an exception; MariaDB is green. Easy to catch in CI, + but the gated job may not run. +2. **Silent divergences** — both engines succeed but return *different* results. CI on one + engine stays green; the bug only shows on a PostgreSQL site. These are the dangerous ones. + +--- + +## 1. Hard breaks — would error on PostgreSQL + +Flag a changed query that uses any of these: + +- **Loose `GROUP BY`** — selecting/ordering a column that is neither in `GROUP BY` nor wrapped + in an aggregate. MariaDB tolerates it; PostgreSQL errors (`must appear in the GROUP BY + clause or be used in an aggregate function`). Fix: add it to `GROUP BY` **if it is + functionally dependent on the group key**, otherwise wrap it in `Max()`/`Min()`. **See §3 — + the row-count trap — before suggesting "add it to GROUP BY".** +- **MySQL-only functions** — `TIMESTAMP(date,time)`, `TIMEDIFF`, `STR_TO_DATE`, `DATE_FORMAT`, + `DATE_ADD/SUB`, `GROUP_CONCAT`, `PERIOD_DIFF`, SQL `IF(cond,a,b)`. Use the portable + `frappe.query_builder.functions` equivalents (`CombineDatetime`, `DateDiff`, `Case`, + `GroupConcat`, …) or a precomputed column (e.g. `posting_datetime`). +- **`UPDATE … JOIN`** — not valid on PostgreSQL. Rewrite as `UPDATE … WHERE name IN (subquery)`. +- **`HAVING` referencing a `SELECT` alias** with no `GROUP BY` — move the predicate into `WHERE` + on the underlying expression. +- **`SELECT DISTINCT … ORDER BY `** — add the expr to the select. +- **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 + `Coalesce(...)`. +- **Capital-cased identifiers** used as column/field names in `get_value(dt, dn, "Status")` and + similar — PostgreSQL folds unquoted identifiers to lower case; a stored column named + `status` won't match `"Status"`. Use the exact stored case. +- **Boolean passed where an integer column is expected** — `frappe.db.set_value(dt, dn, + check_field, True)` emits `SET col = true`, which PostgreSQL rejects on a `smallint` + (`DatatypeMismatch`). Pass `1`/`0`. + +--- + +## 2. Silent divergences — succeeds on both, returns different results + +These don't error, so a one-engine CI stays green. Flag them: + +- **Case sensitivity on text equality** — `==`, `.isin()`, `Strpos`/`Locate` on free-text + columns are case-**sensitive** on PostgreSQL but case-**insensitive** under MariaDB's default + collation. `Lower()` both sides. *(Not `.like()`/`["like", …]` — those already render as + `ILIKE` on PostgreSQL; see §4.)* +- **Case sensitivity in a doc-`name` lookup** — lower-casing a value then using it as a + document name in `get_value`/`get_doc`/`exists` misses on PostgreSQL (names are + case-sensitive). Keep original case for the identifier; lower-case only comparison operands. +- **Empty string vs NULL** — PostgreSQL stores a blank link/data field as `NULL` on some paths + while MariaDB keeps `''`; `Concat`/`Concat_ws` then diverge. Prefer the stored full value, or + `Coalesce(col, '')` per argument. +- **NULL ordering** — MariaDB sorts `NULL` first, PostgreSQL sorts it last. For + `ORDER BY … LIMIT 1`/`[0]` on a nullable column, guard with `Coalesce`/`isnotnull()`. +- **`ORDER BY … LIMIT 1` with no unique tiebreaker** — when rows tie on the ordered column the + two engines may pick different rows. Add a `creation`/`name` tiebreaker **only if it does not + change MariaDB's current pick** (see §4). +- **Integer division** — `COUNT(...) / COUNT(...) * 100` truncates to `0` on PostgreSQL + (integer/integer) but is decimal on MariaDB. Multiply by `100.0` first. +- **`DISTINCT` list ordering** — `frappe.get_all(distinct=True, order_by=…)` / + `SELECT DISTINCT … ORDER BY`: frappe's `db_query` **silently drops `ORDER BY` for distinct + queries on PostgreSQL**, so the result is unordered there. Sort in Python instead — and use + `key=str.casefold`, because bare `sorted()` is case-sensitive (ASCII) while MariaDB's + collation is case-insensitive, so a plain sort reorders MariaDB's output. +- **Engine-specific function rewrites** — e.g. a PostgreSQL `regexp_replace` branch + reimplementing MariaDB's `CAST(SUBSTRING_INDEX(name,' ',-1) AS UNSIGNED)` (leading digits of + the last whitespace token). Verify the rewrite matches MariaDB on edge cases (`"X - 3a"→3`, + `"X - 1.5"→1`) by diffing both engines on literal rows. +- **`UnixTimestamp(date)` / date→epoch** is timezone-dependent (midnight in the DB session TZ), + so a strict `epoch <= now` bound is flaky on PostgreSQL. + +--- + +## 3. The `GROUP BY` row-count trap (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 +**changes the MariaDB row count** (a regression). The classic traps are adding the **child/row +primary key** or an **editable per-row field**. Instead **`Max()`/`Min()`-wrap** the offending +column: the row count is preserved and the value goes from arbitrary (MariaDB's old loose pick) +to deterministic. + +**Judge functional dependence by the source table, not the column name:** +- A column from a **master joined on the group key** (`t3.x` where `t1.key = t3.name`) is FD → + safe to keep in `GROUP BY`. +- A descriptive field on the **transaction** table (`t1.supplier_name`, `t1.territory`, + `t1.item_name` — fetched/editable, can differ across historical rows for the same key) is + **not** FD even though it looks master-derived → `Max()`-wrap it. + +Conversely, do **not** suggest changing a `Max()`/`Min()`-wrapped column to `Sum()` (or vice +versa) to make a number "more correct" — that changes the MariaDB value. The wrap reproduces +MariaDB's prior one-value-per-group output; a different aggregate is a product change, out of +scope for a portability fix. + +--- + +## 4. False positives — do NOT flag these + +These are auto-handled by the framework and are **not** breaks: + +- **`.like()` / `["like", …]`** already renders as `ILIKE` on PostgreSQL — not a + case-sensitivity bug. +- **Raw `ifnull(...)`** inside `frappe.db.sql()` is rewritten to `coalesce(...)` on all engines. +- **Backticks**, **`LOCATE`**, **`REGEXP`** in raw SQL are auto-translated on PostgreSQL. +- **An `ORDER BY … LIMIT 1` tie where the two engines already agree**, or where adding a + tiebreaker would *change* MariaDB's current pick — leave it; "fixing" it would either change + MariaDB or has no observable effect. + +--- + +## 5. Transaction / runtime (not query-shape, still PostgreSQL-only) + +- **Catch-and-continue inserts** — on PostgreSQL a failed `insert()` aborts the **whole + transaction**, so code that swallows a duplicate and keeps going dies on the next statement + with `InFailedSqlTransaction` (frappe dropped its blanket per-statement savepoint in + frappe#40075). Such a handler must wrap the fallible insert in `frappe.db.savepoint(name)` + + `rollback(save_point=name)` — unless it re-`throw`s with no DB call before the throw, or the + insert uses `ignore_if_duplicate=True` / `autoname="hash"` (→ `ON CONFLICT DO NOTHING`). + +--- + +## 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 +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. + +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. diff --git a/.greptile/config.json b/.greptile/config.json index 8d9c41c662e..3ba2e594b25 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -6,5 +6,17 @@ "repos": [ "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), MySQL-only functions (TIMESTAMP(date,time), TIMEDIFF, STR_TO_DATE, DATE_FORMAT, DATE_ADD/DATE_SUB, GROUP_CONCAT, PERIOD_DIFF, SQL IF()), 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 in get_value(dt,dn,'Status'), or set_value/db_set of a Check field with a Python bool instead of 1/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, NULL ordering (PostgreSQL sorts NULLs last) in ORDER BY..LIMIT 1, integer division (multiply by 100.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'] (already ILIKE on PostgreSQL), raw ifnull/backticks/LOCATE/REGEXP inside frappe.db.sql (auto-translated by the framework), 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": [ + { + "scope": [ + "**/*.py" + ], + "path": ".github/POSTGRES_COMPATIBILITY.md", + "description": "MariaDB <-> PostgreSQL parity rules for ERPNext: query constructs that error on PostgreSQL or silently diverge across the two engines, the GROUP BY row-count trap, the false positives not to flag, and the rule that MariaDB output must not change. Apply to every changed database query in this PR." + } + ] } }