On Thu, Aug 27, 2026 at 1:18 PM Noah Misch <[email protected]> wrote: > 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
That's a bug. > ### 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. This seems like complaining that when you turn the lights off the room gets really dark. You should only start the worker if you're hoping for what is currently in memory to get written out to disk. In this example, what's in memory is nothing, so the file is removed instead. > ### 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. IMHO, it's impossible to look at pg_plan_advice_get_supplied_query_advice() and think that the current behavior was anything but intentional. Of course, that doesn't mean it's optimal, but if you'd like to propose changing it, I suggest starting a new thread on that specific topic and making some kind of argument for your preferred behavior. I suspect a lot of people are going to think that this thread is just about bug-fixing, rather than proposed behavior changes. I'm fine with changing this if others agree. > 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 This is a planner control feature; it does not affect behavior other than at plan time. I don't see that as a bug. That said, I think it would be perfectly valid for someone to try to figure a way for advice stash changes to invalidate plans, but I suspect that will require significant new infrastructure. Plan invalidation is generally tied to catalog modifications, and here we would instead want to tie it to a plan ID. We could do that by adding a custom invalidation type to sinval.h just for the use of pg_stash_advice, but would be a pretty serious piece of core infrastructure for an as-yet-unproven contrib module to use to solve a problem which (for all we know now) may have little practical impact. > ### 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 I'm not saying we couldn't change this, but I think changing it right before release would be a questionable decision. The revised behavior would probably be nicer for users, but I think it would only be slightly nicer. Also, we should weigh any potential benefit against the risk of messing something up under time pressure. Overall, I feel like this is a feature request that is arriving about six months too late. If someone feels motivated to work on it, they can do it for v20. It would be a backward-incompatibility, but I think the number of people intentionally storing invalid strings in their advice stashes will be very small, so I don't think it would be enough of a backward incompatibility to worry about. Looking through the other findings in this report: #2 suggests that we should add a CHECK_FOR_INTERRUPTS() call in some code path. That seems like a bug fix to me, so I'll work on that. #4 says GEQO doesn't know about plan advice. That's true. The observed behavior is implied by this statement in the documentation: "... the use of plan advice can only affect which plan the planner chooses from among those it believes to be viable. It can never force the choice of a plan which the planner refused to consider in the first place." Maybe we should add a specific mention of GEQO. I see that an XXX about GEQO has crept into the committed version of contrib/pg_plan_advice/README, and that should be replaced with something more appropriate. #6 purports to be a bug, but seems like pedantry. #7 looks like it might be a real bug. I will investigate. #9 is wrong about what the behavior should be. There could be an argument for tidying something up here, but I don't think it really matters. #10 is similar to #7 and might also be a real bug. I will investigate. #11 doesn't seem valid. I had no intent of making that case work as described, and I don't think it would be useful. #12 is already fixed. #13 seems like there might be some behavior in there that somebody might like to change for some reason at some point, but there's no clear defect. #15 and #16 are design behavior. #17 seems like another place where maybe there's some room for improvement and maybe there isn't, but I don't think we need to worry about it right now. #18 is bogus. #19 might be a real bug, but needs validation, and can't be easily validated with in-core code. #20 seems like it might be suboptimal behavior, but it's not immediately clear to me what would be better, and it seems unlikely to matter very much in practice. I'm open to proposed changes if someone wants to spend time thinking about this. Overall, I think this report does more to show that this feature is good shape than otherwise. Claude's best candidate for a major bug only affects users who stash advice strings for queries that don't need a stashed advice string. Certainly, that and a few of the other things shown here are worth correcting, but if somebody showed me the subset of this list that actually consists of valid issues and told me that was what we'd had to fix in 19.1 or 19.2 or whatever, I would be genuinely delighted that we had somehow gotten enough people using pg_plan_advice and pg_stash_advice to find this kind of thing. -- Robert Haas EDB: http://www.enterprisedb.com
