Horton
Menu

Re: pg_plan_advice

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2026-03-18T17:07:54Z
Lists: pgsql-hackers 
Hi,

Coverity complained about the fact that pgpa_planner_feedback_warning
does not do anything to free the memory used by flagbuf or detailbuf,
and Tom asked whether that failure could amount to a significant leak.
The best way to use a lot of memory here is to have a very long advice
string that doesn't do anything useful, so I created this test case:

load 'pg_plan_advice';
select set_config('pg_plan_advice.advice', v.v, false) from (select
string_agg('SEQ_SCAN(hogehogehoge' || g || ')', ' ') v from
generate_series(1,10000) g) v;
set pg_plan_advice.feedback_warnings = true;
select 1;

While this is not the worst case imaginable, it's pretty bad. There's
probably no use case for having 10000 advice items in your advice
string even if they were all valid, and here none of them are valid
for the final query (select 1). So this produces "WARNING:  supplied
plan advice was not enforced" with a 10000 line detail message where
all the lines look like this, but with different numbers:

advice SEQ_SCAN(hogehogehoge7348) feedback is "not matched"

Failure to free the buffers costs us just over 1MB of memory, which is
not wildly disproportionate given that the message itself is over half
a MB long, so I'm not sure that freeing it is all that useful here. I
don't think the context we use for planning normally sticks around for
all that long. If I'm wrong about that and it does, then we should
probably wrap our own shorter-lived context around the stuff this
module is doing rather than trying to free allocations individually.

But this does raise two points which are perhaps worthy of some
further consideration:

1. Maybe we should limit the length of the detail message. In some
other cases, like reportDependentObjects, we limit the detail message
to the first 100 problems found, and then say at the end of that
detail message how many more problems there were afterwards. We could
do that here, too. I'm not 100% sure it's worth the code, but then
again it's not much code.

2. The way the current code works is to transform the advice feedback
into a Node-tree representation which is stashed in the PlannedStmt's
extension_state, and then also passes that Node-tree representation to
pgpa_planner_feedback_warning, which generates the actual warning.
That Node-tree representation is currently used by EXPLAIN to display
the advice feedback, which I think for most people will be a nicer
interface than enabling pg_plan_advice.feedback_warnings, and it can
also be used by other extensions. For instance, we could extend
pg_stash_advice so that it looks at the advice feedback and updates
the shared store, so that users can monitor whether their stashed
advice is doing what they hoped it would.

However, in a case like this, that Node tree is actually quite large:
about 16MB. I guess that's because pgpa_planner_append_feedback() has
to do multiple allocations for every item of feedback: a C string,
DefElem, an Integer, plus whatever lappend() charges us to add to a
List. We could save that memory by adding code here to optimize for
the case where we need to generate warnings but we don't need the Node
tree for anything else. I'm inclined not to do that, because (A) I
don't think temporarily using 16MB when the user specifies 10,000
items of bogus advice is really that bad and (B) complicating the code
has its own costs, such as maybe introducing more bugs. However, maybe
someone else sees it differently.

Another idea is to try to find a more economic Node representation.
For instance, we could jam the flags into the DefElem's location
field, instead of building a separate Integer node, or we could build
the advice feedback as a giant binary blob and wrap it in a varlena
and a Const node and leave consumers to make sense of that as best
they're able, or we could invent a new Node type that's just to the
perfect thing to hold a C string and an integer. I'm inclined to think
that the first two are too hacky and the third is too special-purpose,
but again someone else might see it otherwise.

Thoughts?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_plan_advice: Fix another unique-semijoin bug.

  2. pg_plan_advice: Export feedback-related definitions.

  3. pg_plan_advice: Fix a bug when a subquery is pruned away entirely.

  4. pg_plan_advice: Add alternatives test to Makefile.

  5. pg_plan_advice: Handle non-repeatable TABLESAMPLE scans.

  6. pg_stash_advice: Allow stashed advice to be persisted to disk.

  7. Add pg_stash_advice contrib module.

  8. pg_plan_advice: Avoid assertion failure with partitionwise aggregate.

  9. pg_plan_advice: Invent DO_NOT_SCAN(relation_identifier).

  10. Add an alternative_plan_name field to PlannerInfo.

  11. pg_plan_advice: Refactor to invent pgpa_planner_info

  12. Respect disabled_nodes in fix_alternative_subplan.

  13. get_memoize_path: Don't exit quickly when PGS_NESTLOOP_PLAIN is unset.

  14. test_plan_advice: Set TAP test priority 50 in meson.build.

  15. pg_plan_advice: Avoid a crash under GEQO.

  16. Test pg_plan_advice using a new test_plan_advice module.

  17. pg_plan_advice: Always install pg_plan_advice.h, and in the right place

  18. pg_plan_advice: Fix failures to accept identifier keywords.

  19. Add pg_plan_advice contrib module.

  20. Allow extensions to mark an individual index as disabled.

  21. Replace get_relation_info_hook with build_simple_rel_hook.

  22. Store information about Append node consolidation in the final plan.

  23. Store information about elided nodes in the final plan.

  24. Store information about range-table flattening in the final plan.

  25. Pass cursorOptions to planner_setup_hook.

  26. Fix PGS_CONSIDER_NONPARTIAL interaction with Materialize nodes.

  27. Fix mistakes in commit 4020b370f214315b8c10430301898ac21658143f

  28. Allow for plugin control over path generation strategies.

  29. Update some comments for fasthash

  30. Allow passing a pointer to GetNamedDSMSegment()'s init callback.

  31. Don't reset the pathlist of partitioned joinrels.

  32. Treat number of disabled nodes in a path as a separate cost metric.