schema: review-report-v2
target: "CommitFest 6437 — LLVMJIT: introduce force-inlined functions"
source: postgresql-master
slug: commitfest-6437-review
base: 6e5d5680b555523a3d5c547aa22df0267860344a
head: 6e5d5680b555523a3d5c547aa22df0267860344a
diff_sha256: 6f4ded1e1f4bf59fdf22b3337cd3cab68b451306e1e192ebe073b5f4719c375c
verdict: needs-review
blocker_counts:
  verified: 0
  unverified: 0
model: glm-5.2:cloud
review_date: 2026-08-28

CommitFest 6437 — Review Report

Patch: LLVMJIT: introduce force-inlined functions File: src/backend/jit/llvm/llvmjit_expr.c (+103/-4, uncommitted staged diff) Author: p.psql@pinaraf.info (pgsql-hackers thread 635040, 2026-01-26) Reviewer seats: pg-patch-reviewer, pg-security-reviewer, pg-test-author, pg-finding-verifier (all via Amauta handoff protocol) Verdict: needs-review — two MAJOR correctness findings (F1, F2) and one MAJOR tests finding (F3) must be addressed before commit. No blockers.

Deterministic Verdict

Rule applied: no surviving BLOCKER ⇒ not waiting-on-author; correctness MAJORs present (F1, F2) ⇒ needs-review (a softer sub-verdict cannot override a harder finding).

Summary

The patch adds a static helper llvm_try_inline_function_call() to the LLVM JIT expression compiler that manually inlines a small allowlist of strict, leakproof, two-argument comparison operators (int4eq, int8eq, date_lt, date_gt, date_le, date_ge) by emitting a direct LLVMBuildICmp instead of the generic BuildV1Call out-call. The inlining is gated on fcinfo->flinfo->fn_addr pointer equality and is only reached after the strict-argument NULL-check block (b_nonull). The author reports a 3–5% improvement on 1M dateeq calls.

The patch compiles cleanly with gcc + LLVM 21 (the referenced C functions are declared via the generated src/include/utils/fmgrprotos.h, included transitively through utils/builtins.h). The inlined IR semantics are correct for the current allowlist: int4/DateADT are int32 byval (truncation i64→i32 recovers the int32; signed LLVMIntSLT/SGT/SLE/SGE match the C comparisons, including the date infinity sentinels at INT32_MIN/MAX), int8 is int64 byval (full-width LLVMIntEQ), and resnull=false is correct on the strict path where args are guaranteed non-null.

The concerns are: (F1) the inlined path drops the fcinfo->isnull = false invariant that both BuildV1Call and the interpreter maintain; (F2) the matcher does not validate the strictness/leakproof/byval/nargs properties that make inlining legal, with no guard against drift; and (F3) the new path has no test coverage.

Findings

Verification Summary

All 8 findings verified (none refuted, none cannot-verify). Each verified finding carries a concrete suggested code example and a HEAD-bound unified diff. Six code findings (F1, F2, F4, F5, F6, F7) are addressed by one combined refactor patch validated in disposable scratch (apply-check=0, diff-check valid, compile COMPILE_EXIT=0 with NO warnings, gcc + LLVM 21, exact makefile flags). F3 is the test-author's TAP test (proposal-only; perl -c syntax OK). F8 is a docs patch (apply/static-check passed; full doc-build not available in this environment).

Repair Validation Summary

• Combined C patch (repair_combined.diff, sha256

904c6fabb5b9b142614fdcd0ebfc407f1f4e748def3a956023dcb9e86a9e1370): HEAD-bound unified diff on src/backend/jit/llvm/llvmjit_expr.c. Subsumes the original staged diff and adds the F1 isnull reset, the F2 strict-opcode call-site guard + doc comment, the F4 symmetric allowlist, the F5 PG-style doc comment, the F6 EMIT_INT_COMPARE/LOAD_ARG factoring, and the F7 l_funcvalue/TypeDatum usage. Validated: apply-check=0, apply=0, diff-check valid, compile COMPILE_EXIT=0 with NO warnings (gcc + LLVM 21, exact makefile flags captured via make -n llvmjit_expr.o).

