mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-12 06:01:46 +00:00
ci(greptile): teach the review bot to enforce MariaDB↔PostgreSQL parity
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.
This commit is contained in:
Mihir Kandoi
151
.github/POSTGRES_COMPATIBILITY.md
vendored
Normal file
151
.github/POSTGRES_COMPATIBILITY.md
vendored
Normal file
@@ -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 <expr not in the select list>`** — 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.
|
||||
Reference in New Issue
Block a user