CommitFest 7244 Review: effective_cpu_cache_size

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-7244-review

Tags

Updated
2026-09-04T01:31:52.372150+00:00

Verdict / Summarywaiting-on-author

Front matter

schema: review-report-v2
target: CommitFest 7244 — effective_cpu_cache_size GUC for hash-aggregation cache-miss cost
base: 2b3d11aaed891958f24aed05bf454524d603b6ea
head: 2b3d11aaed891958f24aed05bf454524d603b6ea
diff_blob: f7c375fdf5842eebbd16a14fef93bfa7c32614a6
scope_identity: diff-f7c375fd
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
model: glm-5.2:cloud
seats:
  - pg-patch-reviewer
  - pg-security-reviewer
  - pg-finding-verifier
  - pg-change-coverage-reviewer (coordinator-executed)
date: 2026-09-04

CommitFest 7244 Review — `effective_cpu_cache_size`

Summary

The patch introduces a new planner cost GUC effective_cpu_cache_size (type int,

MAX_KILOBYTES). When aggstrategy == AGG_HASHED || AGG_MIXED and the GUC is > 0, cost_agg computes the hash-table resident size and charges an extra cpu_tuple_cost * HASH_RESIDENCY_PENALTY * LOG2(table_bytes / cache_bytes) * input_tuples when the table exceeds the configured cache size. HASH_RESIDENCY_PENALTY = 4.0.

The core idea is reasonable and the plumbing is clean (reuses hash_agg_entry_size/hash_agg_set_limits from nodeAgg.c, GUC in the cost-tuning group next to effective_cache_size). However, the patch should not be committed until the stale AGG_SORTED == AGG_HASHED invariant comment is reconciled and the new behavior is demonstrated with tests.

Verdict: waiting-on-author — 1 verified BLOCKER (stale comment + plan-instability risk), 1 verified MAJOR (no tests). 2 findings refuted (AGG_MIXED double-charge, LOG2 Inf).

Findings

PATCH-1

— BLOCKER (correctness/maintainability) — verified

Location: src/backend/optimizer/path/costsize.c:2874

The cost_agg block comment still states *"AGG_SORTED and AGG_HASHED have exactly the same total CPU cost"* and instructs maintainers to *"form the same intermediate values in the same order"*. The patch adds hash_entry_cost (nonzero only for AGG_HASHED/AGG_MIXED) to both strategy branches, breaking the asserted equality. While the patch added an explanatory paragraph at costsize.c:2870-2872, it left the contradicting invariant sentence standing.

Evidence (executed): Live EXPLAIN on the feature build confirmed AGG_HASHED total cost rises from 17900.50 (at effective_cpu_cache_size=8MB, table fits) to 121844.01 (at 1kB, penalty applies). AGG_SORTED total is unaffected by the GUC. The cost delta is exactly hash_entry_cost.

Repair (passed validation): Update the stale comment to state the totals are equal only when hash_entry_cost is zero, documenting the penalty as intentional plan-steering behavior. Comment-only fix; compiled and validated alongside PATCH-2's test. Patch SHA-256: a23ca1efe2c6db7c53810e6c93c6c27d370f8b301d29511c6c750c0a7b0fc855.

PATCH-2

— MAJOR (tests) — verified

Location: src/test/regress/sql/effective_cpu_cache_size.sql (missing)

The new planner cost GUC is shipped with no regress coverage. grep -rn effective_cpu_cache_size src/test/ returns zero matches. The GUC measurably changes plan choice (HashAggregate → GroupAggregate+Sort when cache assumption is tiny).

Evidence (executed): Live demonstration shows HashAggregate at default/0/1GB, GroupAggregate+Sort at 1kB. make installcheck with the new test passes (All 244 tests passed).

Repair (passed validation): New regress test src/test/regress/sql/effective_cpu_cache_size.sql + expected output + parallel_schedule entry. Validated: make installcheck EXTRA_TESTS=effective_cpu_cache_size passes on both feature-only and fully-corrected builds. Patch SHA-256: 5c869451ebb81b1055bf8a0599b1df755815d14ec6b592c6db615743b998d7bd.

PATCH-3

— MAJOR (correctness) — refuted

Location: src/backend/optimizer/path/costsize.c:2829

Claim: AGG_MIXED charges the full input_tuples penalty, then each hashed sub-rollup is separately costed as AGG_HASHED with the same subpath->rows, double-charging for grouping sets.

Refutation: The per-rollup full-input_tuples charge pattern is identical to the pre-existing, long-standing hash-agg spill disk/CPU cost treatment (costsize.c:2966-3025: spill_cost = depth * input_tuples * 2.0 * cpu_tuple_cost). The "don't charge input cost again" comment at pathnode.c:3250-3251 refers to input_startup_cost/input_total_cost (passed as 0.0, 0.0), not per-tuple hash-probe work. The patch introduces no new inconsistency.

SEC-1

— MAJOR (security) — refuted

Location: src/backend/optimizer/path/costsize.c:2853

Claim: LOG2 can produce Inf because entrysize * numGroups can overflow to +Inf.

