diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md index 675e47d62e7..4cc33740441 100644 --- a/.github/POSTGRES_COMPATIBILITY.md +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -31,9 +31,12 @@ 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".** + 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 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`, @@ -47,12 +50,23 @@ Flag a changed query that uses any of these: 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. +- **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 named `status`/`account` won'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)` emits `SET col = true`, which PostgreSQL rejects on a `smallint` - (`DatatypeMismatch`). Pass `1`/`0`. + check_field, True)`, `doc.db_set(field, False)`, or `frappe.qb.update(dt).set(check_field, True)` + emit `SET col = true`, which PostgreSQL rejects on a `smallint`/`Check` column + (`column is of type smallint but expression is of type boolean`). Pass `1`/`0`. +- **`.like()`/`.ilike()` (or raw `LIKE`) on a NON-text column** — `idx`, `docstatus`, a date, etc. + frappe maps `.like()` → `ILIKE`, and PostgreSQL has no `bigint ILIKE text` operator (`operator + does not exist: bigint ~~* unknown`). Cast the column to text first — **`Cast_(col, "varchar")`**, + not `Cast(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 bare `CHAR` is `character(1)`, so + `CAST(12 AS CHAR)` → `'1'` (silently truncates multi-digit values); MariaDB gives the full string. + Use `VARCHAR` / `Cast_(x, "varchar")`. +- **`.rlike()` / raw `RLIKE`** — frappe rewrites `REGEXP` → `~*` on PostgreSQL but does **not** + translate `RLIKE` (no such PostgreSQL operator). Use `.regexp()` (or `.like()` for a simple prefix). --- @@ -75,8 +89,11 @@ These don't error, so a one-engine CI stays green. Flag them: - **`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. +- **Integer division** — `int / int` truncates on PostgreSQL but is decimal on MariaDB, e.g. + `COUNT(...) / COUNT(...) * 100` → `0`, or `manufacturing_time_in_mins / 1440` flooring a + lead-time to whole days. Force float: multiply by `100.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.) - **`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 @@ -119,9 +136,12 @@ scope for a portability fix. These are auto-handled by the framework and are **not** breaks: - **`.like()` / `["like", …]`** already renders as `ILIKE` on PostgreSQL — not a - case-sensitivity bug. + case-sensitivity bug. *(Exception: `.like()` on a **non-text** column — `idx`, `docstatus` — + is a hard break, `bigint ILIKE`; see §1.)* - **Raw `ifnull(...)`** inside `frappe.db.sql()` is rewritten to `coalesce(...)` on all engines. -- **Backticks**, **`LOCATE`**, **`REGEXP`** in raw SQL are auto-translated on PostgreSQL. +- **Backticks**, **`LOCATE`**, **`REGEXP`** / **`.regexp()`** in raw SQL are auto-translated on + PostgreSQL (`REGEXP` → `~*`). **But `RLIKE` / `.rlike()` is NOT translated** — that one is a + hard break (see §1). - **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. diff --git a/.github/helper/postgres_compat.py b/.github/helper/postgres_compat.py index 273c4218193..969208f72ab 100755 --- a/.github/helper/postgres_compat.py +++ b/.github/helper/postgres_compat.py @@ -62,6 +62,10 @@ SQL_PATTERNS: list[tuple[re.Pattern, str]] = [ "single-quoted column alias breaks on Postgres -> use a bare or double-quoted alias"), (re.compile(r"\bif\s*\(", re.I), "SQL IF() is MySQL-only -> use CASE WHEN ... THEN ... ELSE ... END (frappe.qb.Case())"), + (re.compile(r"\brlike\b", re.I), + "RLIKE is MySQL-only -> frappe rewrites REGEXP->~* on Postgres but NOT RLIKE; use REGEXP / .regexp() / ~"), + (re.compile(r"\bcast\s*\([^)]*\bas\s+char\b", re.I), + "CAST(... AS CHAR) is character(1) on Postgres and truncates -> CAST AS VARCHAR (frappe Cast_(x, 'varchar'))"), ] # UPDATE ... JOIN: both keywords in the same SQL string. @@ -71,6 +75,10 @@ MYSQL_RESULT_KEYS = {"Column_name", "Key_name", "Seq_in_index", "Non_unique", "I SET_BOOL_FUNCS = {"set_value", "db_set"} +# query-builder cast helpers: pypika Cast / frappe Cast_. A "char" target type is character(1) +# on Postgres (truncates); "varchar" is the full-length cast. +CAST_FUNCS = {"Cast", "Cast_"} + # frappe.get_all / get_list: frappe's db_query SILENTLY drops ORDER BY for `distinct` queries on # Postgres (the ORDER BY column must appear in the SELECT-DISTINCT list), so `distinct=True` together # with a literal `order_by` is a no-op on PG and the result comes back unordered. @@ -178,6 +186,18 @@ class Visitor(ast.NodeVisitor): if has_distinct and has_literal_order: self._flag(node, f"{name}(distinct=True, order_by=...) -> frappe drops ORDER BY for distinct queries on Postgres; sort in python instead, e.g. sorted(..., key=str.casefold)") + # query-builder .rlike(...): pypika emits the MySQL-only RLIKE operator, which frappe does + # NOT translate for Postgres (it rewrites only REGEXP -> ~*). + if name == "rlike": + self._flag(node, ".rlike() emits MySQL-only RLIKE (not translated on Postgres) -> use .regexp() (rewritten to ~*) or .like()") + + # Cast(col, "char") / Cast_(col, "char"): on Postgres a bare CHAR is character(1) and truncates + # (e.g. CAST(12 AS CHAR) -> '1'); use "varchar" for a full-length string cast. + if name in CAST_FUNCS: + for arg in (*node.args, *(kw.value for kw in node.keywords)): + if isinstance(arg, ast.Constant) and isinstance(arg.value, str) and arg.value.strip().lower() == "char": + self._flag(node, f"{name}(..., 'char') is character(1) on Postgres and truncates -> use 'varchar'") + self.generic_visit(node) def visit_Subscript(self, node: ast.Subscript) -> None: diff --git a/.greptile/config.json b/.greptile/config.json index 62a5b58e7e3..ba7d630dbb8 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), 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.", + "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), or a Python bool written to a Check/int column via set_value/db_set/qb.update().set() 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 (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.", "customContext": { "files": [ {