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.
17 KiB
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:
- Hard breaks — PostgreSQL raises an exception; MariaDB is green. Easy to catch in CI, but the gated job may not run.
- 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 inGROUP BYnor wrapped in an aggregate. MariaDB tolerates it; PostgreSQL errors (must appear in the GROUP BY clause or be used in an aggregate function). This also covers an aggregate (Sum/Count/…) selected alongside bare columns with NO.groupby()at all — MariaDB silently collapses every row into one arbitrary-valued row (often a wrong-output bug there too), PostgreSQL errors. Fix: add the bare column toGROUP BYif it is functionally dependent on the group key, otherwise wrap it inMax()/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, SQLIF(cond,a,b). Use the portablefrappe.query_builder.functionsequivalents (CombineDatetime,DateDiff,Case,GroupConcat, …) or a precomputed column (e.g.posting_datetime). UPDATE … JOIN— not valid on PostgreSQL. Rewrite asUPDATE … WHERE name IN (subquery).HAVINGreferencing aSELECTalias — PostgreSQL rejects output-column aliases inHAVING(regardless of whether the query has aGROUP BY; MariaDB allows them). Repeat the underlying expression inHAVING, or move a non-aggregate predicate intoWHERE.SELECT DISTINCT … ORDER BY <expr not in the select list>— add the expr to the select only if it is single-valued per distinct row; otherwise it grows theDISTINCTkey and the MariaDB row count (see §3) — drop the SQLORDER BYand 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. UseCoalesce(...).- Capital-cased identifiers used as column/field names in
get_value(dt, dn, "Status"),get_all(dt, fields=["Account"]), and similar — PostgreSQL quotes the identifier and matches it case-sensitively; a stored column namedstatus/accountwon't match"Status"/"Account"(column "Account" does not exist). Use the exact stored (lower-case) fieldname. - Boolean passed where an integer column is expected —
frappe.db.set_value(dt, dn, check_field, True),doc.db_set(field, False), orfrappe.qb.update(dt).set(check_field, True)emitSET col = true, which PostgreSQL rejects on asmallint/Checkcolumn (column is of type smallint but expression is of type boolean). Pass1/0. .like()/.ilike()(or rawLIKE) on a NON-text column —idx,docstatus, a date, etc. frappe maps.like()→ILIKE, and PostgreSQL has nobigint ILIKE textoperator (operator does not exist: bigint ~~* unknown). Cast the column to text first —Cast_(col, "varchar"), notCast(col, "char")(see below). MariaDB coerces the int implicitly, so the cast is a no-op there.CAST(… AS CHAR)/Cast(x, "char")— on PostgreSQL bareCHARischaracter(1), soCAST(12 AS CHAR)→'1'(silently truncates multi-digit values); MariaDB gives the full string. UseVARCHAR/Cast_(x, "varchar")..rlike()/ rawRLIKE— frappe rewritesREGEXP→~*on PostgreSQL but does not translateRLIKE(no such PostgreSQL operator). Use.regexp()(or.like()for a simple prefix).IfNull/Coalesceof a typed column with a different-typed literal —IfNull(asset.disposal_date, 0)rendersCOALESCE("disposal_date", 0), coalescing a DATE with an integer. PostgreSQL requiresCOALESCEargs to share a type (DatatypeMismatch: COALESCE types date and integer cannot be matched); MariaDB'sIFNULLis permissive. The common shape isIfNull(date_col, 0) != 0 / == 0as a presence test — replace withdate_col.isnotnull()/date_col.isnull()(identical, and valid on both). Otherwise coalesce to a same-type default (Coalesce(date_col, '1900-01-01'),Coalesce(text_col, '')).- Division by a possibly-zero divisor —
Sum(a) / Sum(b),x / col, etc. where the divisor can be0/empty. MariaDB returnsNULLfor division by zero; PostgreSQL raisesdivision by zeroand aborts the query. Wrap the divisor inNullIf(divisor, 0)— that yieldsNULLon both engines, matching MariaDB's value. (Only the literal/ 0is a parse constant; the trap is a divisor that is an aggregate or column the data can drive to zero.)
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/Locateon 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 asILIKEon PostgreSQL; see §4.) - Case sensitivity in a doc-
namelookup — lower-casing a value then using it as a document name inget_value/get_doc/existsmisses 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
NULLon some paths while MariaDB keeps'';Concat/Concat_wsthen diverge. Prefer the stored full value, orCoalesce(col, '')per argument. - NULL ordering — MariaDB sorts
NULLfirst, PostgreSQL sorts it last. ForORDER BY … LIMIT 1/[0]on a nullable column, guard withCoalesce/isnotnull(). ORDER BY … LIMIT 1with no unique tiebreaker — when rows tie on the ordered column the two engines may pick different rows. Add acreation/nametiebreaker only if it does not change MariaDB's current pick (see §4).- Integer division —
int / inttruncates on PostgreSQL but is decimal on MariaDB, e.g.COUNT(...) / COUNT(...) * 100→0, ormanufacturing_time_in_mins / 1440flooring a lead-time to whole days. Force float: multiply by100.0, or make a literal a float (/ 1440→/ 1440.0), or cast an operand. (Only SQL-level/on integer columns/literals — Python/is already float.) DISTINCTlist ordering —frappe.get_all(distinct=True, order_by=…)/SELECT DISTINCT … ORDER BY: frappe'sdb_querysilently dropsORDER BYfor distinct queries on PostgreSQL, so the result is unordered there. Sort in Python instead — and usekey=str.casefold, because baresorted()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_replacebranch reimplementing MariaDB'sCAST(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 strictepoch <= nowbound is flaky on PostgreSQL.
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
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.xwheret1.key = t3.name) is FD → safe to keep inGROUP 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.
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.
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 independentMax()/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/MINignore NULLs, soMax()over a mostly-NULL discriminator (anoriginal_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)whereyvaries within the group invents a number no row ever had (andMaxbiases 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 blindMaxcan 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
These are auto-handled by the framework and are not breaks:
.like()/["like", …]already renders asILIKEon PostgreSQL — not a case-sensitivity bug. (Exception:.like()on a non-text column —idx,docstatus— is a hard break,bigint ILIKE; see §1.)- Raw
ifnull(...)insidefrappe.db.sql()is rewritten tocoalesce(...)on all engines. - Backticks,
LOCATE,REGEXP/.regexp()in raw SQL are auto-translated on PostgreSQL (REGEXP→~*). ButRLIKE/.rlike()is NOT translated — that one is a hard break (see §1). - An
ORDER BY … LIMIT 1tie 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 withInFailedSqlTransaction(frappe dropped its blanket per-statement savepoint in frappe#40075). Such a handler must wrap the fallible insert infrappe.db.savepoint(name)+rollback(save_point=name)— unless it re-throws with no DB call before the throw, or the insert usesignore_if_duplicate=True/autoname="hash"(→ON CONFLICT DO NOTHING). - Recover the txn with a scoped savepoint, not a full
frappe.db.rollback(), if any prior work must survive. A full rollback un-poisons the txn but also discards every row the handler committed before the failure — which MariaDB kept (it has no statement-abort), so it's a silent MariaDB regression. "The background job / whitelist entrypoint owns the txn" does NOT make a full rollback safe if it did multiple inserts in a loop first — it drops the partial results MariaDB retained. A full rollback is safe only when it (a) immediately re-throws/raises (MariaDB rolls back anyway), (b) has nothing successful before it (a single op), or (c) the batch is genuinely meant to be atomic (a partial result is an invalid state → rollback + mark Failed is correct). Otherwise use a per-iteration / per-record savepoint — and keep the function's success/Nonereturn contract: do not return the doc when the savepoint was rolled back.
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), (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.
The static pre-commit checker (.github/helper/postgres_compat.py) catches the mechanical
§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.