Horton
Menu

Re: pg_plan_advice

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: "David G. Johnston" <david.g.johnston@gmail.com>
Cc: Alexandra Wang <alexandra.wang.oss@gmail.com>, Richard Guo <guofenglinux@gmail.com>, Lukas Fittl <lukas@fittl.com>, Tom Lane <tgl@sss.pgh.pa.us>, Jacob Champion <jacob.champion@enterprisedb.com>, Dian Fay <di@nmfay.com>, Matheus Alcantara <matheusssilv97@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-04T16:20:45Z
Lists: pgsql-hackers 
On Wed, Mar 4, 2026 at 10:45 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> I do need to work in a way to better annotate/comment on the why of these.  Any suggestions for a better flow or feedback format?  Inline comments wrapped in sgml comments?  Or just copy the diff into the email body and inline comment there - leaving the original diff attachment as-is?

My suggestion is to break these fixes up into three categories: clear
errors, stylistic suggestions, substantive concerns.

Clear errors can be handled by just sending a patch that fixes all the
clear errors without changing anything else. If I intended to write
"applied" and I actually typed "aplied," it's fine to bundle that with
10 other mistakes and submit them without commentary.

For substantive concerns, I think it's most helpful to just quote the
patch hunk in the body of your email and say what your concern is. If
you have suggested wording feel free to suggest that as well, but I'd
focus more on saying what the problem is rather than jumping to the
solution. At least some of these are cases where what I wrote wasn't
sufficient for you to understand the patch, which a very fair issue to
raise, but if you try to write your own wording, the fact that you
don't understand the patch makes it hard for you to write quality
documentation for it. If you say "I read where you said X and I tried
Y and it seemed like the wrong thing happened, so either the
documentation sucks or the code is buggy," now we're having a
worthwhile conversation. If you just change the documentation based on
your understanding of the results of an undisclosed experiment, I feel
like the chances of the result being an improvement are not great.

Stylistic concerns are the most complicated. If your concern is
something like "this sentence is hard to understand," I'd class that
as a substantive concern and treat it the same way. Beyond that, I'm
not really sure. Honestly, I think we may just have different
stylistic preferences, because my experience so far reading your
proposed documentation patches is that I tend to agree with relatively
little of what you want to do. I believe, though, that other
committers feel differently about it and find your proposed changes
quite helpful. So I'm not sure exactly what to recommend here, but
perhaps take a lighter touch when it's my patch? I'm OK with some
friendly suggestions that I can accept or reject, but going through a
huge list of minor wording tweaks is as much work as going through a
substantial review of the code itself, but for a lot less benefit,
especially if they all look like random changes that I don't
understand why you're proposing.

Hope that makes sense and isn't too harsh.

Thanks,

-- 
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.