• F3 TAP test (proposal-only; sha256

41538e8ff5e71e2423b95c372c43e551f92b41bc1134b4e3184c3fa7697689d8): perl -c syntax OK; make -C src/test/jit check not-run (read-only worktree; applying remains user-controlled).

• F8 docs patch (sha256

a40eec68f6c138a5c6063f78601fab292b722d0a302e0c60095061a26bc34832): apply-check=0, apply=0, static SGML well-formedness check passed; full doc-build (xsltproc) not available.

Security

No memory-safety, privilege-escalation, SQL-injection, or authorization-bypass vulnerabilities found. The emitted LLVM IR is type-consistent and in-bounds (l_funcvaluep GEPs to the n'th NullableDatum slot; all operators have nargs==2; loads/stores match Datum/TypeStorageBool widths). The fn_addr pointer is read once at JIT-compile time and is immutable (no TOCTOU). The one security-adjacent concern (F2 / SEC-1) — unconditional resnull=false relying on strictness — is not currently exploitable because the executor never emits the non-strict EEOP_FUNCEXPR opcode for these strict operators; it is a latent footgun hardened by the F2 call-site guard.

Test-Gap Section

The new inlined path is reached only when JIT compiles the expression (jit=on + cost above threshold). The standard regression suite does not force JIT (jit=off in explain.sql; jit_above_cost default 100000), so the inlined operators' results are effectively untested. The proposed TAP test (F3) closes this gap by comparing jit=on vs jit=off over edge values and NULLs. A mutation-differential (swap LLVMIntEQ→LLVMIntNE and confirm int4eq parity fails) is recommended to prove the net covers the inlined path.

pgsql-hackers Thread Ledger

Status: reconciled. One thread (Thread-ID 635040, subject "LLVMJIT: introduce force-inlined functions"). Ledger report slug: commitfest-6437-review-thread-ledger.

• 2026-01-26 p.psql@pinaraf.info (patch): initial submission; reports 3–5%

gain on 1M dateeq calls; announces a v2 with more functions.

• 2026-03-23 matheusssilv97@gmail.com (review): asks for planning-time

comparison; asks whether a v2 is planned; requests a pgindent pass.

Per-finding dispositions: F4 = author-deferred (v2 with more functions announced); F5 = already-raised (pgindent requested); F1/F2/F3/F6/F7/F8 = new. No committer reply; no v2 in the archive as of the review date.

Per-commit rollup

Single uncommitted staged diff on src/backend/jit/llvm/llvmjit_expr.c (+103/-4). No commit history to roll up.

Verified-clean section

The following non-findings were explicitly verified clean (not findings):

• int4eq truncation i64→i32 + LLVMIntEQ — correct (int32 byval in low 4 bytes).
• int8eq full i64 compare — correct (int64 byval).
• date_* signed predicates on i32 — correct (DateADT=int32; infinities at INT32_MIN/MAX map correctly).
• resnull=false on the strict path — correct (args guaranteed non-null in b_nonull).
• v_fcinfo_isnull not set on inlined path — correct by construction (only read by the immediate fallback store).
• Compilation — make llvmjit_expr.o succeeds (gcc + LLVM 21); &int4eq etc. resolve via generated fmgrprotos.h.

Aggregated Test Plan

1. F3 TAP test (src/test/jit/t/001_jit_inline_ops.pl): apply the four

proposed files + two integration edits, build --with-llvm, run make -C src/test/jit check. Expected: PASS on base (oracle holds via BuildV1Call), PASS on HEAD (inlined path matches interpreter).

1. Mutation-differential (recommended, optional): swap

LLVMIntEQ→LLVMIntNE in the int4eq branch and confirm int4 = (int4eq) parity FAILS while jit=off still returns correct results — proves the net covers the inlined IR.

1. Combined C patch: compile-check passes; full behavior parity asserted by

the F3 TAP test oracle.

Changed-Code Coverage

Coverage requirement: required (one changed instrumentable path: src/backend/jit/llvm/llvmjit_expr.c). Status: measured (fresh disposable counter epoch).

Execution lane: host-disposable (passed). A fresh --enable-coverage --enable-cassert --with-llvm build was produced in disposable scratch; PostgreSQL started from the install bin; the 243-test standard regression suite passed (make check), plus 7 JIT-forcing queries (jit_above_cost=0, jit_inline_above_cost=0) over int4/int8/date covering all six inlined operators (EXPLAIN confirmed JIT: Functions N, Expressions true). The server was stopped to flush .gcda; gcovr 8.6 consumed the gcno/gcda with integrity checks. integrity_errors: [].

Touched functions:

• llvm_try_inline_function_call (lines 75–169): entered; whole-function lines

73/73 covered; whole-function branches 12/12 covered.

• llvm_compile_expr (lines 175–3075, contains the call site at 838–844):

entered; whole-function lines 261/997; whole-function branches 67/254 (the changed call-site lines 838–844 are all covered: 32 inline / 9 fallback).

Uncovered regions: none. All 77 executable changed lines and all 14 changed branches are covered. The 26 changed lines without coverage coordinates are bounded structural diagnostics — comments (/* load arg 1 */, /* compare and convert result */), declarations (LLVMValueRef v_fcinfo, v_retval;), braces ({/}), and one continuation line (&v_fcinfo_isnull);) — for which gcc emits no coverage coordinate. These are non-actionable structural exclusions, not executable uncovered code.