Refutation: numGroups is clamped by clamp_row_est to MAXIMUM_ROWCOUNT=1e100. entrysize is a finite Size (max ~1.8e19 on 64-bit). The product cannot reach DBL_MAX (~1.8e308). LOG2 input is guarded by table_bytes > cache_bytes with cache_bytes > 0, so the argument is finite and > 1. No Inf, NaN, or division by zero is reachable. Matches existing LOG2 convention at costsize.c:2008 and :506.

Verification Summary

Finding	Severity	Status	Repair Validation
PATCH-1	BLOCKER	verified	passed (apply-check, diff-check, compile, repro, targeted-test)
PATCH-2	MAJOR	verified	passed (apply-check, diff-check, targeted-test)
PATCH-3	MAJOR	refuted	not-applicable-refuted
SEC-1	MAJOR	refuted	not-applicable-refuted

Findings verified: 2 (PATCH-1, PATCH-2)
Findings refuted: 2 (PATCH-3, SEC-1)
Repairs passed validation: 2

Repair Validation Summary

PATCH-1 (comment fix): git apply --check clean against HEAD; make -j4 exit 0;

live EXPLAIN repro confirmed the cost effect. Test example waived (comment-only, no behavior oracle). Validation methods: apply-check, diff-check, compile, repro, targeted-test.

PATCH-2 (regress test): git apply --check clean; make -j4 exit 0;

make installcheck EXTRA_TESTS=effective_cpu_cache_size — All 244 tests passed (including the new test). Validation methods: apply-check, diff-check, targeted-test.

Changed-Code Coverage

Coverage requirement: required (changed instrumentable path: src/backend/optimizer/path/costsize.c)

Execution lane: host-disposable (passed)

Test commands:

make installcheck EXTRA_TESTS=effective_cpu_cache_size (full regress suite, 244 tests)

Counter epoch: fresh (gcda files deleted before test run; gcovr 8.6 analysis)

Metrics:

Metric	Value
Changed executable lines	13
Covered changed lines	13
Compiler uncovered changed lines	0
Uncovered changed lines	0
Structural uncovered changed lines	0
Changed lines without coverage coordinates	0
Changed branches	10
Covered changed branches	10

Changed line coverage: 13/13 (100%). Changed branch coverage: 10/10 (100%).

Touched function: cost_agg (costsize.c:2806-3051)

Whole function lines: 83/83 covered (100%)
Whole function branches: 38/40 covered (95% taken at least once)
Entered: yes (execution count 12807)

Uncovered regions: none.

Uncovered branches: none (in the changed lines).

Non-instrumentable changes:

doc/src/sgml/config.sgml — documentation
src/backend/utils/misc/guc_parameters.dat — GUC data file (generates into guc.c)
src/backend/utils/misc/postgresql.conf.sample — config sample
src/include/optimizer/cost.h — header (macro/default)
src/include/optimizer/optimizer.h — header (extern declaration)

Integrity errors: none (empty list confirmed).

Security Review

No BLOCKER or MAJOR security vulnerability introduced by this patch. The GUC is correctly bounded (0..MAX_KILOBYTES), correctly typed (int KB), uses double arithmetic that avoids int-width overflow, and is gated behind effective_cpu_cache_size > 0. Privilege model matches existing PGC_USERSET cost-hint GUCs. The LOG2 Inf concern (SEC-1) was refuted: numGroups is clamped to 1e100 and the product cannot reach DBL_MAX.

pgsql-hackers Thread Ledger

Status: no-thread-found

One message found: the original patch submission by wbernbaum@dwdev.com on 2026-09-01 (Message-ID: PH0PR18MB444308903D2F293CEA42B1CCA6A82@PH0PR18MB4443.namprd18.prod.outlook.com). No review replies or follow-up discussion. All findings are new. The patch references CF 7201 (skipping NULL keys when uniqueifying a semijoin's RHS) and CF 7208 (eager deduplication) as related work.

Aggregated Test Plan

New regress test: src/test/regress/sql/effective_cpu_cache_size.sql +

expected output + parallel_schedule entry (PATCH-2 repair).

Oracle: EXPLAIN (COSTS OFF) plan shape: HashAggregate at default/0/1GB;

GroupAggregate+Sort at 1kB; SHOW default = 8MB.

Validation: make installcheck EXTRA_TESTS=effective_cpu_cache_size passes

(All 244 tests passed).

Residual Risk

HASH_RESIDENCY_PENALTY=4.0 is unjustified by any calibration or benchmark in

the patch comments. The author's mailing-list post provides calibration data (geometric mean factor 2.8x error unpatched, 1.06x patched) but the constant should be documented in-code.

The doc does not mention the interaction with work_mem/hash_mem_multiplier

(the resident-set clamp means the penalty saturates at mem_limit for spilled hash aggregates). A doc sentence would help users understand the saturation.

Provenance

Coordinator: pg-review-harness (glm-5.2:cloud)
pg-patch-reviewer: glm-5.2:cloud
pg-security-reviewer: glm-5.2:cloud
pg-finding-verifier: glm-5.2:cloud (executed builds, tests, and EXPLAIN repros)
Coverage: coordinator-executed host-disposable lane (gcc --coverage, gcovr 8.6)

CommitFest 7244 Review — effective_cpu_cache_size