An Opus 4.8 review of pg_*_advice commits wrote test cases, attached, for several malfunctions. The test patch also contains its markdown report. A lot of its findings are more like long-term bugfix opportunities than open items, but I recommend treating at least these as an open item batch:
### 1. An empty (non-NULL) stashed advice string persists a TSV line the loader rejects ### 3. pg_start_stash_advice_worker() destroys an existing pg_stash_advice.tsv when persistence was off at boot, silently losing all persisted advice I wasn't 100% convinced this is unintentional, but Opus is more convinced. Worth a look from you. ### 14. Stash-supplied advice silently overrides an explicitly-set pg_plan_advice.advice for the same query; the precedence is documented nowhere I do feel the GUC should preempt the stash. Best to solidify this behavior before release. Here's the second tier, as I see it: ### 5. pg_plan_advice.advice / stash advice changes are silently ignored by an already-cached generic plan ### 8. pg_set_stashed_advice stores advice with no syntax validation, so malformed advice is accepted then emits a plan-time WARNING on every execution and applies nothing Bounded harm, but changing this would be a relatively-large boost to the user experience.
commit 96b082f (pg-advice-defect-tests) Author: Noah Misch <[email protected]> AuthorDate: Fri Jul 10 17:04:43 2026 +0000 Commit: Noah Misch <[email protected]> CommitDate: Fri Jul 10 17:04:43 2026 +0000 Add regression tests demonstrating pg_*_advice defects Add test cases that reproduce user-visible defects found in the pg_plan_advice and pg_stash_advice contrib modules (analysis in defect-report-pg-advice.md). Each test captures the current, buggy behavior of master and is annotated with the correct behavior; the expected output must be updated when a defect is fixed. contrib/pg_plan_advice/sql/defect_cases.sql covers: - JOIN_ORDER advice silently ignored under GEQO, because GEQO scores tours by total_cost only and ignores the disabled_nodes that enforcement sets (major) - advice changes ignored by an already-cached generic plan, and advice sticking after RESET (major) - SEMIJOIN_NON_UNIQUE and unordered-{} JOIN_ORDER reported "matched, failed" though the plan honors them (major) - scan advice on a partitioned parent disabling its Append, and schema-omitted advice on same-named partitions across schemas reported failed (minor) - FOREIGN_JOIN(()) accepted while FOREIGN_JOIN((a)) is rejected (minor) - a repeated-identifier JOIN_ORDER list accepted with no dedup/length cap, the root of the quadratic planner DoS (the hang itself is not exercised) contrib/pg_stash_advice/sql/defect_cases.sql covers: - pg_set_stashed_advice() accepting malformed advice with no validation, which then warns at plan time on every execution, unlike the GUC path (minor) contrib/pg_stash_advice TAP tests: - t/002_defect_empty_advice.pl: an empty (non-NULL) stashed advice string is written as a TSV line the loader rejects, so after a restart the persistence worker crash-loops and the whole stash subsystem is locked out (major) - t/003_defect_persist_off.pl: pg_start_stash_advice_worker() on a server booted with persist=off deletes an existing pg_stash_advice.tsv without loading it, losing all persisted advice (major) Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01FY4The4wz1uBZJkjTgyo1V --- contrib/pg_plan_advice/Makefile | 2 +- contrib/pg_plan_advice/expected/defect_cases.out | 290 +++++++++++++++++++++ contrib/pg_plan_advice/meson.build | 1 + contrib/pg_plan_advice/sql/defect_cases.sql | 222 ++++++++++++++++ contrib/pg_stash_advice/Makefile | 2 +- contrib/pg_stash_advice/expected/defect_cases.out | 71 +++++ contrib/pg_stash_advice/meson.build | 3 + contrib/pg_stash_advice/sql/defect_cases.sql | 56 ++++ .../pg_stash_advice/t/002_defect_empty_advice.pl | 68 +++++ .../pg_stash_advice/t/003_defect_persist_off.pl | 60 +++++ defect-report-pg-advice.md | 271 +++++++++++++++++++ 11 files changed, 1044 insertions(+), 2 deletions(-) diff --git a/contrib/pg_plan_advice/Makefile b/contrib/pg_plan_advice/Makefile index c844846..e37de17 100644 --- a/contrib/pg_plan_advice/Makefile +++ b/contrib/pg_plan_advice/Makefile @@ -20,7 +20,7 @@ HEADERS_pg_plan_advice = pg_plan_advice.h PGFILEDESC = "pg_plan_advice - help the planner get the right plan" REGRESS = alternatives gather join_order join_strategy partitionwise \ - prepared scan semijoin syntax + prepared scan semijoin syntax defect_cases TAP_TESTS = 1 diff --git a/contrib/pg_plan_advice/expected/defect_cases.out b/contrib/pg_plan_advice/expected/defect_cases.out new file mode 100644 index 0000000..068bd38 --- /dev/null +++ b/contrib/pg_plan_advice/expected/defect_cases.out @@ -0,0 +1,290 @@ +-- Regression tests that demonstrate user-visible defects in pg_plan_advice +-- (see defect-report-pg-advice.md). Each block captures the CURRENT (buggy) +-- behavior of master; the comments state what the correct behavior would be. +-- When a defect is fixed, the corresponding expected output must be updated. +LOAD 'pg_plan_advice'; +SET max_parallel_workers_per_gather = 0; +-- Helper: return only the "Supplied Plan Advice" verdict line(s) from +-- EXPLAIN (PLAN_ADVICE), so assertions are stable regardless of plan shape. +CREATE FUNCTION advice_feedback(q text) RETURNS SETOF text +LANGUAGE plpgsql AS $$ +DECLARE + line text; + grab boolean := false; +BEGIN + FOR line IN EXECUTE 'EXPLAIN (COSTS OFF, PLAN_ADVICE) ' || q + LOOP + IF line ~ 'Generated Plan Advice:' THEN grab := false; END IF; + IF grab AND line ~ '/\*' THEN RETURN NEXT btrim(line); END IF; + IF line ~ 'Supplied Plan Advice:' THEN grab := true; END IF; + END LOOP; +END; +$$; +------------------------------------------------------------------------------ +-- Finding 2 (major): a JOIN_ORDER advice list has no dedup or length cap, the +-- root cause of the Theta(N^2), CHECK_FOR_INTERRUPTS-free planner DoS. The +-- quadratic *planning* blowup is not exercised here (a large repeat count would +-- hang for minutes); this only documents that the degenerate input is accepted. +------------------------------------------------------------------------------ +-- (SET only takes a literal, so build the long list with set_config().) +SELECT set_config('pg_plan_advice.advice', + 'JOIN_ORDER(' || repeat('a ', 200) || 'a)', false) + IS NOT NULL AS advice_accepted; + advice_accepted +----------------- + t +(1 row) + +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 4 (major): under GEQO, JOIN_ORDER advice is silently ignored, because +-- GEQO scores tours by total_cost only and ignores the disabled_nodes that +-- enforcement sets. Correct behavior: the advice should be honored (or GEQO +-- should be bypassed) exactly as it is with geqo=off. +------------------------------------------------------------------------------ +CREATE TABLE dc_f (k1 int, k2 int, k3 int, k4 int); +INSERT INTO dc_f SELECT (g%1000)+1,(g%1000)+1,(g%1000)+1,(g%1000)+1 + FROM generate_series(1,200000) g; +CREATE TABLE dc_d1 (id int primary key, b int); +CREATE TABLE dc_d2 (id int primary key, b int); +CREATE TABLE dc_d3 (id int primary key, b int); +CREATE TABLE dc_d4 (id int primary key, b int); +INSERT INTO dc_d1 SELECT g, (g=1)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d2 SELECT g, (g<=10)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d3 SELECT g, (g<=100)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d4 SELECT g, 1 FROM generate_series(1,1000) g; +CREATE INDEX ON dc_f(k1); CREATE INDEX ON dc_f(k2); +CREATE INDEX ON dc_f(k3); CREATE INDEX ON dc_f(k4); +ANALYZE dc_f, dc_d1, dc_d2, dc_d3, dc_d4; +-- Force the expensive-but-feasible order (apply the most selective dim last). +SET pg_plan_advice.advice = + 'JOIN_ORDER(dc_d4 dc_f dc_d3 dc_d2 dc_d1)'; +-- geqo off: advice honored. +SET geqo = off; +SELECT advice_feedback($$ + SELECT * FROM dc_f + JOIN dc_d1 ON dc_f.k1=dc_d1.id JOIN dc_d2 ON dc_f.k2=dc_d2.id + JOIN dc_d3 ON dc_f.k3=dc_d3.id JOIN dc_d4 ON dc_f.k4=dc_d4.id + WHERE dc_d1.b=1 AND dc_d2.b=1 AND dc_d3.b=1 AND dc_d4.b=1 $$) AS geqo_off; + geqo_off +-------------------------------------------------------- + JOIN_ORDER(dc_d4 dc_f dc_d3 dc_d2 dc_d1) /* matched */ +(1 row) + +-- geqo on: DEFECT -- same achievable advice is reported "matched, failed" +-- because GEQO returned the cheaper violating order. +SET geqo = on; +SET geqo_threshold = 2; +SET geqo_seed = 0; +SELECT advice_feedback($$ + SELECT * FROM dc_f + JOIN dc_d1 ON dc_f.k1=dc_d1.id JOIN dc_d2 ON dc_f.k2=dc_d2.id + JOIN dc_d3 ON dc_f.k3=dc_d3.id JOIN dc_d4 ON dc_f.k4=dc_d4.id + WHERE dc_d1.b=1 AND dc_d2.b=1 AND dc_d3.b=1 AND dc_d4.b=1 $$) AS geqo_on; + geqo_on +---------------------------------------------------------------- + JOIN_ORDER(dc_d4 dc_f dc_d3 dc_d2 dc_d1) /* matched, failed */ +(1 row) + +RESET geqo; RESET geqo_threshold; RESET geqo_seed; +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 5 (major): once a generic plan is cached (prepared statement), a +-- change to pg_plan_advice.advice is silently ignored, and advice baked into a +-- cached plan survives RESET. Correct behavior: the cached plan should be +-- invalidated / re-planned when the advice changes. +------------------------------------------------------------------------------ +CREATE TABLE dc_t (id int primary key, v int); +INSERT INTO dc_t SELECT g, g%100 FROM generate_series(1,50000) g; +CREATE INDEX ON dc_t(v); +ANALYZE dc_t; +PREPARE dc_ps AS SELECT * FROM dc_t WHERE v = $1; +SET plan_cache_mode = force_generic_plan; +-- Cache the generic plan with no advice. +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); + QUERY PLAN +--------------------------------------- + Bitmap Heap Scan on dc_t + Recheck Cond: (v = $1) + -> Bitmap Index Scan on dc_t_v_idx + Index Cond: (v = $1) +(4 rows) + +-- DEFECT: setting advice now does not change the already-cached plan. +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_t)'; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); + QUERY PLAN +--------------------------------------- + Bitmap Heap Scan on dc_t + Recheck Cond: (v = $1) + -> Bitmap Index Scan on dc_t_v_idx + Index Cond: (v = $1) +(4 rows) + +-- Proof the advice itself is valid: a freshly prepared statement honors it. +DEALLOCATE dc_ps; +PREPARE dc_ps AS SELECT * FROM dc_t WHERE v = $1; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); + QUERY PLAN +-------------------- + Seq Scan on dc_t + Filter: (v = $1) +(2 rows) + +-- DEFECT (reverse): clearing the advice does not drop it from the cached plan. +RESET pg_plan_advice.advice; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); + QUERY PLAN +-------------------- + Seq Scan on dc_t + Filter: (v = $1) +(2 rows) + +DEALLOCATE dc_ps; +RESET plan_cache_mode; +------------------------------------------------------------------------------ +-- Finding 6 (major): SEMIJOIN_NON_UNIQUE advice on a semijoin the planner +-- cannot unique-ify is reported "matched, failed" even though the plan is the +-- requested non-unique Semi Join. Correct behavior: plain "matched". +------------------------------------------------------------------------------ +CREATE TABLE dc_sa (x int); +CREATE TABLE dc_sb (y int); +INSERT INTO dc_sa SELECT generate_series(1,100); +INSERT INTO dc_sb SELECT generate_series(1,100); +ANALYZE dc_sa, dc_sb; +SET pg_plan_advice.advice = 'SEMIJOIN_NON_UNIQUE(dc_sb)'; +-- DEFECT: non-equality correlation -> "matched, failed" though plan honors it. +SELECT advice_feedback($$ + SELECT * FROM dc_sa WHERE EXISTS + (SELECT 1 FROM dc_sb WHERE dc_sb.y > dc_sa.x) $$) AS non_equality; + non_equality +-------------------------------------------------- + SEMIJOIN_NON_UNIQUE(dc_sb) /* matched, failed */ +(1 row) + +-- Control: equality correlation reports a clean "matched". +SELECT advice_feedback($$ + SELECT * FROM dc_sa WHERE EXISTS + (SELECT 1 FROM dc_sb WHERE dc_sb.y = dc_sa.x) $$) AS equality; + equality +------------------------------------------ + SEMIJOIN_NON_UNIQUE(dc_sb) /* matched */ +(1 row) + +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 7 (major): JOIN_ORDER advice with an unordered {..} sublist is +-- reported "matched, failed" while the identical plan under the parenthesized +-- (..) form reports "matched". Correct behavior: both report "matched". +------------------------------------------------------------------------------ +-- Same shape as sql/join_order.sql (which bakes this bug into join_order.out): +-- the selective dims are hash-joined together first, forming a {d1,d2} sub-join +-- that the advice names. The query aliases the tables f/d1/d2 so the advice +-- identifiers match. +CREATE TABLE dc_jd1 (id integer primary key, val1 int); +INSERT INTO dc_jd1 SELECT g, (g % 3) + 1 FROM generate_series(1,100) g; +CREATE TABLE dc_jd2 (id integer primary key, val2 int); +INSERT INTO dc_jd2 SELECT g, (g % 53) + 1 FROM generate_series(1,1000) g; +CREATE TABLE dc_jf (id int primary key, dim1_id int, dim2_id int); +INSERT INTO dc_jf SELECT g, (g%100)+1, (g%100)+1 FROM generate_series(1,100000) g; +ANALYZE dc_jf, dc_jd1, dc_jd2; +-- DEFECT: unordered {d1 d2} sublist -> "matched, failed" though plan conforms. +SET pg_plan_advice.advice = 'JOIN_ORDER(f {d1 d2})'; +SELECT advice_feedback($$ + SELECT * FROM dc_jf f LEFT JOIN dc_jd1 d1 ON f.dim1_id=d1.id + LEFT JOIN dc_jd2 d2 ON f.dim2_id=d2.id + WHERE d1.val1=1 AND d2.val2=1 $$) AS unordered; + unordered +--------------------------------------------- + JOIN_ORDER(f {d1 d2}) /* matched, failed */ +(1 row) + +-- Control: ordered (d1 d2) sublist on the identical plan -> "matched". +SET pg_plan_advice.advice = 'JOIN_ORDER(f (d1 d2))'; +SELECT advice_feedback($$ + SELECT * FROM dc_jf f LEFT JOIN dc_jd1 d1 ON f.dim1_id=d1.id + LEFT JOIN dc_jd2 d2 ON f.dim2_id=d2.id + WHERE d1.val1=1 AND d2.val2=1 $$) AS ordered; + ordered +------------------------------------- + JOIN_ORDER(f (d1 d2)) /* matched */ +(1 row) + +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 9 (minor): scan-type advice naming a partitioned parent disables its +-- Append (Disabled: true) and is applied to no scan. Correct behavior: the +-- advice should reach the partition scans, and the Append should not be +-- disabled. +------------------------------------------------------------------------------ +CREATE TABLE dc_p (id int) PARTITION BY RANGE (id); +CREATE TABLE dc_p1 PARTITION OF dc_p FOR VALUES FROM (0) TO (100); +CREATE TABLE dc_p2 PARTITION OF dc_p FOR VALUES FROM (100) TO (200); +INSERT INTO dc_p SELECT generate_series(0,199); +ANALYZE dc_p; +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_p)'; +-- DEFECT: the Append over the partitions is flagged Disabled: true. +EXPLAIN (COSTS OFF) SELECT * FROM dc_p; + QUERY PLAN +---------------------------------------- + Append + Disabled: true + -> Seq Scan on dc_p1 dc_p_1 + -> Seq Scan on dc_p2 dc_p_2 + Supplied Plan Advice: + SEQ_SCAN(dc_p) /* matched, failed */ +(6 rows) + +-- DEFECT: the advice is reported "matched, failed". +SELECT advice_feedback($$ SELECT * FROM dc_p $$) AS partitioned_parent; + partitioned_parent +-------------------------------------- + SEQ_SCAN(dc_p) /* matched, failed */ +(1 row) + +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 10 (minor): schema-omitted advice matching two same-named partitions +-- in different schemas is enforced on both, yet reported "matched, failed". +-- Correct behavior: "matched". +------------------------------------------------------------------------------ +CREATE TABLE dc_pp (a int) PARTITION BY RANGE (a); +CREATE SCHEMA dc_s1; +CREATE SCHEMA dc_s2; +CREATE TABLE dc_s1.child PARTITION OF dc_pp FOR VALUES FROM (0) TO (10); +CREATE TABLE dc_s2.child PARTITION OF dc_pp FOR VALUES FROM (10) TO (20); +INSERT INTO dc_pp SELECT generate_series(0,19); +ANALYZE dc_pp; +-- DEFECT: schema omitted (dc_pp/child) -> enforced on both but "matched, failed". +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_pp/child)'; +SELECT advice_feedback($$ SELECT * FROM dc_pp $$) AS cross_schema; + cross_schema +--------------------------------------------- + SEQ_SCAN(dc_pp/child) /* matched, failed */ +(1 row) + +RESET pg_plan_advice.advice; +------------------------------------------------------------------------------ +-- Finding 12 (minor): FOREIGN_JOIN(()) with an empty sublist bypasses the +-- >1-relation arity check and is silently accepted, while FOREIGN_JOIN((a)) is +-- correctly rejected. Correct behavior: FOREIGN_JOIN(()) should also error. +------------------------------------------------------------------------------ +-- DEFECT: accepted (no error). +SET pg_plan_advice.advice = 'FOREIGN_JOIN(())'; +SELECT current_setting('pg_plan_advice.advice') AS empty_sublist_accepted; + empty_sublist_accepted +------------------------ + FOREIGN_JOIN(()) +(1 row) + +RESET pg_plan_advice.advice; +-- Control: the one-relation sublist is correctly rejected at SET time. +SET pg_plan_advice.advice = 'FOREIGN_JOIN((dc_x))'; +ERROR: invalid value for parameter "pg_plan_advice.advice": "FOREIGN_JOIN((dc_x))" +DETAIL: Could not parse advice: FOREIGN_JOIN targets must contain more than one relation identifier at or near ")" +RESET pg_plan_advice.advice; +-- Cleanup. +DROP FUNCTION advice_feedback(text); +DROP TABLE dc_f, dc_d1, dc_d2, dc_d3, dc_d4, dc_t, dc_sa, dc_sb, + dc_jf, dc_jd1, dc_jd2, dc_p, dc_pp CASCADE; +DROP SCHEMA dc_s1, dc_s2 CASCADE; diff --git a/contrib/pg_plan_advice/meson.build b/contrib/pg_plan_advice/meson.build index bbab676..702ffb1 100644 --- a/contrib/pg_plan_advice/meson.build +++ b/contrib/pg_plan_advice/meson.build @@ -62,6 +62,7 @@ tests += { 'scan', 'semijoin', 'syntax', + 'defect_cases', ], }, 'tap': { diff --git a/contrib/pg_plan_advice/sql/defect_cases.sql b/contrib/pg_plan_advice/sql/defect_cases.sql new file mode 100644 index 0000000..85c9298 --- /dev/null +++ b/contrib/pg_plan_advice/sql/defect_cases.sql @@ -0,0 +1,222 @@ +-- Regression tests that demonstrate user-visible defects in pg_plan_advice +-- (see defect-report-pg-advice.md). Each block captures the CURRENT (buggy) +-- behavior of master; the comments state what the correct behavior would be. +-- When a defect is fixed, the corresponding expected output must be updated. + +LOAD 'pg_plan_advice'; +SET max_parallel_workers_per_gather = 0; + +-- Helper: return only the "Supplied Plan Advice" verdict line(s) from +-- EXPLAIN (PLAN_ADVICE), so assertions are stable regardless of plan shape. +CREATE FUNCTION advice_feedback(q text) RETURNS SETOF text +LANGUAGE plpgsql AS $$ +DECLARE + line text; + grab boolean := false; +BEGIN + FOR line IN EXECUTE 'EXPLAIN (COSTS OFF, PLAN_ADVICE) ' || q + LOOP + IF line ~ 'Generated Plan Advice:' THEN grab := false; END IF; + IF grab AND line ~ '/\*' THEN RETURN NEXT btrim(line); END IF; + IF line ~ 'Supplied Plan Advice:' THEN grab := true; END IF; + END LOOP; +END; +$$; + +------------------------------------------------------------------------------ +-- Finding 2 (major): a JOIN_ORDER advice list has no dedup or length cap, the +-- root cause of the Theta(N^2), CHECK_FOR_INTERRUPTS-free planner DoS. The +-- quadratic *planning* blowup is not exercised here (a large repeat count would +-- hang for minutes); this only documents that the degenerate input is accepted. +------------------------------------------------------------------------------ +-- (SET only takes a literal, so build the long list with set_config().) +SELECT set_config('pg_plan_advice.advice', + 'JOIN_ORDER(' || repeat('a ', 200) || 'a)', false) + IS NOT NULL AS advice_accepted; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 4 (major): under GEQO, JOIN_ORDER advice is silently ignored, because +-- GEQO scores tours by total_cost only and ignores the disabled_nodes that +-- enforcement sets. Correct behavior: the advice should be honored (or GEQO +-- should be bypassed) exactly as it is with geqo=off. +------------------------------------------------------------------------------ +CREATE TABLE dc_f (k1 int, k2 int, k3 int, k4 int); +INSERT INTO dc_f SELECT (g%1000)+1,(g%1000)+1,(g%1000)+1,(g%1000)+1 + FROM generate_series(1,200000) g; +CREATE TABLE dc_d1 (id int primary key, b int); +CREATE TABLE dc_d2 (id int primary key, b int); +CREATE TABLE dc_d3 (id int primary key, b int); +CREATE TABLE dc_d4 (id int primary key, b int); +INSERT INTO dc_d1 SELECT g, (g=1)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d2 SELECT g, (g<=10)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d3 SELECT g, (g<=100)::int FROM generate_series(1,1000) g; +INSERT INTO dc_d4 SELECT g, 1 FROM generate_series(1,1000) g; +CREATE INDEX ON dc_f(k1); CREATE INDEX ON dc_f(k2); +CREATE INDEX ON dc_f(k3); CREATE INDEX ON dc_f(k4); +ANALYZE dc_f, dc_d1, dc_d2, dc_d3, dc_d4; + +-- Force the expensive-but-feasible order (apply the most selective dim last). +SET pg_plan_advice.advice = + 'JOIN_ORDER(dc_d4 dc_f dc_d3 dc_d2 dc_d1)'; + +-- geqo off: advice honored. +SET geqo = off; +SELECT advice_feedback($$ + SELECT * FROM dc_f + JOIN dc_d1 ON dc_f.k1=dc_d1.id JOIN dc_d2 ON dc_f.k2=dc_d2.id + JOIN dc_d3 ON dc_f.k3=dc_d3.id JOIN dc_d4 ON dc_f.k4=dc_d4.id + WHERE dc_d1.b=1 AND dc_d2.b=1 AND dc_d3.b=1 AND dc_d4.b=1 $$) AS geqo_off; + +-- geqo on: DEFECT -- same achievable advice is reported "matched, failed" +-- because GEQO returned the cheaper violating order. +SET geqo = on; +SET geqo_threshold = 2; +SET geqo_seed = 0; +SELECT advice_feedback($$ + SELECT * FROM dc_f + JOIN dc_d1 ON dc_f.k1=dc_d1.id JOIN dc_d2 ON dc_f.k2=dc_d2.id + JOIN dc_d3 ON dc_f.k3=dc_d3.id JOIN dc_d4 ON dc_f.k4=dc_d4.id + WHERE dc_d1.b=1 AND dc_d2.b=1 AND dc_d3.b=1 AND dc_d4.b=1 $$) AS geqo_on; + +RESET geqo; RESET geqo_threshold; RESET geqo_seed; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 5 (major): once a generic plan is cached (prepared statement), a +-- change to pg_plan_advice.advice is silently ignored, and advice baked into a +-- cached plan survives RESET. Correct behavior: the cached plan should be +-- invalidated / re-planned when the advice changes. +------------------------------------------------------------------------------ +CREATE TABLE dc_t (id int primary key, v int); +INSERT INTO dc_t SELECT g, g%100 FROM generate_series(1,50000) g; +CREATE INDEX ON dc_t(v); +ANALYZE dc_t; + +PREPARE dc_ps AS SELECT * FROM dc_t WHERE v = $1; +SET plan_cache_mode = force_generic_plan; + +-- Cache the generic plan with no advice. +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); +-- DEFECT: setting advice now does not change the already-cached plan. +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_t)'; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); +-- Proof the advice itself is valid: a freshly prepared statement honors it. +DEALLOCATE dc_ps; +PREPARE dc_ps AS SELECT * FROM dc_t WHERE v = $1; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); +-- DEFECT (reverse): clearing the advice does not drop it from the cached plan. +RESET pg_plan_advice.advice; +EXPLAIN (COSTS OFF) EXECUTE dc_ps(7); + +DEALLOCATE dc_ps; +RESET plan_cache_mode; + +------------------------------------------------------------------------------ +-- Finding 6 (major): SEMIJOIN_NON_UNIQUE advice on a semijoin the planner +-- cannot unique-ify is reported "matched, failed" even though the plan is the +-- requested non-unique Semi Join. Correct behavior: plain "matched". +------------------------------------------------------------------------------ +CREATE TABLE dc_sa (x int); +CREATE TABLE dc_sb (y int); +INSERT INTO dc_sa SELECT generate_series(1,100); +INSERT INTO dc_sb SELECT generate_series(1,100); +ANALYZE dc_sa, dc_sb; + +SET pg_plan_advice.advice = 'SEMIJOIN_NON_UNIQUE(dc_sb)'; +-- DEFECT: non-equality correlation -> "matched, failed" though plan honors it. +SELECT advice_feedback($$ + SELECT * FROM dc_sa WHERE EXISTS + (SELECT 1 FROM dc_sb WHERE dc_sb.y > dc_sa.x) $$) AS non_equality; +-- Control: equality correlation reports a clean "matched". +SELECT advice_feedback($$ + SELECT * FROM dc_sa WHERE EXISTS + (SELECT 1 FROM dc_sb WHERE dc_sb.y = dc_sa.x) $$) AS equality; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 7 (major): JOIN_ORDER advice with an unordered {..} sublist is +-- reported "matched, failed" while the identical plan under the parenthesized +-- (..) form reports "matched". Correct behavior: both report "matched". +------------------------------------------------------------------------------ +-- Same shape as sql/join_order.sql (which bakes this bug into join_order.out): +-- the selective dims are hash-joined together first, forming a {d1,d2} sub-join +-- that the advice names. The query aliases the tables f/d1/d2 so the advice +-- identifiers match. +CREATE TABLE dc_jd1 (id integer primary key, val1 int); +INSERT INTO dc_jd1 SELECT g, (g % 3) + 1 FROM generate_series(1,100) g; +CREATE TABLE dc_jd2 (id integer primary key, val2 int); +INSERT INTO dc_jd2 SELECT g, (g % 53) + 1 FROM generate_series(1,1000) g; +CREATE TABLE dc_jf (id int primary key, dim1_id int, dim2_id int); +INSERT INTO dc_jf SELECT g, (g%100)+1, (g%100)+1 FROM generate_series(1,100000) g; +ANALYZE dc_jf, dc_jd1, dc_jd2; + +-- DEFECT: unordered {d1 d2} sublist -> "matched, failed" though plan conforms. +SET pg_plan_advice.advice = 'JOIN_ORDER(f {d1 d2})'; +SELECT advice_feedback($$ + SELECT * FROM dc_jf f LEFT JOIN dc_jd1 d1 ON f.dim1_id=d1.id + LEFT JOIN dc_jd2 d2 ON f.dim2_id=d2.id + WHERE d1.val1=1 AND d2.val2=1 $$) AS unordered; +-- Control: ordered (d1 d2) sublist on the identical plan -> "matched". +SET pg_plan_advice.advice = 'JOIN_ORDER(f (d1 d2))'; +SELECT advice_feedback($$ + SELECT * FROM dc_jf f LEFT JOIN dc_jd1 d1 ON f.dim1_id=d1.id + LEFT JOIN dc_jd2 d2 ON f.dim2_id=d2.id + WHERE d1.val1=1 AND d2.val2=1 $$) AS ordered; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 9 (minor): scan-type advice naming a partitioned parent disables its +-- Append (Disabled: true) and is applied to no scan. Correct behavior: the +-- advice should reach the partition scans, and the Append should not be +-- disabled. +------------------------------------------------------------------------------ +CREATE TABLE dc_p (id int) PARTITION BY RANGE (id); +CREATE TABLE dc_p1 PARTITION OF dc_p FOR VALUES FROM (0) TO (100); +CREATE TABLE dc_p2 PARTITION OF dc_p FOR VALUES FROM (100) TO (200); +INSERT INTO dc_p SELECT generate_series(0,199); +ANALYZE dc_p; + +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_p)'; +-- DEFECT: the Append over the partitions is flagged Disabled: true. +EXPLAIN (COSTS OFF) SELECT * FROM dc_p; +-- DEFECT: the advice is reported "matched, failed". +SELECT advice_feedback($$ SELECT * FROM dc_p $$) AS partitioned_parent; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 10 (minor): schema-omitted advice matching two same-named partitions +-- in different schemas is enforced on both, yet reported "matched, failed". +-- Correct behavior: "matched". +------------------------------------------------------------------------------ +CREATE TABLE dc_pp (a int) PARTITION BY RANGE (a); +CREATE SCHEMA dc_s1; +CREATE SCHEMA dc_s2; +CREATE TABLE dc_s1.child PARTITION OF dc_pp FOR VALUES FROM (0) TO (10); +CREATE TABLE dc_s2.child PARTITION OF dc_pp FOR VALUES FROM (10) TO (20); +INSERT INTO dc_pp SELECT generate_series(0,19); +ANALYZE dc_pp; + +-- DEFECT: schema omitted (dc_pp/child) -> enforced on both but "matched, failed". +SET pg_plan_advice.advice = 'SEQ_SCAN(dc_pp/child)'; +SELECT advice_feedback($$ SELECT * FROM dc_pp $$) AS cross_schema; +RESET pg_plan_advice.advice; + +------------------------------------------------------------------------------ +-- Finding 12 (minor): FOREIGN_JOIN(()) with an empty sublist bypasses the +-- >1-relation arity check and is silently accepted, while FOREIGN_JOIN((a)) is +-- correctly rejected. Correct behavior: FOREIGN_JOIN(()) should also error. +------------------------------------------------------------------------------ +-- DEFECT: accepted (no error). +SET pg_plan_advice.advice = 'FOREIGN_JOIN(())'; +SELECT current_setting('pg_plan_advice.advice') AS empty_sublist_accepted; +RESET pg_plan_advice.advice; +-- Control: the one-relation sublist is correctly rejected at SET time. +SET pg_plan_advice.advice = 'FOREIGN_JOIN((dc_x))'; +RESET pg_plan_advice.advice; + +-- Cleanup. +DROP FUNCTION advice_feedback(text); +DROP TABLE dc_f, dc_d1, dc_d2, dc_d3, dc_d4, dc_t, dc_sa, dc_sb, + dc_jf, dc_jd1, dc_jd2, dc_p, dc_pp CASCADE; +DROP SCHEMA dc_s1, dc_s2 CASCADE; diff --git a/contrib/pg_stash_advice/Makefile b/contrib/pg_stash_advice/Makefile index 470c07b..f7f1f62 100644 --- a/contrib/pg_stash_advice/Makefile +++ b/contrib/pg_stash_advice/Makefile @@ -11,7 +11,7 @@ EXTENSION = pg_stash_advice DATA = pg_stash_advice--1.0.sql PGFILEDESC = "pg_stash_advice - store and automatically apply plan advice" -REGRESS = pg_stash_advice pg_stash_advice_utf8 +REGRESS = pg_stash_advice pg_stash_advice_utf8 defect_cases TAP_TESTS = 1 EXTRA_INSTALL = contrib/pg_plan_advice diff --git a/contrib/pg_stash_advice/expected/defect_cases.out b/contrib/pg_stash_advice/expected/defect_cases.out new file mode 100644 index 0000000..c6a67b1 --- /dev/null +++ b/contrib/pg_stash_advice/expected/defect_cases.out @@ -0,0 +1,71 @@ +-- Regression test demonstrating a user-visible defect in pg_stash_advice +-- (see defect-report-pg-advice.md, finding 8). This captures the CURRENT +-- (buggy) behavior of master; when the defect is fixed the expected output +-- must be updated. +-- The pg_stash_advice extension is created by the pg_stash_advice regression +-- test, which runs earlier in the same database; this test reuses it. +SET compute_query_id = on; +SET max_parallel_workers_per_gather = 0; +-- Helper: extract the query identifier from EXPLAIN VERBOSE output. +CREATE FUNCTION get_query_id_dc(query_text text) RETURNS bigint +LANGUAGE plpgsql AS $$ +DECLARE + line text; + qid bigint; +BEGIN + FOR line IN EXECUTE 'EXPLAIN (VERBOSE, FORMAT TEXT) ' || query_text + LOOP + IF line ~ 'Query Identifier:' THEN + qid := regexp_replace(line, '.*Query Identifier:\s*(-?\d+).*', '\1')::bigint; + RETURN qid; + END IF; + END LOOP; + RAISE EXCEPTION 'Query Identifier not found'; +END; +$$; +CREATE TABLE dcs_t (id int primary key, v int); +INSERT INTO dcs_t SELECT g, g % 100 FROM generate_series(1,1000) g; +ANALYZE dcs_t; +SELECT get_query_id_dc($$ SELECT count(*) FROM dcs_t WHERE v = 7 $$) AS qid \gset +------------------------------------------------------------------------------ +-- Finding 8 (minor): pg_set_stashed_advice() performs no syntax validation, so +-- malformed advice is accepted, then emits a plan-time WARNING on every +-- execution of the matching query and applies nothing. Correct behavior: reject +-- the malformed string at set time, exactly as the pg_plan_advice.advice GUC +-- does (shown at the end). +------------------------------------------------------------------------------ +SELECT pg_create_advice_stash('dcs_stash'); + pg_create_advice_stash +------------------------ + +(1 row) + +-- DEFECT: malformed advice accepted with no complaint. +SELECT pg_set_stashed_advice('dcs_stash', :qid, 'BOGUS_TAG(dcs_t)'); + pg_set_stashed_advice +----------------------- + +(1 row) + +SET pg_stash_advice.stash_name = 'dcs_stash'; +-- DEFECT: planning the query now warns and applies nothing (count still 10). +SELECT count(*) FROM dcs_t WHERE v = 7; +WARNING: could not parse supplied advice: syntax error at or near "BOGUS_TAG" + count +------- + 10 +(1 row) + +-- Contrast: the identical string via the GUC is rejected at SET time. +SET pg_plan_advice.advice = 'BOGUS_TAG(dcs_t)'; +ERROR: invalid value for parameter "pg_plan_advice.advice": "BOGUS_TAG(dcs_t)" +DETAIL: Could not parse advice: syntax error at or near "BOGUS_TAG" +RESET pg_stash_advice.stash_name; +SELECT pg_drop_advice_stash('dcs_stash'); + pg_drop_advice_stash +---------------------- + +(1 row) + +DROP TABLE dcs_t; +DROP FUNCTION get_query_id_dc(text); diff --git a/contrib/pg_stash_advice/meson.build b/contrib/pg_stash_advice/meson.build index 96f485b..fb0a75b 100644 --- a/contrib/pg_stash_advice/meson.build +++ b/contrib/pg_stash_advice/meson.build @@ -33,11 +33,14 @@ tests += { 'sql': [ 'pg_stash_advice', 'pg_stash_advice_utf8', + 'defect_cases', ], }, 'tap': { 'tests': [ 't/001_persist.pl', + 't/002_defect_empty_advice.pl', + 't/003_defect_persist_off.pl', ], }, } diff --git a/contrib/pg_stash_advice/sql/defect_cases.sql b/contrib/pg_stash_advice/sql/defect_cases.sql new file mode 100644 index 0000000..dbfc299 --- /dev/null +++ b/contrib/pg_stash_advice/sql/defect_cases.sql @@ -0,0 +1,56 @@ +-- Regression test demonstrating a user-visible defect in pg_stash_advice +-- (see defect-report-pg-advice.md, finding 8). This captures the CURRENT +-- (buggy) behavior of master; when the defect is fixed the expected output +-- must be updated. + +-- The pg_stash_advice extension is created by the pg_stash_advice regression +-- test, which runs earlier in the same database; this test reuses it. +SET compute_query_id = on; +SET max_parallel_workers_per_gather = 0; + +-- Helper: extract the query identifier from EXPLAIN VERBOSE output. +CREATE FUNCTION get_query_id_dc(query_text text) RETURNS bigint +LANGUAGE plpgsql AS $$ +DECLARE + line text; + qid bigint; +BEGIN + FOR line IN EXECUTE 'EXPLAIN (VERBOSE, FORMAT TEXT) ' || query_text + LOOP + IF line ~ 'Query Identifier:' THEN + qid := regexp_replace(line, '.*Query Identifier:\s*(-?\d+).*', '\1')::bigint; + RETURN qid; + END IF; + END LOOP; + RAISE EXCEPTION 'Query Identifier not found'; +END; +$$; + +CREATE TABLE dcs_t (id int primary key, v int); +INSERT INTO dcs_t SELECT g, g % 100 FROM generate_series(1,1000) g; +ANALYZE dcs_t; + +SELECT get_query_id_dc($$ SELECT count(*) FROM dcs_t WHERE v = 7 $$) AS qid \gset + +------------------------------------------------------------------------------ +-- Finding 8 (minor): pg_set_stashed_advice() performs no syntax validation, so +-- malformed advice is accepted, then emits a plan-time WARNING on every +-- execution of the matching query and applies nothing. Correct behavior: reject +-- the malformed string at set time, exactly as the pg_plan_advice.advice GUC +-- does (shown at the end). +------------------------------------------------------------------------------ +SELECT pg_create_advice_stash('dcs_stash'); +-- DEFECT: malformed advice accepted with no complaint. +SELECT pg_set_stashed_advice('dcs_stash', :qid, 'BOGUS_TAG(dcs_t)'); +SET pg_stash_advice.stash_name = 'dcs_stash'; + +-- DEFECT: planning the query now warns and applies nothing (count still 10). +SELECT count(*) FROM dcs_t WHERE v = 7; + +-- Contrast: the identical string via the GUC is rejected at SET time. +SET pg_plan_advice.advice = 'BOGUS_TAG(dcs_t)'; + +RESET pg_stash_advice.stash_name; +SELECT pg_drop_advice_stash('dcs_stash'); +DROP TABLE dcs_t; +DROP FUNCTION get_query_id_dc(text); diff --git a/contrib/pg_stash_advice/t/002_defect_empty_advice.pl b/contrib/pg_stash_advice/t/002_defect_empty_advice.pl new file mode 100644 index 0000000..4106655 --- /dev/null +++ b/contrib/pg_stash_advice/t/002_defect_empty_advice.pl @@ -0,0 +1,68 @@ +# Copyright (c) 2016-2026, PostgreSQL Global Development Group + +# Finding 1 (major) from defect-report-pg-advice.md: an empty (non-NULL) +# stashed advice string is written to pg_stash_advice.tsv as a line the loader +# then rejects. After a restart the persistence worker crash-loops on the parse +# error, stashes_ready is never set, and the ENTIRE stash subsystem is locked +# out. This test documents the defect; it is expected to fail once the defect is +# fixed (the empty entry should be rejected at set time or round-trip cleanly). + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf( + 'postgresql.conf', + qq{shared_preload_libraries = 'pg_plan_advice, pg_stash_advice' +pg_stash_advice.persist = true +pg_stash_advice.persist_interval = 0}); +$node->start; + +$node->safe_psql('postgres', 'CREATE EXTENSION pg_stash_advice;'); + +# Stash an empty (non-NULL) advice string. It is accepted with no complaint. +$node->safe_psql( + 'postgres', qq{ + SELECT pg_create_advice_stash('s'); + SELECT pg_set_stashed_advice('s', 12345, ''); +}); +is( $node->safe_psql( + 'postgres', q{SELECT count(*) FROM pg_get_advice_stash_contents('s')}), + '1', + 'empty-advice entry accepted and present before restart'); + +# Restart: the shutdown write emits the entry, and startup tries to reload it. +my $logstart = -s $node->logfile; +$node->restart; + +# DEFECT: the worker cannot parse the file it just wrote, and exits. +$node->wait_for_log(qr/syntax error in file .*pg_stash_advice\.tsv/, $logstart); + +# DEFECT: because the reload never succeeded, stash modifications are locked +# out -- the whole subsystem is unusable, not just the one bad entry. +my ($stdout, $stderr); +my $rc = $node->psql( + 'postgres', + q{SELECT pg_create_advice_stash('s2')}, + stdout => \$stdout, + stderr => \$stderr); +isnt($rc, 0, 'stash modification fails after the rejected reload'); +like( + $stderr, + qr/has not been loaded yet/, + 'DEFECT: an empty stashed advice string bricks the stash subsystem after restart' +); + +# And no stashes are visible. +is( $node->safe_psql('postgres', q{SELECT count(*) FROM pg_get_advice_stashes()}), + '0', + 'no stashes visible after the rejected reload'); + +$node->stop; + +done_testing(); diff --git a/contrib/pg_stash_advice/t/003_defect_persist_off.pl b/contrib/pg_stash_advice/t/003_defect_persist_off.pl new file mode 100644 index 0000000..e522b21 --- /dev/null +++ b/contrib/pg_stash_advice/t/003_defect_persist_off.pl @@ -0,0 +1,60 @@ +# Copyright (c) 2016-2026, PostgreSQL Global Development Group + +# Finding 3 (major) from defect-report-pg-advice.md: calling +# pg_start_stash_advice_worker() on a server booted with pg_stash_advice.persist +# = off deletes an existing pg_stash_advice.tsv without ever loading it, silently +# and permanently losing all previously persisted advice. This test documents +# the defect; it is expected to fail once the defect is fixed (the file should be +# preserved when persistence is off). + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf( + 'postgresql.conf', + qq{shared_preload_libraries = 'pg_plan_advice, pg_stash_advice' +pg_stash_advice.persist = true +pg_stash_advice.persist_interval = 0}); +$node->start; + +$node->safe_psql('postgres', 'CREATE EXTENSION pg_stash_advice;'); + +# Populate a stash, then stop so the shutdown write persists it to disk. +$node->safe_psql( + 'postgres', qq{ + SELECT pg_create_advice_stash('s1'); + SELECT pg_set_stashed_advice('s1', 12345, 'SeqScan(t)'); +}); +$node->stop; + +my $tsv = $node->data_dir . '/pg_stash_advice.tsv'; +ok(-f $tsv, 'dump file written while persistence was on'); + +# Restart with persistence OFF, then merely start the worker, then stop. +# pg_start_stash_advice_worker() blocks until the worker is running, so the +# subsequent stop reliably triggers the worker's shutdown write. +$node->adjust_conf('postgresql.conf', 'pg_stash_advice.persist', 'off'); +$node->start; +$node->safe_psql('postgres', 'SELECT pg_start_stash_advice_worker()'); +$node->stop; + +# DEFECT: the worker deleted the file it never loaded. +ok( !-f $tsv, + 'DEFECT: pg_start_stash_advice_worker() with persist=off destroyed the dump file' +); + +# Restore persistence and confirm the previously persisted stash is gone. +$node->adjust_conf('postgresql.conf', 'pg_stash_advice.persist', 'on'); +$node->start; +is( $node->safe_psql('postgres', q{SELECT count(*) FROM pg_get_advice_stashes()}), + '0', + 'DEFECT: previously persisted stash is permanently lost'); +$node->stop; + +done_testing(); diff --git a/defect-report-pg-advice.md b/defect-report-pg-advice.md new file mode 100644 index 0000000..3218a75 --- /dev/null +++ b/defect-report-pg-advice.md @@ -0,0 +1,271 @@ +# User-visible defects in the pg_*_advice modules still present in master + +**Commits:** 5883ff30b and 6455e55b0 (contrib/pg_plan_advice) and e8ec19aa3 and c10edb102 (contrib/pg_stash_advice), all Robert Haas. +**Master at time of audit:** c1702cb +**Method:** 40 area-focused finders produced 48 raw findings; deduplicated to 22; the top 20 were each judged by two independent adversarial verifiers — a three-gate gatekeeper (still in master / attributable to these four commits / user-visible) and a mechanism refuter, both instructed to default to "refuted". A finding is **confirmed** only when both verifiers judged it real, and **plausible** when exactly one did. Confirmed candidates were then reproduced (or refuted) on a live 20devel cluster built from this tree, in two passes: an initial pass (findings 1–3) and a follow-up pass that (a) re-tested the join-order/GEQO finding with a stronger repro and flipped it from *withdrawn* back to *confirmed* (finding 4), (b) added the prepared-statement/generic-plan finding (finding 5), and (c) tested and **refuted** the hypothesis that `compute_query_id=auto` silently disables stashed advice (see "Additional checks" below). + +Findings are numbered consecutively in presentation order. Line numbers refer to current master. + +--- + +## Live-cluster verification + +Reproduced on a running PostgreSQL 20devel cluster built from this tree (meson, `build/tmp_install`), preloading `pg_plan_advice` / `pg_stash_advice` as each finding required, driven with `psql -X`. + +| # | Static verdict | Live result | Observed | +|---|---|---|---| +| 1 | confirmed | **CONFIRMED** | After `pg_set_stashed_advice('s',12345,'')` and a restart, the worker crash-loops: `ERROR: syntax error in file "pg_stash_advice.tsv" line 2: expected advice string`, exit code 1. `pg_get_advice_stashes()` then returns 0 rows and `pg_create_advice_stash('s2')` fails `ERROR: stash modifications are not allowed because "pg_stash_advice.tsv" has not been loaded yet`. Control with non-empty advice round-trips cleanly. | +| 2 | confirmed | **CONFIRMED** | `EXPLAIN` of a 3-table join under a repeated-identifier `JOIN_ORDER` list: N=25000 planned in 7.6 s, N=50000 in 29.4 s (≈4× for 2×N — quadratic); N=100000 did not finish in 2 min. With `statement_timeout='2s'` the cancel fired only after 6 s / 17.6 s / 59.8 s for N=25k/50k/100k — the enforcement lag itself grows quadratically. Controls (no advice, or `JOIN_ORDER(a b c)`) plan in ~1 ms. | +| 3 | confirmed | **CONFIRMED** | Populate + persist a stash with `persist=on`; restart with `persist=off`; `pg_start_stash_advice_worker()`; stop. `$PGDATA/pg_stash_advice.tsv` is gone (`ls: No such file or directory`). Restarting with `persist=on` reloads 0 rows — the persisted `s1/12345` entry is permanently lost. | +| 4 | confirmed | **CONFIRMED** (corrects an earlier withdrawal) | Star join, forced expensive order `JOIN_ORDER(d4 f d3 d2 d1)`. `geqo=off`: honored, `/* matched */`, cost 10632. `geqo=on, geqo_threshold=2`: **all five seeds ignore the advice**, revert to the cheap order `d1 f d2 d3 d4` (cost 596), plan flagged `Disabled: true` throughout, feedback `/* matched, failed */`. At default `feedback_warnings=off` there is **no warning** — the plan silently contradicts the advice. | +| 5 | confirmed | **CONFIRMED** | `PREPARE`d statement, `plan_cache_mode=force_generic_plan`: cache the generic plan, then `SET pg_plan_advice.advice='SEQ_SCAN(t)'` → plan unchanged (advice silently ignored). `DEALLOCATE`+re-`PREPARE` with the same advice → Seq Scan (proving the staleness is the cache). Reverse: cache *with* advice, `RESET` it → plan still applies the now-cleared advice. | + +**Bottom line:** all five live-tested findings reproduce. Finding 4 was initially withdrawn after an under-powered live test; a stronger repro in the follow-up pass reproduced it and it is restored as a confirmed major defect (see its entry for what the first test missed). + +### Additional checks in this pass + +- **`compute_query_id=auto` does *not* silently disable stashed advice — hypothesis refuted.** A prior completeness critic suspected that, because the server default is `compute_query_id=auto`, `pgsa_advisor` (pg_stash_advice.c:165) would see `queryId==0` and no-op silently. Live test: with the default `auto` and `pg_stash_advice` preloaded, `EXPLAIN VERBOSE` shows a non-zero Query Identifier and stashed `SEQ_SCAN(t)` advice *is* applied (plan flips to Seq Scan, `SEQ_SCAN(t) /* matched */`). The reason is `pg_stash_advice.c:93–94`, which calls `EnableQueryId()` in `_PG_init` precisely so that `auto` yields query IDs. Only an *explicit* `compute_query_id=off` produces the silent no-op — arguably expected, though pgstashadvice.sgml does not spell out that turning query IDs off disables all automatic advice. Not a defect as originally framed; noted here for the record. + +--- + +## Confirmed — major + +### 1. An empty (non-NULL) stashed advice string persists a TSV line the loader rejects, permanently bricking the whole stash subsystem after restart — LIVE-VERIFIED ✓ + +**Where:** `contrib/pg_stash_advice/stashpersist.c:419` (writer at 751–754; reader NULL-return at 616–617) + +**Symptom:** After `pg_set_stashed_advice('s',<id>,'')` — accepted silently, because `''` is not SQL NULL — and any restart, the persistence worker fails every startup with `ERROR: syntax error in file "pg_stash_advice.tsv" line N: expected advice string`, exits, and is relaunched every ~60 s forever. `stashes_ready` is never set, so the *entire* stash subsystem is inaccessible: `pg_get_advice_stashes()` / `pg_get_advice_stash_contents()` return 0 rows, no stashed advice is applied to any query, and `pg_create_advice_stash` / `pg_drop_advice_stash` / `pg_set_stashed_advice` all fail with `stash modifications are not allowed because "pg_stash_advice.tsv" has not been loaded yet`. Recovery requires manually deleting the file, which loses every stash. + +**Repro:** `shared_preload_libraries='pg_plan_advice, pg_stash_advice'`, `persist=on` (default); `CREATE EXTENSION pg_stash_advice; SELECT pg_create_advice_stash('s'); SELECT pg_set_stashed_advice('s',12345,'');` then restart. The worker crash-loops on the parse error and `SELECT pg_create_advice_stash('s2')` then errors with the lockout message. + +**Mechanism:** `pg_set_stashed_advice` special-cases only SQL NULL (stashfuncs.c:313–318); `''` is stored via `dsa_allocate(strlen("")+1)` (pg_stash_advice.c:684) as a valid, non-`InvalidDsaPointer` pointer. `pgsa_write_entries` skips only `InvalidDsaPointer` (stashpersist.c:740), so the empty entry is written as `entry\t<name>\t<qid>\t` followed by an empty escaped string and `\n` (stashpersist.c:751–754) — a line ending in a bare tab. On reload the 4th `pgsa_next_tsv_field` sees the cursor already at `'\0'` and returns NULL (stashpersist.c:616–617), indistinguishable from a missing field, so stashpersist.c:419–423 raises `ERRCODE_DATA_CORRUPTED`. `pgsa_read_from_disk` runs with no `PG_TRY` and `stashes_ready` is set only after it returns (stashpersist.c:145–147); with `persist=true` that flag is otherwise never set, so the `bgworker` `proc_exit(1)` (restart interval `BGW_DEFAULT_RESTART_INTERVAL` = 60 s) leaves `stashes_ready` clear forever, and `pgsa_check_lockout` (pg_stash_advice.c:330–334) blocks all writes. The writer emits a record its own reader cannot parse; the empty-advice case is untested by `t/001_persist.pl`. + +**Verifier notes:** Both verifiers confirmed all three gates. Live-reproduced against a 20devel cluster at HEAD c1702cb: the persisted line was `entry⇥s⇥12345⇥` (bare trailing tab); the worker logged `ERROR: syntax error in file "pg_stash_advice.tsv" line 2: expected advice string` / exit code 1; post-restart `pg_get_advice_stashes()` returned 0 rows and `pg_create_advice_stash('s2')` hit the lockout error. The control (non-empty advice `'SeqScan(foo)'`) wrote a well-formed field, reloaded, and round-tripped with no lockout. User-reachable by any role granted EXECUTE on `pg_set_stashed_advice`. + +### 2. Quadratic, uninterruptible planner CPU blowup applying hostile JOIN_ORDER advice (unprivileged planner DoS) — LIVE-VERIFIED ✓ + +**Where:** `contrib/pg_plan_advice/pgpa_planner.c:1264` + +**Symptom:** A backend spins in the planner for seconds to hours — Θ(N²) in the number N of identifiers in a `JOIN_ORDER` advice list — planning an ordinary 3+-table query. `pg_plan_advice.advice` is PGC_USERSET and its only validation is grammar, so any user in a session where the module is loaded can trigger it. There is no `CHECK_FOR_INTERRUPTS` anywhere in the module, so each `pgpa_join_order_permits_join` call runs its full O(N²) work before any cancel or `statement_timeout` can fire. + +**Repro:** Tables `a,b,c` (one aliased `a`); `SET pg_plan_advice.advice = 'JOIN_ORDER(' || repeat('a ',300000) || 'a)'; EXPLAIN SELECT * FROM a,b,c WHERE a.i=b.i AND b.i=c.i;` — multi-minute hang scaling quadratically with the repeat count. + +**Mechanism:** The advice's only check hook runs `pgpa_parse` for grammar; the parser accepts an arbitrarily long `JOIN_ORDER` list with no dedup or length cap, and a bare identifier defaults to occurrence 1, so `a a … a` yields N children all equal to `a#1`. For each planned query `pgpa_join_order_permits_join` reaches the prefix loop at pgpa_planner.c:1264–1289: for `outer_length = 1..length` it does `list_copy_head` (O(outer_length)) and `pgpa_identifiers_match_target` (O(outer_length)). With the list one alias repeated N times and a considered join whose outer side is `{a,b}`, every prefix marks all `a` targets used but never covers `b`, so `pgpa_identifiers_match_target` returns `PGPA_ITM_TARGETS_ARE_SUBSET` on every iteration; neither the `PGPA_ITM_EQUAL` break (1276) nor the `!= TARGETS_ARE_SUBSET` `DENIED` early-out (1287) ever fires, and the loop runs all N iterations at O(outer_length) each — Θ(N²) in a single call. + +**Verifier notes:** Both verifiers confirmed at high confidence; the refuter specifically closed the only escape (incrementing occurrence numbers would hit the DENIED early-out, but occurrence defaults to 1). Live-reproduced: N=25000 planned in 7.6 s, N=50000 in 29.4 s (≈3.9× for 2×N); N=100000 exceeded 2 min. With `statement_timeout='2s'` the cancel fired only after 6 s / 17.6 s / 59.8 s for N=25k/50k/100k — the lag itself scales quadratically, so a 2 s cap does not bound the stall. One precision on the static claim: a `CHECK_FOR_INTERRUPTS` in core join search does eventually fire *between* the coarse-grained `pgpa_join_order_permits_join` calls, so it is not strictly uninterruptible, but each individual call runs to completion with no interrupt check, making cancellation latency unbounded in N. Controls (no advice; `JOIN_ORDER(a b c)`) planned in ~1 ms. + +### 3. pg_start_stash_advice_worker() destroys an existing pg_stash_advice.tsv when persistence was off at boot, silently losing all persisted advice — LIVE-VERIFIED ✓ + +**Where:** `contrib/pg_stash_advice/stashfuncs.c:344` (root cause the skipped load at stashpersist.c:145–149 plus the unconditional shutdown write at stashpersist.c:214, whose unlink path is 541–545) + +**Symptom:** On a server started with `pg_stash_advice.persist=off`, calling `pg_start_stash_advice_worker()` overwrites or deletes the on-disk `pg_stash_advice.tsv`, permanently losing every previously persisted stash and entry — even though the file was never loaded into memory. + +**Repro:** Boot preloaded with `persist=on`; create and populate a stash; stop (file written). Restart with `persist=off`; `SELECT pg_start_stash_advice_worker();` then stop. `pg_stash_advice.tsv` is now gone; restarting with `persist=on` loads nothing. + +**Mechanism:** With `persist` false, `pgsa_init_shared_state` sets `stashes_ready` at init (pg_stash_advice.c:599–600). `pg_start_stash_advice_worker` (stashfuncs.c:344) launches the worker unconditionally, with no persist guard. In the worker the disk-load block runs only while `stashes_ready` is still clear (stashpersist.c:145), so `pgsa_read_from_disk` is skipped and the worker runs with an empty in-memory hash while the file still holds data. At shutdown the worker unconditionally calls `pgsa_write_to_disk` (stashpersist.c:214), which with `members==0` `unlink()`s `PGSA_DUMP_FILE` (stashpersist.c:541–545) and otherwise `durable_rename`s a replacement (stashpersist.c:560). Either way prior advice is gone. The `stashes_ready` lockout that protects the normal path does not apply here because it was already set at init. + +**Verifier notes:** Both verifiers confirmed (each at medium confidence, arguing only that the operator sequence is unusual and that `persist=off` could be read as "I don't care about the file"). Live-reproduced: after Phase 1 the file existed and read `stash⇥s1` / `entry⇥s1⇥12345⇥SeqScan(foo)`; after Phase 2 (`persist=off`, `pg_start_stash_advice_worker()`, stop) `ls` reported the file gone; a Phase 3 restart with `persist=on` showed `pg_get_advice_stashes()` = 0 rows. See the discussion of whether this is intentional at the end of this report. + +### 4. JOIN_ORDER advice is silently ignored under GEQO because GEQO scores tours by total_cost only, not disabled_nodes — LIVE-VERIFIED ✓ + +**Where:** enforcement in `contrib/pg_plan_advice/pgpa_planner.c` (clears `PGS_JOIN_ANY` for non-conforming joinrels, expressed as `disabled_nodes`, not a hard path suppression); defeated by `src/backend/optimizer/geqo/geqo_eval.c:115`. + +**Symptom:** With GEQO active (`geqo=on` and the relation count at/over `geqo_threshold`), a query plan can use a join order different from the one the user supplied via `JOIN_ORDER` advice. No error is raised; with the default `feedback_warnings=off` there is no warning either — the plan silently contradicts the advice. This defeats the feature's headline use case, the 12+-table star-schema join that GEQO is specifically responsible for. + +**Repro:** Star schema — fact `f` (200k rows) joined to dims `d1..d4` (1000 rows each) on `f.kN=dN.id`, with dim filters of sharply different selectivity (`d1.b=1` matches 0.1%, `d4.b=1` matches 100%) so join order matters a lot. Force the expensive order that applies the selective dim last: +`SET pg_plan_advice.advice='JOIN_ORDER(d4 f d3 d2 d1)';` +`EXPLAIN SELECT * FROM f JOIN d1 ON f.k1=d1.id JOIN d2 ON f.k2=d2.id JOIN d3 ON f.k3=d3.id JOIN d4 ON f.k4=d4.id WHERE d1.b=1 AND d2.b=1 AND d3.b=1 AND d4.b=1;` +`geqo=off` → the requested order, `/* matched */`, cost ~10632. `geqo=on, geqo_threshold=2` → the cheap order `d1 f d2 d3 d4`, `/* matched, failed */`, cost ~596, every node `Disabled: true`, and (default settings) no warning. + +**Mechanism:** For a joinrel that does not conform to the advice, pg_plan_advice clears the join-strategy bits from `joinrel->pgs_mask` (the `*pgs_mask_p &= ~PGS_JOIN_ANY` family in pgpa_planner.c, ~1144–1148). In the core join code this does **not** suppress path generation: only mergejoin (`joinpath.c:242`) and hashjoin (`joinpath.c:353`) are hard-gated on the mask, while nestloop paths are produced by `match_unsorted_outer`, which is gated on `mergejoin_allowed` — a local that defaults to `true` (`joinpath.c:133`) and is only *lowered* by `select_mergejoin_clauses`, which the cleared mask skips. So `match_unsorted_outer` still runs, `try_nestloop_path` calls `initial_cost_nestloop` (which sets `workspace.disabled_nodes` from the mask) and then `add_path` unconditionally (`joinpath.c:966–975`). The non-conforming joinrel therefore has a *feasible* nestloop path carrying `disabled_nodes ≥ 1`. The ordinary planner still prefers the conforming order because `add_path` orders by `(disabled_nodes, cost)` — but GEQO does not: `geqo_eval.c:113–115` sets `fitness = joinrel->cheapest_total_path->total_cost`, reading only `total_cost` and never `disabled_nodes` (the `fitness = DBL_MAX` at line 118 fires only when the tour is geometrically infeasible, i.e. `joinrel == NULL`, which never happens here because every f-first order is a valid star join). A cheaper advice-violating tour thus outscores the costlier conforming tour and is returned. The module installs no mitigation — it neither forces `geqo=off` when join-order advice is present, nor hard-suppresses non-conforming paths, nor teaches GEQO about `disabled_nodes` — so its "the planner cannot reverse course" guarantee does not hold under GEQO. + +**Verifier notes:** Both static verifiers confirmed the finding at the outset. An initial live test then *refuted* it and the finding was withdrawn on the theory that clearing `PGS_JOIN_ANY` "suppresses all join-path generation" so violating tours score `DBL_MAX`. That theory is wrong (see the mechanism: nestloop paths survive as `disabled_nodes`-flagged), and the first live test was under-powered — it supplied only *achievable, near-cost-optimal* advice orders, for which the conforming order is also the cheapest, so no violating tour could win. The one "failed" case it saw was a clauseless cross-join order that is infeasible with GEQO on *and* off, which it correctly excluded — but that is not the bug. The follow-up test above uses a forced *expensive-but-feasible* order and reproduces the silent violation deterministically across five `geqo_seed` values. GEQO's cost-only fitness has been long-standing core behavior; the defect is that pg_plan_advice's enforcement mechanism (soft-disable via `disabled_nodes`) is the one enforcement path GEQO ignores, and the module ships no guard for it. Note the irony that in this repro ignoring the advice happens to yield the *better* plan — but the failure mode that matters is the inverse: advice given to correct a planner cost misestimate is silently discarded under GEQO, which is exactly when the user most needs it. + +### 5. pg_plan_advice.advice / stash advice changes are silently ignored by an already-cached generic plan (prepared statements) — LIVE-VERIFIED ✓ + +**Where:** `contrib/pg_plan_advice/pg_plan_advice.c:70` (the `pg_plan_advice.advice` GUC) and `contrib/pg_stash_advice/pg_stash_advice.c:164` (`pgsa_advisor`); neither module registers any plan-cache invalidation. + +**Symptom:** Once a prepared statement's generic plan is cached, changing `pg_plan_advice.advice` (or `pg_stash_advice.stash_name`, or the stashed advice for that queryId) has no effect on that statement: newly-supplied advice is silently ignored, and advice supplied *before* the plan was cached keeps shaping it even after the user `RESET`s it. The user sets or clears advice and the plan does not move, with no error and no warning. + +**Repro (both directions):** `PREPARE ps AS SELECT * FROM t WHERE v=$1; SET plan_cache_mode=force_generic_plan;` +- Forward (new advice ignored): `EXPLAIN EXECUTE ps(7)` caches a Bitmap/Index plan; `SET pg_plan_advice.advice='SEQ_SCAN(t)'; EXPLAIN EXECUTE ps(7)` → still Bitmap. `DEALLOCATE ps` + re-`PREPARE` + `EXPLAIN EXECUTE` → Seq Scan, proving the advice is valid and only the cache was stale. +- Reverse (old advice sticks): with advice set, `PREPARE`/`EXECUTE` caches a Seq Scan plan; `RESET pg_plan_advice.advice; EXPLAIN EXECUTE` → still Seq Scan. + +**Mechanism:** Generic plans are built once and cached by the core plan cache and reused across executions; a bare GUC change is not an invalidation event, so the cached plan is not re-planned. `pg_plan_advice` supplies advice only during planning (via its planner hook) and registers no callback to discard cached plans when `pg_plan_advice.advice` changes; `pg_stash_advice`'s `pgsa_advisor` likewise feeds advice in at plan time and keys on `parse->queryId`, which is identical across executions of one prepared statement, so a stash entry set after the generic plan is cached is never consulted for it. This is the same class as core planner GUCs such as `enable_seqscan` not invalidating prepared statements — but it is newly relevant here because the entire purpose of these GUCs is to force plan choices, the module's own `always_store_advice_details` help text ("Use this option to see generated advice for prepared queries") shows prepared statements are in scope, and neither pgplanadvice.sgml nor pgstashadvice.sgml warns that advice does not take effect on an already-cached generic plan. + +**Verifier notes:** Reproduced live on a 20devel cluster at HEAD c1702cb (see finding 5 in the live table). Attribution caveat: the caching/invalidation behavior is core plan-cache mechanism, not a fault inside the four contrib commits; the defect is that the advice feature layers on top of it without an invalidation hook or any documentation of the interaction, producing "advice silently ignored / plan contradicts current advice" — a core failure mode for a plan-forcing feature. A workaround exists (`plan_cache_mode=force_custom_plan`, or re-`PREPARE`), which is why this is reported as major-by-impact but with the mechanism honestly attributed to shared core behavior. + +### 6. SEMIJOIN_NON_UNIQUE advice on a semijoin the planner cannot unique-ify is reported "matched, failed" and warns, though the chosen plan honors it + +**Where:** `contrib/pg_plan_advice/pgpa_walker.c:134` (filter at 131–137) + +**Symptom:** For a semijoin with a non-equality correlation (e.g. `EXISTS (SELECT 1 FROM b WHERE b.y > a.x)`), `SEMIJOIN_NON_UNIQUE(b)` yields feedback `SEMIJOIN_NON_UNIQUE(b) /* matched, failed */` and, with `feedback_warnings=on`, `WARNING: supplied plan advice was not enforced` — yet the plan is exactly the requested non-unique Semi Join. The feedback (a core promise of the feature) is a false negative and the warning a false positive. + +**Repro:** `CREATE TABLE a(x int); CREATE TABLE b(y int);` 100 rows each; `ANALYZE; SET pg_plan_advice.feedback_warnings=on; SET pg_plan_advice.advice='semijoin_non_unique(b)'; EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM a WHERE EXISTS (SELECT 1 FROM b WHERE b.y > a.x);` shows `matched, failed` plus the WARNING, though the Semi Join matches the advice. Replacing `b.y > a.x` with `b.y = a.x` yields the clean `/* matched */`. + +**Mechanism:** For a non-equality clause `create_unique_paths` returns NULL, so `JOIN_UNIQUE_INNER/OUTER` is never tried and `pgpa_join_path_setup` never records the rel in `sj_unique_rels`, leaving `sj_unique_rtis` empty. The final `JOIN_SEMI` still marks the supplied `SEMIJOIN_NON_UNIQUE(b)` entry `PGPA_FB_MATCH_FULL` (the join is permitted and the plan is the requested Semi Join). But the plan-derived `SEMIJOIN_NON_UNIQUE` feature is dropped at finalization by the `list_member(sj_unique_rtis, ...)` filter over the now-empty list (pgpa_walker.c:131–137), so `pgpa_walker_would_advise` → `pgpa_walker_contains_feature` returns false, `pgpa_planner_append_feedback` sets `PGPA_FB_FAILED`, and the warning fires. The filter is correct for suppressing redundant *generated* advice but wrong to apply to feedback on *supplied* advice. + +**Verifier notes:** Both verifiers confirmed at high confidence. The existing regression `expected/semijoin.out:81` covers only the equality (uniquifiable) case, which correctly prints `/* matched */`, so this non-equality path is untested. Not run on a live cluster (capacity). + +### 7. JOIN_ORDER advice with an unordered {…} sublist is honored by the planner but reported "matched, failed" with a spurious feedback warning + +**Where:** `contrib/pg_plan_advice/pgpa_walker.c:999` (guarded by the `unrolled_join`/`ttype` test at 996–999) + +**Symptom:** `JOIN_ORDER` advice containing an unordered `{…}` sublist — the syntax the README invites for a join whose sides are unspecified — is reported FAILED even when the plan honors it exactly: `EXPLAIN (PLAN_ADVICE)` prints `/* matched, failed */` and, with `feedback_warnings=on`, warns for every such query. The identical plan under the parenthesized form reports plain `/* matched */`. + +**Repro:** `jo_fact/jo_dim1/jo_dim2` from `sql/join_order.sql`; `SET max_parallel_workers_per_gather=0; SET pg_plan_advice.feedback_warnings=on; SET pg_plan_advice.advice='join_order(f {d1 d2})'; EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM jo_fact f LEFT JOIN jo_dim1 d1 ON f.dim1_id=d1.id LEFT JOIN jo_dim2 d2 ON f.dim2_id=d2.id WHERE val1=1 AND val2=1;` prints `JOIN_ORDER(f {d1 d2}) /* matched, failed */` plus a WARNING, though the plan conforms; the `(d1 d2)` form on the same plan reports plain `matched`. + +**Mechanism:** Enforcement and feedback disagree. At plan time `pgpa_join_order_permits_join` treats `{…}` permissively (`PGPA_JO_INDIFFERENT`/`PERMITTED`, sets `MATCH_FULL`). At feedback time, for a plan member that is a real sub-join (`member->unrolled_join != NULL`) whose target is `PGPA_TARGET_UNORDERED_LIST` rather than `ORDERED_LIST`, `pgpa_walker.c:998–999` returns false unconditionally, so `pgpa_walker_would_advise` returns false, `PGPA_FB_FAILED` is set, and `pgpa_trove_append_flags` renders the contradictory `matched, failed`. An `unrolled_join` member should match an unordered target when the sub-join's relid set equals the target's, mirroring the permissive enforcement. + +**Verifier notes:** Both verifiers confirmed. Master's own `expected/join_order.out` bakes in the buggy output: line 184 shows `JOIN_ORDER(f {d1 d2}) /* matched, failed */` while line 156 shows `JOIN_ORDER(f (d1 d2)) /* matched */` for the identical plan. Since `{d1 d2}` is strictly more permissive than `(d1 d2)`, the `failed`/warning on the same honoring plan is spurious. Not run on a live cluster (capacity). + +--- + +## Confirmed — minor + +### 8. pg_set_stashed_advice stores advice with no syntax validation, so malformed advice is accepted then emits a plan-time WARNING on every execution and applies nothing — LIVE-OBSERVED ✓ + +**Recommended quick fix (low-hanging fruit).** The user impact is bounded and self-inflicted (you have to stash bad advice yourself), so this is *minor* by blast radius — but it is a high-annoyance, low-effort UX fix worth prioritizing above the other minors: validate the advice string with `pgpa_parse` inside `pg_set_stashed_advice` and reject it at set time, exactly as the `pg_plan_advice.advice` GUC already does. the recurring plan-time warning is an avoidable annoyance for the querying user, who typically cannot fix it themselves; the fix is a few lines at the one choke point. + +**Where:** `contrib/pg_stash_advice/stashfuncs.c:316` + +**Symptom:** `pg_set_stashed_advice('s',qid,'<malformed>')` succeeds silently, but the advice never applies. Every session whose `stash_name` points at that stash and that plans the matching queryId gets `WARNING: could not parse supplied advice` on every execution (non-prepared queries re-plan each time). The user hitting the warning usually lacks EXECUTE on `pg_set_stashed_advice` and cannot fix it, and there is no GUC to silence it — inconsistent with `SET pg_plan_advice.advice = '<same string>'`, which rejects it immediately at SET time. + +**Repro:** `SELECT pg_create_advice_stash('s'); SELECT pg_set_stashed_advice('s',<qid>,'NOTATAG(x');` succeeds. `SET pg_stash_advice.stash_name='s';` then run the query → `could not parse supplied advice` WARNING each plan, plan unchanged. Contrast `SET pg_plan_advice.advice='NOTATAG(x'` → immediate ERROR. + +**Mechanism:** `pg_set_stashed_advice` copies the text (stashfuncs.c:313) and stores it verbatim via `pgsa_set_advice_string` (stashfuncs.c:316) with no `pgpa_parse` check (`pgsa_check_stash_name` validates only the NAME). The GUC path validates in `pg_plan_advice_advice_check_hook` (pg_plan_advice.c:436). At plan time `pgsa_advisor` hands the raw string to `pgpa_planner_setup`, which on parse failure only `ereport(WARNING)` and leaves `advice_items` NIL (pgpa_planner.c:244–247), so no trove is built. + +**Verifier notes:** Both verifiers confirmed at medium confidence: the plan-time WARNING is a deliberate safety net for advisor-hook-supplied advice (comment at pgpa_planner.c:236–243), and the persist-reload path also loads unvalidated strings — so whether to validate at set time is a design judgment. The factual mechanism (silent acceptance, per-plan WARNING, advice silently ineffective, asymmetry with the GUC path) is solid. Live-observed incidentally during this pass's Test A: a stashed `'SeqScan(t)'` (wrong tag name for `SEQ_SCAN`) was accepted by `pg_set_stashed_advice` and produced `WARNING: could not parse supplied advice: syntax error at or near "SeqScan"` at plan time. (A set-time validator must still permit the disk-reload path to tolerate legacy strings, or must validate on write to disk too.) + +### 9. Scan-type advice naming a partitioned table's parent disables its Append and is applied to no scan + +**Where:** `contrib/pg_plan_advice/pgpa_planner.c:1851` + +**Symptom:** With `SEQ_SCAN(p)` (or `INDEX_SCAN`/`BITMAP_HEAP_SCAN`/`TID_SCAN`) where `p` is a partitioned table, `EXPLAIN` shows the top Append/MergeAppend flagged `Disabled: true`, the scan advice is applied to no actual scan node, and (with `feedback_warnings`) it reports `matched, failed`. The plan is needlessly degraded and the advice silently ignored. + +**Repro:** `CREATE TABLE p(id int) PARTITION BY RANGE(id); CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (0) TO (100); CREATE TABLE p2 PARTITION OF p FOR VALUES FROM (100) TO (200); SET pg_plan_advice.advice='SEQ_SCAN(p)'; EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM p;` — Append `Disabled: true`; advice `matched, failed`. + +**Mechanism:** `pgpa_build_simple_rel` fires for the partitioned parent baserel, whose identifier (alias `p`, `partrel=NULL`) matches the unqualified target, so `pgpa_planner_apply_scan_advice` runs on the parent. For a positive tag `scan_type` is a single bit, and enforcement at pgpa_planner.c:1850–1851 (`rel->pgs_mask &= ~(all_scan_mask & ~scan_type)`, where `all_scan_mask` includes `PGS_APPEND|PGS_MERGE_APPEND`) clears the append bits on the parent appendrel. `cost_append` reads that mask, sets `disabled_nodes=1`, and the Append (the only path) is chosen while disabled. Child partitions carry a non-NULL `partrel`, so `(p,NULL,NULL)` never matches them, and the intended scan is enforced on nothing. Round-trip-generated advice avoids this by emitting child-qualified targets. + +**Verifier notes:** Both verifiers confirmed at high confidence. `disabled_nodes` also propagates into enclosing plan-cost comparisons in larger queries. Not run on a live cluster (capacity). + +### 10. Schema-omitted scan/join advice matching two same-named partitions across schemas is reported "matched, failed" though it is enforced on both + +**Where:** `contrib/pg_plan_advice/pgpa_identifier.c:378` (`>1 match` guard at 378–382) + +**Symptom:** For a partitioned table whose partitions share a relation name in different schemas, schema-omitted advice like `SEQ_SCAN(p/child)` is actually enforced on every matching partition, yet `EXPLAIN (PLAN_ADVICE)` shows `/* matched, failed */` and, with `feedback_warnings=on`, emits a spurious `supplied plan advice was not enforced` WARNING. The README explicitly permits omitting the partition schema. + +**Repro:** `CREATE TABLE p(a int) PARTITION BY RANGE(a); CREATE SCHEMA s1; CREATE SCHEMA s2; CREATE TABLE s1.child PARTITION OF p FOR VALUES FROM (0) TO (10); CREATE TABLE s2.child PARTITION OF p FOR VALUES FROM (10) TO (20); SET pg_plan_advice.feedback_warnings=on; SET pg_plan_advice.advice='SEQ_SCAN(p/child)'; EXPLAIN (PLAN_ADVICE, COSTS OFF) SELECT * FROM p;` — both partitions seq-scanned yet feedback `matched, failed` plus WARNING. + +**Mechanism:** Enforcement (`pgpa_identifier_matches_target`, pgpa_ast.c:260–262) treats a NULL `partnsp` as a wildcard, so both partitions hit the shared trove entry and it is flagged `MATCH_FULL`. Feedback resolves the single target via `pgpa_compute_rti_from_identifier`, whose same wildcard matches BOTH RTIs, hitting the `>1 match` guard that returns 0 (pgpa_identifier.c:378–382). `would_advise` then treats `rti==0` as nonexistent and returns false, so the entry is flagged `PGPA_FB_FAILED`. The feedback path cannot represent the enforcement path's multiplicity. + +**Verifier notes:** Both verifiers confirmed at high confidence. Generated advice always includes the schema and thus round-trips; only hand-written schema-omitted advice on multiple same-named partitions triggers it. Not run on a live cluster (capacity). + +### 11. DO_NOT_SCAN feedback compares the partition schema exactly, inconsistent with the wildcard rule used everywhere else, giving a spurious "failed"/warning + +**Where:** `contrib/pg_plan_advice/pgpa_walker.c:765` + +**Symptom:** Schema-omitted `DO_NOT_SCAN` advice on a partition child that was genuinely excluded (e.g. via a discarded AlternativeSubPlan / MinMaxAgg alternative) is reported `/* matched, failed */` and warns under `feedback_warnings`, even though the partition really was excluded as requested. Generated advice always includes the schema, so only hand-written schema-omitted advice triggers this. + +**Repro:** Build a query where `DO_NOT_SCAN` excludes a partition child via a discarded alternative subplan / MinMaxAgg loser, then supply `DO_NOT_SCAN(alias/partition)` with the schema omitted; feedback reports `failed` although the exclusion succeeded; adding the schema reports `matched`. + +**Mechanism:** `do_not_scan_identifiers` are populated from discarded alternatives and always carry a populated `partnsp`. The `DO_NOT_SCAN` branch of `pgpa_walker_would_advise` compares stored `rid->partnsp` against `target->rid.partnsp` with `strings_equal_or_both_null` (pgpa_walker.c:765–766), so an omitted (NULL) schema never matches a stored `public`, returning false → `PGPA_FB_FAILED`. Everywhere else — enforcement (pgpa_ast.c:260–262) and other-tag feedback (pgpa_identifier.c:373–374) — an omitted partition schema is a wildcard. + +**Verifier notes:** Both verifiers confirmed at medium confidence: the code inconsistency is unambiguous, but the manifestation requires the specific reachable state of a partition child inside a discarded alternative, so a clean live repro depends on a partitioned MinMaxAgg / AlternativeSubPlan setup. Not run on a live cluster (capacity). + +### 12. FOREIGN_JOIN(()) with an empty sublist bypasses the >1-relation arity check and is silently accepted as an inert no-op + +**Where:** `contrib/pg_plan_advice/pgpa_parser.y:138` (check spans 137–140) + +**Symptom:** `SET pg_plan_advice.advice='FOREIGN_JOIN(())'` succeeds, whereas `FOREIGN_JOIN((a))` is rejected with `FOREIGN_JOIN targets must contain more than one relation identifier`, and the docs (pgplanadvice.sgml:294–296) say fewer than two foreign tables is "neither necessary nor permissible". The degenerate zero-rel sublist is not reported as a parse error and produces no hash key, so it is silently ignored during planning and appears only later as `FOREIGN_JOIN(()) /* not matched */` in feedback. + +**Repro:** `LOAD 'pg_plan_advice';` then `SET pg_plan_advice.advice='FOREIGN_JOIN(())';` (succeeds) versus `SET pg_plan_advice.advice='FOREIGN_JOIN((a))';` (errors). + +**Mechanism:** The FOREIGN_JOIN sanity check (pgpa_parser.y:135–140) flags a target only when `target->ttype == PGPA_TARGET_IDENTIFIER || list_length(target->children) == 1`. An empty sublist `()` parses into a `PGPA_TARGET_ORDERED_LIST` with `children==NIL`, so `list_length` is 0 — neither `==1` nor IDENTIFIER — and the check passes. The correct predicate is `< 2`. Downstream the 0-child entry adds no hash entry, so the advice is dropped without warning. + +**Verifier notes:** Both verifiers confirmed at high confidence. The primary, high-confidence defect is the parse-time asymmetry (`FOREIGN_JOIN(())` accepted while `FOREIGN_JOIN((a))` and `FOREIGN_JOIN(a)` are rejected, per `expected/syntax.out:190–195`); the downstream `not matched` behavior is plausible but was verified only by the refuter. Not run on a live cluster (capacity). + +--- + +## Plausible — confirmed by exactly one verifier (not confirmed) + +These were judged real by one adversarial verifier and refuted by the other. They are reported here explicitly as **not confirmed**; each rests on a design/severity judgment rather than a clear code fault, and none was run on a live cluster. + +### 13. Multi-relation SEMIJOIN_UNIQUE/NON_UNIQUE advice on a non-semijoin disables every join path instead of being reported "inapplicable" + +**Where:** `contrib/pg_plan_advice/pgpa_planner.c:1619` + +The gatekeeper (medium) judged the defect *class* real but corrected the repro; the refuter (high) refuted the finding as written. Both agree the finding's own example — `SEMIJOIN_UNIQUE((a b))` on `SELECT * FROM a,b,c WHERE a.id=b.id AND b.id=c.id` — does **not** disable the plan, because `{a,b}` is a buildable joinrel and the orientation with `{a,b}` as the top join's inner side sets `restrict_method=true` and reports `INAPPLICABLE` (pgpa_planner.c:1087–1094), yielding a normal plan reported `matched, inapplicable, failed` (mirroring the tested single-rel case at `expected/semijoin.out:369`). The gatekeeper argued a genuine defect survives only for a *non-adjacent* target such as `SEMIJOIN_UNIQUE((a c))`, where `{a,c}` is a clauseless pair the planner never builds, so every join hits the `return false` at pgpa_planner.c:1619, disabling all join paths without ever marking the advice inapplicable — contradicting pgplanadvice.sgml:636–639. That corrected variant was not verified against a live cluster and rests on static reasoning about planner join-orientation exploration. Reported as plausible pending a live check of the non-adjacent target. + +### 14. Stash-supplied advice silently overrides an explicitly-set pg_plan_advice.advice for the same query; the precedence is documented nowhere + +**Where:** `contrib/pg_plan_advice/pg_plan_advice.c:196` + +The mechanism is factual: `pg_plan_advice_get_supplied_query_advice` iterates advisor hooks first (185–192) and returns the first non-NULL string, so a stash hit unconditionally wins over the GUC, falling through to `return pg_plan_advice_advice` (196) only when every advisor returns NULL. The refuter (medium) called it real; the gatekeeper (medium) refuted it, because precedence must go one way or the other, the code violates no stated contract, and the docs merely omit the interaction. Both note the override is not truly silent: with the default `always_explain_supplied_advice=true`, `EXPLAIN` displays the winning (stash) string under "Supplied Plan Advice" and `pgsa_advisor` logs a DEBUG2 line, so the override is invisible only when the query runs without `EXPLAIN`. Reduces to an undocumented precedence / documentation-completeness gap. + +### 15. Terminal wrap/trailing-newline formatting of advice/feedback strings leaks into structured EXPLAIN (FORMAT JSON/XML/YAML) + +**Where:** `contrib/pg_plan_advice/pg_plan_advice.c:289` + +The mechanism is confirmed: for non-TEXT formats the advice/feedback value is passed straight to `ExplainPropertyText` (287–291); the "Supplied Plan Advice" string carries a trailing `\n` from the `" */\n"` per-item terminator (334–336) and "Generated Plan Advice" carries 76-column wrap `\n` bytes from `pgpa_maybe_linebreak`, all of which `ExplainProperty*` escapes into the JSON/XML/YAML scalar. The gatekeeper (medium) called it a legitimate minor defect; the refuter (high) refuted it as out-of-scope cosmetics — the pass-through is explicitly intended (comment at pg_plan_advice.c:286, and TEXT format deliberately strips the trailing newline at 311–315), the strings round-trip (newline is whitespace in the advice grammar), and the docs make no claim about structured-format representation, so nothing promised is broken. No functional consequence. + +--- + +## Refuted + +### 16. ~~feedback_warnings warns "supplied plan advice was not enforced" for advice that does not pertain to the query at all (e.g. SELECT 1)~~ + +**Where:** `contrib/pg_plan_advice/pgpa_planner.c:1932` + +The mechanism is real — `pgpa_planner_append_feedback` appends every trove entry including `flags==0` ones, and `pgpa_planner_feedback_warning` skips only `flags == (MATCH_PARTIAL|MATCH_FULL)`, so unrelated advice plus `feedback_warnings=on` does warn on `SELECT 1`. But both verifiers refuted it as documented, intended behavior: the docs (pgplanadvice.sgml:758–760) say the warning fires "whenever supplied plan advice is not successfully enforced," advice whose target is absent genuinely was not enforced, and the design-intent comment (pgpa_planner.c:1926–1927) shows only clean full matches are meant to be silent. The finding's premise that "not matched" is documented as benign is unsupported. The cross-query noise stems from session-level `SET` versus `SET LOCAL` on a PGC_USERSET GUC — a configuration choice. + +### 17. ~~DO_NOT_SCAN is completely inert on a Sample Scan (TABLESAMPLE) and on a foreign-table scan, contradicting the documented "marked disabled" fallback~~ + +**Where:** `doc/src/sgml/pgplanadvice.sgml:304` + +The technical mechanism is confirmed — `cost_samplescan` builds its enable mask from 0 and never consults the scan-type bits `DO_NOT_SCAN` clears, and foreign base-rel paths come from `GetForeignPaths()` with a caller-supplied `disabled_nodes` of 0, so neither node is ever marked `Disabled: true`. Both verifiers refuted it as a cosmetic-only, no-functional-impact issue: a sample-scanned or foreign base rel has exactly one possible path, so a `disabled_nodes` offset can never change plan selection; the `matched, failed` feedback is identical to the plain-table case (`expected/scan.out:325`), so the user is honestly told the advice could not be applied; the doc hedges "In most cases," and the following paragraph (pgplanadvice.sgml:312–319) already documents single-scan-method rels as beyond advice's reach. The underlying pgs_mask/sample-scan design is core commit 4020b37, not one of the four contrib commits. + +### 18. ~~feedback_warnings emits recurring warnings for stash advice the querying user never authored and cannot fix~~ + +**Where:** `contrib/pg_plan_advice/pgpa_planner.c:386` + +The causal chain is mechanically real (stash advice flows through the advisor hook into `pgpa_planner_setup`, which forces `generate_advice_feedback=true` whenever `feedback_warnings` is on, and the shutdown warning fires for every non-clean-match entry). Both verifiers refuted it: the warning is a true positive per pgplanadvice.sgml:758–760, and the finding's load-bearing claim — "the user cannot silence it except by disabling functionality" — is false. `pg_plan_advice.feedback_warnings` is a PGC_USERSET, default-false, purely diagnostic GUC; the affected user turned it on and can `SET pg_plan_advice.feedback_warnings=off` to silence it while leaving stash advice fully functional. The absence of a per-source opt-out is a UX/design preference, out of scope. + +--- + +## Unverified (found, ranked below the 20-verifier cap; not adversarially checked) + +### 19. pgpa_relids() ignores CustomScan.custom_relids: a join-substituting custom scan causes XX000 "plan node has no RTIs", or silently vanishes from generated advice (minor) + +**Where:** `contrib/pg_plan_advice/pgpa_walker.c:571` + +Generating advice for a plan containing a `CustomScan` that substitutes for a join (`scanrelid==0`, `custom_relids` naming multiple base rels) is claimed to fail with `ERROR: plan node has no RTIs: <nodetag>` (XX000) when that node is an inner/outer input of an enclosing join, or to silently drop the covered relations from all advice when it is the top node. `pgpa_relids()` handles `ForeignScan` (`fs_relids`) and Append/MergeAppend (`apprelids`) but has no `CustomScan` branch and never consults `custom_relids`, falling through to `return NULL`. Requires a custom-scan provider that replaces a join; `src/test/modules/test_plan_advice` already exists as a C harness that could drive it, but this was not built for this audit. + +### 20. With the default persist=true when preloaded, the entire stash API is permanently locked out in single-user mode (minor) + +**Where:** `contrib/pg_stash_advice/pg_stash_advice.c:599` + +Running `postgres --single` with `pg_stash_advice` preloaded (`persist` defaults true) is claimed to leave `stashes_ready` clear forever — it is set at init only when `persist` is false, and otherwise only by the worker after a load, but no postmaster exists to launch the worker in single-user mode. Every stash-modifying function would then error `stash modifications are not allowed because "pg_stash_advice.tsv" has not been loaded yet`, any existing dump would never load, and `pg_start_stash_advice_worker()` would fail `could not register background process`. Live-testable but not run. + +--- + +## Is finding 3 (worker destroys the TSV) intentional? + +Almost certainly not; estimated ~10–15% chance it is a deliberate design. The destruction is an *emergent* collision of three individually-reasonable behaviors, none written to delete the file when the worker starts under `persist=off`: + +1. **Skip-load-when-`persist`-off.** With `persist=false`, `stashes_ready` is set at init (pg_stash_advice.c:599–600), so the worker's load block (stashpersist.c:145–149) is skipped and it runs with an empty in-memory hash — reasonable in isolation. +2. **Unconditional shutdown write.** The worker always calls `pgsa_write_to_disk` at exit (stashpersist.c:214), with no `persist` guard — reasonable if you assume the worker only runs when persistence is wanted. +3. **Unlink-when-empty.** `pgsa_write_to_disk` deliberately removes the file rather than leaving a zero-length one when `members==0` (stashpersist.c:538–545, with an explanatory comment) — reasonable as cleanup after a legitimately-emptied stash. + +The bug is that `pg_start_stash_advice_worker` (stashfuncs.c:326–347) starts the worker with **no `persist` guard and no comment**, so under `persist=off` behaviors (1)+(2)+(3) combine to delete a file that was never loaded. Three things argue against intent: the unlink-when-empty comment addresses only the emptied-stash case, not this one; the shutdown write is ungated where nearly every other persist action is gated on `persist`; and the commit message for c10edb102 frames `persist` purely as "if true, write the file; on restart, lock out modifications until it reloads" — it never contemplates a `persist=off` worker mutating the file. The single charitable reading (which the verifiers raised at medium confidence) is that `persist=off` could mean "I don't care about that file," so deleting it is acceptable. Even under that reading the behavior is indefensible as *designed*: it deletes without first loading, without a warning, and as a side effect of merely starting a worker — so at minimum it is an undocumented, data-losing surprise. The safe fix is to gate both the worker's shutdown write and `pg_start_stash_advice_worker` on `persist`, or to have the worker load the file before it can ever overwrite it. + +--- + +## Coverage notes + +- **Live-verified (5):** findings 1, 2, 3, 4, 5. Finding 4 corrects a prior withdrawal (the first live test was under-powered; see its Verifier notes). Finding 8 was additionally observed live in passing. +- **Not run on a live cluster (capacity):** confirmed findings 6, 7, 9, 10, 11, 12 and plausible 13, 14, 15 were verified statically only; their citations were re-checked against master line-by-line for this report. +- **Additional checks in this pass:** the `compute_query_id=auto` "silent no-op" hypothesis was tested and **refuted** (`EnableQueryId()` at pg_stash_advice.c:94 makes `auto` yield query IDs; advice applies under the default). See "Additional checks in this pass" above. +- **Unverified (below the 20-finding verifier cap):** findings 19 (CustomScan `custom_relids`) and 20 (single-user mode lockout). +- **Triage-dropped (6):** six lower-impact items were cut at the 22-finding cap — five documentation/config caveats (the `JOIN_ORDER`-to-prevent-foreign-pushdown recommendation in the sgml; the JOIN_ORDER synopsis BNF admitting nested sublists the grammar rejects; `persist` "on by default" being undefined without `shared_preload_libraries`; `pg_stash_advice.tsv` not being WAL-logged / carried by pg_upgrade/basebackup/standby; re-enabling `persist` reloading a stale dump) and one durability asymmetry (removal of `pg_stash_advice.tsv` is not crash-durable — no directory fsync after unlink, stashpersist.c:543). +- All cited line numbers refer to master at HEAD c1702cb.
