Horton
Menu

Re: pg_plan_advice

Lukas Fittl <lukas@fittl.com>

View thread
From: Lukas Fittl <lukas@fittl.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-13T08:38:25Z
Lists: pgsql-hackers

Attachments

Hi Robert,

On Thu, Mar 12, 2026 at 10:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I've now committed the main patch. I think that I'm not likely to get
> too much more feedback on that in this release cycle, and I judge that
> more people will be sad if it doesn't get shipped this cycle than if
> it does. That might turn out to be catastrophically wrong, but if many
> problem reports quickly begin to emerge, it can always be reverted.

I think its good to get this in, and not do it in the last minute, but
just to be clear on my part, I've reviewed earlier versions, but I
have not reviewed the latest code to the extent I would have liked
before it was committed, due to competing priorities on my end. That
said, its a good forcing function, so let me do my part to help shake
out any bugs that remain.

From a code review perspective, I found some minor issues:

- Identifiers that are named like advice types cause an error (so I
can't name my table "hash_join")
- pgpa_destroy_join_unroller does not free the inner_unrollers values
(which I think goes against the intent of cleaning up the allocations
right away)
- pgpa_parser uses pgpa_yyerror, but that function doesn't use
elog(ERROR) like the main parser does - I think it'd be more
consistent to use YYABORT explicitly, e.g. how we do it in cubeparse
- pgpa_scanner accepts integers with underscores, but incorrectly uses
a simple strtoint on them (which would fail), instead of pg_strtoint32
/ pg_strtoint32_safe
- pgpa_walk_recursively uses "0" for the boolean value being passed to
a recursive call in one case (should be "false")

Attached nocfbot-0001 that addresses these.

From a usability perspective, I do wonder about two things when it
comes to users specifying advice directly (which will happen in
practice, e.g. when debugging plan problems):

1) The lack of being able to target scan advice (e.g. SEQ_SCAN) to a
partition parent is frustrating - I believe we discussed partitioning
earlier in the thread in terms of gathering+applying it, but I do
wonder if we shouldn't at least allow users to specify a partitioned
table/partitioned index, instead of only the children. See attached
nocfbot-0002 for an idea what would be enough, I think.

2) I find the join strategy advice hard to use - pg_hint_plan has
hints (e.g. HashJoin) that allow saying "use a hash join when joining
these two rels in any order", vs pg_plan_advice requires setting a
specific outer rel, which only makes sense when you want to fully
specify every aspect of the plan. I suspect users who directly write
advice will struggle with specifying join strategy advice as it is
right now. We could consider having a different syntax for saying "I
want a hash join when these two rels are joined, but I don't care
about the order", e.g. "USE_HASH_JOIN(a b)". If you think that's
worthwhile I'd be happy to take a stab at a patch.

> I'm still hoping to get some more feedback on the remaining patches,
> which are much smaller and conceptually simpler. While there is no
> time to redesign them at this point in the release cycle, there is
> still the opportunity to fix bugs, or decide that they're too
> half-baked to ship. So here is v20 with just those patches. Of course,
> post-commit review of the main patch is also very welcome.

For v20-0001, from a quick conceptual review:

I find the two separate GUC mechanisms for local backend vs shared
memory a bit confusing as a user (which one should I be using?).
Enabling the shared memory mechanism on a system-wide basis seems like
it'd likely have too high overhead anyway for production systems?
(specifically worried about the advice capturing for each plan, though
I haven't benchmarked it)

I wonder if we shouldn't keep this simpler for now, and e.g. only do
the backend local version to start - we could iterate a bit on
system-wide collection out-of-core, e.g. I'm considering teaching
pg_stat_plans to optionally collect plan advice the first time it sees
a plan ID (plan advice is roughly a superset of what we consider a
plan ID there), and then we could come back to this for PG20.

For v20-0002:

I think we need a form of this in tree, because otherwise
pg_plan_advice breaks as planner logic changes. This of course puts a
burden on other hackers making changes, but I think that's what we're
signing up for with having pg_plan_advice in contrib - and maybe we're
overestimating how often that'll actually be a problem.

To help assess impact, I did a quick test run and looked at three
not-yet-committed patches in the commitfest that affect planner logic
([0], [1] and [2]), to see if they'd require pg_plan_advice changes
(master with v20-0002 applied). Maybe I picked the wrong patches, but
at least with those no pg_plan_advice changes were needed with the
test_plan_advice test enabled.

On the code itself:
- Is there a reason you're setting
"pg_plan_advice.always_store_advice_details" to true, instead of using
pg_plan_advice_request_advice_generation?
- I wonder if we could somehow detect advice not matching? Right now
that'd be silently ignored, i.e. you'd only get a test failure when we
generate the wrong advice that causes a plan change in the regression
tests.

For v20-0003, initial thoughts:

I think getting at least a basic version of this in would be good, as
a server-wide way to set advice for queries can help people get out of
a problem when Postgres behaves badly - and we know from pg_hint_plan
(which has a hint table) that this can be useful even without doing
any kind of parameter sniffing/etc to be smart about different
parameters for the same query.

The name "stash" feels a bit confusing as an end-user facing term.
Maybe something like "pg_apply_advice", or "pg_query_advice" would be
better? (though I kind of wish we could tie it more closely to "plan
advice", but e.g. "pg_plan_advice_apply" feels too lengthy)

---

Thanks,
Lukas

[0]: https://commitfest.postgresql.org/patch/5487/ ("Pull-up subquery
if INNER JOIN-ON contains refs to upper-query")
[1]: https://commitfest.postgresql.org/patch/5738/ ("Improve hash
join's handling of tuples with null join keys")
[2]: https://commitfest.postgresql.org/patch/6315/ ("Enable
partitionwise join for partition keys wrapped by RelabelType")


--
Lukas Fittl

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.