Max()/Min() over a text column is a sort, and the engines sort text
differently: MariaDB's utf8mb4 collations fold case, PostgreSQL as CI runs it
orders by byte value. MAX('abc','ABD') is 'ABD' on MariaDB and 'abc' on
PostgreSQL, confirmed on CI in the probe attached to #56241.
That makes a Max() over a text column which varies in case within its group a
live parity gap, rather than the arbitrary-pick preservation the wrap is
usually justified as. Where the column is functionally dependent on the group
key it stays a genuine no-op and collation cannot matter, so the rule is
scoped to non-FD columns to keep it a high-precision signal.
Recorded as a fifth second-order trap in the guide and in the Greptile
instructions, including the trap that a local macOS PostgreSQL agrees with
MariaDB here and reports a false all-clear.
GITHUB_REF is already the fully qualified ref for both branch and tag events,
so reconstructing refs/heads/$GITHUB_REF_NAME and refs/tags/$GITHUB_REF_NAME
just risks the two drifting apart. Keep the type check, since it still decides
whether the develop fallback applies, and take the ref verbatim.
The probe used --heads with a bare name, so it could not describe a tag push
and would have fallen back to develop for one. Resolve a fully qualified ref
from the event instead: the PR base or pushed branch under refs/heads, a tag
under refs/tags, and fail loudly on an unrecognised ref type.
Only branch refs are eligible for the develop fallback. A tag that is absent
from frappe is a real error, not a stacked-PR base, so it still fails.
The previous `||` treated every fetch failure as a missing branch, so a
transient network or auth error on a base that does exist in frappe would
silently substitute develop and report Patch Test results against the wrong
revision.
Probe with `ls-remote --exit-code` instead: exit 2 means no matching ref, so
fall back; any other non-zero status is a real failure and is re-raised.
The Patch Test fetches the frappe repo using this erpnext PR's base branch
name. For an ordinary PR that is develop, which exists in frappe/frappe. For a
stacked PR the base is an erpnext feature branch with no counterpart there, so
the fetch fails and the step exits 128 before any patch runs:
fatal: couldn't find remote ref pg-audit/bom-amount-per-line
This affects every stacked PR. It has been latent rather than absent: earlier
stacks passed only because their Patch Test ran while they still targeted
develop, before being retargeted onto the layer below.
Fall back to develop when the base ref does not resolve. Ordinary PRs and
version-branch PRs are unaffected -- their base exists in frappe, so the first
fetch succeeds and the fallback never runs.
* ci: fall back to develop when frappe has no matching branch
The frappe branch to install is taken from the pull request's base branch. A
stacked pull request targets another erpnext branch, so the clone fails with
"couldn't find remote ref", no bench is installed, and every job that needs one
fails with it.
Fall back to develop when the base branch does not exist in frappe. An explicit
FRAPPE_BRANCH is left alone, since it can be a commit sha rather than a branch.
* ci: only fall back when frappe is known to lack the branch
git ls-remote --exit-code reports 2 for a branch that is not there and 128 for a
remote it could not reach. Treating both as absence let a transient network or
DNS failure install develop over the branch the pull request was built against.
Fall back on 2 alone and log anything else, so a flaky probe leaves the branch
as it was.
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.
Both server-test workflows now name the test job "Python Unit Tests" so the
check appears under the same name regardless of engine.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror frappe/erpnext#56655 for the Postgres CI. Run the
bootstrap_test_data module in the setup job while Postgres is still up, so
the BootStrapTestData records are baked into the PGDATA artifact every shard
hydrates from — the shards start on already-warmed data instead of each
building it.
Unlike the MariaDB step, no `su -m` wrapper: the Postgres CI is
GitHub-hosted ubuntu-latest running as the runner user directly, matching
its own "Run Tests" step.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror the config.json guidance in POSTGRES_COMPATIBILITY.md: when scoping a rollback, keep the function's
success/None return contract -- don't return the doc that was just rolled back. (greptile #56688)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Recovering a poisoned Postgres txn with a full frappe.db.rollback() discards rows the handler already
created before the failure -- which MariaDB keeps (no statement-abort) -- so it's a silent MariaDB
regression. 'Owns the txn' does not make a full rollback safe in a loop handler. Document the safe cases
(re-raise / single op / atomic batch) and the per-iteration/per-record savepoint alternative.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Greptile flagged that `gh release download` with `github.token` could be
rejected for fork pull requests (token scoped to the fork, asset in
frappe/erpnext). The release is public and published, so the asset is
downloadable anonymously from objects.githubusercontent.com — drop the token
and curl the public URL directly. Removes the cross-repo token dependency and
keeps fork PRs working. Cloudflare is still bypassed since GitHub serves the
asset, not frappe.io.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "Stop DB and stage datadir" step swallowed a failed `pg_ctl -m fast -w
stop` with `|| true`, then moved and tarred the PGDATA regardless. A stop
that times out or errors would bake a still-running, crash-inconsistent
cluster into the artifact every test shard consumes — and with
full_page_writes off, crash recovery can't repair torn pages. Drop the
`|| true` so a failed stop fails the job, mirroring the MariaDB sister's
"don't bake a dirty datadir" guard.
Also drop the redundant `ALTER SYSTEM SET fsync/synchronous_commit/
full_page_writes = off` block from install.sh. Its comment claimed the
postgres workflow "runs a service-container DB and never calls start-db.sh",
but it does call start-db.sh, which already applies those flags via `-o` on
every postgres start (setup job and each shard). The block was a no-op and
its justification was factually wrong.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Patch Test job intermittently failed on the "Download erpnext v14 backup"
step with HTTP 403 Forbidden: frappe.io sits behind Cloudflare, and wget's
default User-Agent gets flagged by bot protection on cache misses. This caused
random failures across PRs that only a re-run would clear.
Pull the fixed baseline from the v14-baseline GitHub release using the built-in
token instead. Release assets are served from GitHub's CDN and authenticated
from the runner, so no rate-limit roulette.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Measured A/B on the self-hosted setup: warm-bench restore 85s vs full
bench init 82s — no gain (slightly slower). bench init is already fast
because the uv/pip caches are mounted warm, so the cache only replaces a
~13s init while adding a ~200MB untar and still running bench build.
Drop BENCH_CACHE_DIR so warm-bench stays inert (the helper functions
remain, matching develop's install.sh).
- Wire up warm-bench: set BENCH_CACHE_DIR on the setup job so the bench-base
cache actually activates (was inert with no dir set, so every run did a
full bench init). Lives on the node-local bench-staging hostPath; any
miss/failure still falls back to a full init.
- run_ci_step: capture the timeout exit code under `set -e` (the previous
`timeout ...` + `ec=$?` aborted at the timeout line on failure, skipping
::endgroup:: and the exit-code return).
- Raise the per-step timeout 600s -> 1800s so a contended reinstall isn't
killed before the 40-min job timeout.
- Propagate DB through the su re-exec in start-db.sh / hydrate.sh so a
DB=postgres invocation can't silently fall back to the mariadb branch.
- Simplify the coverage job `if` to the equivalent plain non-PR gate.
The fan-out moved fsync/synchronous_commit/full_page_writes=off into
start-db.sh startup flags, but the Postgres workflow runs a postgres:13.3
service container and calls install.sh directly — it never runs start-db.sh.
So those flags never reached the Postgres CI, regressing it to full durability
on a commit-heavy suite. Re-apply them via ALTER SYSTEM (reloadable) in the
DB == "postgres" path, where the service-container workflow executes. MariaDB
is unaffected (DB != postgres).
Remove the `|| 'develop'` fallback on FRAPPE_BRANCH so install.sh resolves the
frappe framework branch from the git context (GITHUB_BASE_REF/GITHUB_REF), the
same as the Postgres workflow. This makes the workflow backportable unchanged:
on version-15-hotfix / version-16-hotfix it now clones the matching frappe
branch instead of develop.
Adds the division-by-zero divergence class to the PG-compat review tooling:
on a divisor that the data can drive to 0 (e.g. Sum(a)/Sum(b)), MariaDB
returns NULL for division by zero while PostgreSQL raises `division by zero`
and aborts the query. The portable fix is to wrap the divisor in
NullIf(divisor, 0), which yields NULL on both engines (matching MariaDB).
- .greptile/config.json: add it to the "would ERROR on PostgreSQL" list.
- .github/POSTGRES_COMPATIBILITY.md: document it under §1 (hard breaks).
- .github/helper/postgres_compat.py: note it in the docstring as a
deliberately-not-statically-checked semantic divergence (data-dependent,
like integer-division intent), so it stays a reviewer/Greptile concern.
Tooling-only; no source query changes. The instance fix shipped in #56361.
ci(postgres): teach the guard about COALESCE(date, int) type mismatch
New class found by the whole-repo audit (the Asset Depreciations report fix in this PR): IfNull/Coalesce of a typed column with a different-typed literal -- e.g. IfNull(date_col, 0) -> COALESCE(date, integer), which PostgreSQL rejects (DatatypeMismatch). Added to the Greptile config and POSTGRES_COMPATIBILITY.md (not statically checkable without column types).
The [^)]* span stopped at the first inner ')', so CAST(ABS(col) AS CHAR) slipped through. Use a non-greedy .+? with re.S; still zero production false positives (verified). Addresses review feedback.
The whole-repo MariaDB<->PostgreSQL audits surfaced classes the checker and
review guide did not yet cover. Add them:
Static checker (.github/helper/postgres_compat.py) - new mechanical breaks:
- .rlike() / raw RLIKE: frappe rewrites REGEXP->~* on Postgres but NOT RLIKE.
- Cast(x, "char") / raw CAST AS CHAR: bare CHAR is character(1) on Postgres
and truncates multi-digit values; use "varchar".
(Both flag zero production code; the only repo hit is in patches/, which the
hook already excludes.)
Greptile config + POSTGRES_COMPATIBILITY.md - new semantic/hard classes:
- aggregate (Sum/Count) selected next to bare columns with no GROUP BY at all.
- .like()/LIKE on a non-text column (bigint ILIKE) -> Cast_ to varchar.
- get_all(fields=["CapitalCase"]) identifier-case (extends the get_value case).
- bool into a Check column via qb.update().set() (extends set_value/db_set).
- int/int division: float a literal (col/1440 -> col/1440.0).
- Concat over a nullable column leaking a bare prefix on Postgres.
- clarify REGEXP/.regexp() is translated but RLIKE/.rlike() is not.
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 `get_all/get_list(distinct=True,
order_by="<col>")` is a no-op there and the result comes back unordered — the root cause of
the Sales Register, Purchase Register and Sales Analytics ordering fixes. Add an AST rule to
.github/helper/postgres_compat.py that flags this (literal order_by only; an empty order_by=""
suppression and a dynamic/variable order_by are not flagged). `# pg-ok` escape hatch as usual.
Grandfather the three pre-existing low-impact sites the rule surfaces (paging/iteration order
only, not data): job_card operation autocomplete, inventory_dimension config list, and a
work_order test loop.
The Patch Test starts the full bench (incl. workers) and then runs migrate.
Migrate enqueues orphan-link cleanup jobs (delete_dynamic_links) that the
workers pick up and process while migrate is altering tables, which
intermittently fails with MySQL 1412 'Table definition has changed, please
retry transaction'.
Start every bench process except the workers during migrate, so nothing
consumes the queue mid-migrate. Redis and the other services stay up; the
queued jobs just wait.
