[TheBeast85]

What

• Split report_severity into report_severity_cached, report_severity_dynamic and a report_severity that is a coalesce of the two.
• Mark the severity functions STABLE.

Why

report_severity was one function body, and that body was planned again on every call: 1479 ms of planning across 5047 calls against 30 ms of execution, measured on the severity aggregate with pg_stat_statements.track_planning on.

Almost every call is answered by the cached value in report_counts, a single index lookup. The four dynamic branches underneath scan the results table and are hardly ever evaluated, yet every call paid to plan them. A function body is only planned when the function is invoked, and coalesce stops at the first non null argument at execution time, so moving the expensive branches into their own function means they are only planned when there is no cached value. They stop appearing in pg_stat_statements altogether.

STABLE alone did not help, and neither did removing the CTE: task_severity calls report_severity from its own SQL body and is itself not inlinable, so every row creates a fresh call context.

The severity aggregate goes from 0.351 s to 0.098 s. Severity values and the aggregate are identical character for character with the reference taken before the change.

References

Split out of #3067 as requested.

Checklist

• Tested manually against a Community Edition stack

[clawosiris]

I reviewed the latest iteration at exact head 9121cef100a4a8bdb8e79c19e6575e1d2a7b9c06 for correctness, architectural fit, goal attainment, and test coverage.

Verdict

I found no P0-P2 correctness or architectural issue. The split is sound, is located at the correct PostgreSQL-function boundary, preserves the existing cache/fallback contract, and demonstrably avoids planning the expensive dynamic body on cached hits. I would treat the implementation as mergeable after rebasing onto current main and obtaining a current-head build/test run.

The main weakness is test coverage: the PR adds no automated regression for this central report-severity path. I also found one non-blocking wording nuance around inlining.

Correctness and architecture

• The old cached CTE used an aggregate without GROUP BY, so it always produced one row; its EXISTS(...) AND max IS NOT NULL guard was effectively just max IS NOT NULL. Replacing that with coalesce(report_severity_cached(...), report_severity_dynamic(...)) is semantically equivalent.
• The cache key and validity rules are unchanged: report, user, override mode, minimum QoD, and non-expired end_time still select the cached maximum.
• The four dynamic branches are preserved unchanged inside report_severity_dynamic: dynamic severity on/off crossed with overrides on/off.
• Marking gvmd_user, dynamic_severity, report_severity_*, and task_severity STABLE is appropriate. They are read-only and should observe one statement snapshot; gvmd_user wraps PostgreSQL's stable current_setting behavior.
• Keeping the split inside manage_create_sql_functions() is the right boundary. Callers continue using report_severity/task_severity; no C-level interface or persisted schema contract changes.
• Current main has since extended OVERRIDES_SQL for OCI digest matching. A merge-tree simulation is clean, and the macro expansion is carried into the new dynamic helper, so I found no semantic conflict there.

Goal validation

I reproduced the old and new SQL shapes on PostgreSQL 17 and compared nine cases: valid cached value, user-specific cache selection, expired cache, null cached aggregate, uncached static severity, dynamic severity, static override, dynamic override, null result type, and no results. Every old/new result was identical, including null behavior.

With pg_stat_statements.track = all and planning tracking enabled:

• 1,000 cached calls planned/executed only the report_counts lookup; the results-scanning dynamic body did not appear at all.
• 1,000 uncached calls invoked the dynamic body 1,000 times while planning that body once in the shared call context.
• In a synthetic task-shaped 5,000-call cached workload, the old form took about 2.38 s and the split form about 1.02 s. The old monolithic body accounted for about 1.45 s of planning. This is not a production benchmark, but it independently confirms the mechanism and direction claimed by the PR.

Non-blocking wording nuance: report_severity is eligible for scalar SQL inlining with simple arguments, but PostgreSQL 17 did not inline it in my task-shaped probe where the report-id subquery becomes an argument referenced by both coalesce arms. The optimization still worked because the dynamic helper was not invoked or planned on a cache hit. I would therefore describe the guaranteed benefit as lazy planning of the dynamic helper, not unconditional wrapper inlining.

Test coverage

The PR adds no tests. The existing C suite does not create these PostgreSQL functions or exercise cached/dynamic report severity, function volatility, or planning behavior.

Recommended additions:

1. A database-backed equivalence matrix covering cached non-null, absent/null/expired cache, user/override/min-QoD key separation, dynamic severity on/off, overrides on/off, null result type, and empty results.
2. A regression proving the dynamic function is not invoked on a cache hit and is invoked on a miss. A pg_stat_statements assertion on presence/absence of the dynamic body is more stable than asserting a wall-clock threshold.
3. A metadata check that the intended functions are STABLE, plus coverage that recreating functions during startup/upgrade installs all three definitions.
4. A benchmark script or documented query/data shape so the reported aggregate improvement can be reproduced.

Independent verification

• Exact-head Release build: passed.
• Exact-head Debug build: passed.
• All 11 configured CTest executables: passed.
• Focused PostgreSQL 17 semantic and planning probe: passed.
• git diff --check: passed.
• Clean merge simulation onto current main 1e50843f62deadd18579fd4a663dee0b26d5991d: passed.
• The branch is one commit ahead and 166 commits behind main. src/manage_pg.c changed upstream, so it should be rebased and revalidated before merge.
• GitHub currently reports only the auto-merge/Mergify checks, not a build/test check for this head.

Agent: GEA
Model: openai/gpt-5.6-sol
Thinking: xhigh
Bot-Owner: @llunved

[TheBeast85]

Rebased onto current main, no conflicts; the extended OVERRIDES_SQL is carried into report_severity_dynamic. Also reworded the comment and commit message: the benefit is lazy planning of the dynamic helper and does not depend on report_severity being inlined. Release build and all CTests pass.

[daniel-grnbn]

@TheBeast85 if that is OK with you, I'll merge #3206 into this branch?

[TheBeast85]

@daniel-grnbn Yes, please go ahead and merge #3206 into this branch. Thanks for the tests!