Non-instrumented changes: none.

Residual limitations: The coverage is qualified by the executed test set (standard regress + 7 forced-JIT queries). The proposed F3 TAP test, once applied, would provide a deterministic, repeatable JIT-path oracle; the coverage measurement here used ad-hoc forced-JIT queries because the TAP test is proposal-only.

Provenance / model routes

• Orchestrator: pg-review-harness (glm-5.2:cloud, user override).
• pg-patch-reviewer: glm-5.2:cloud (handoff slug

commitfest-6437-review-handoff-patch-reviewer, sha256 fb86946e18689f475bf30488377a393765ac1ecc3116f40668ccaea2649bace7).

• pg-security-reviewer: glm-5.2:cloud (handoff slug

commitfest-6437-review-handoff-security-reviewer, sha256 e16eae84ae4b7a2bb33ed6a5066eec0265fb87303fb70451989002fa53e741a7).

• pg-test-author: glm-5.2:cloud (handoff slug

commitfest-6437-review-handoff-test-author, sha256 1d4279c89b48a3fc80156ed0318f9cc392b8cc5ed179c407157bd01d17d15a03).

• pg-finding-verifier: glm-5.2:cloud (handoff slug

commitfest-6437-review-handoff-finding-verifier, sha256 5248212d017e21baaffb328592ab0116fde92b71635f5a3126f71f0c01c1fbb3).

• Router policy: recommend_model per seat (sonnet-5/opus-4.8 baseline; user

override glm-5.2:cloud honored for all seats).

• Coverage: measured by the orchestrator in a host-disposable lane (gcc + LLVM

21, gcovr 8.6).

Residual risk

The patch is correct for the current allowlist and current executor usage. The residual risk is the latent fcinfo->isnull invariant (F1) and the unguarded matcher (F2), both cheaply fixed by the combined patch, plus the absence of a regression net (F3) that would catch future IR mistakes. With F1, F2, and F3 addressed the patch is committer-ready.

Maintainer CC

Andres Freund andres@anarazel.de — JIT/LLVM area owner (natural reviewer for src/backend/jit/llvm/). The patch thread (635040) is on pgsql-hackers.

Artifact

The validated pg-review-artifact-v2 JSON is at the run scratch artifact path (review-artifact-v2.json, sha256 b6168e997df4e2ddbb7502f966f7e1946f433180f9fcd0863bdb37b90c000adf); it passed the supplied validator (validate_review_artifact.py, digest edbcb304ffbd062627196c1ee0ce3d9b60cae80dd7b201e420335fea0a31979e, exit 0).