On Fri, Dec 5, 2025, at 2:57 PM, Robert Haas wrote:
> 014f9a831a320666bf2195949f41710f970c54ad removes the need for what was
> previously 0004, so here is a new patch series with that dropped, to
> avoid confusing cfbot or human reviewers.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
Hey Robert,
Thanks for working on this! I think the idea has merit, I hope it lands
sometime soon.
I've worked on extending PostgreSQL's planner for a JSONB-like document system.
The new query shapes frequently caused mysterious planner issues. Having the
ability to:
1. Dump detailed planner decisions for comparison during development
2. Record planner choices from customer databases to reproduce their issues
3. Apply fixed plans to specific queries as a quick customer workaround while
addressing root causes in later releases
...would have been invaluable.
Yes, there's danger here ("with great power comes great responsibility"), but I
see this as providing more information to make better decisions when working
with the black art of planner logic.
ORM Query Variation Challenge
Jakob's point about "crazy ORMs" is important. ORMs generate queries with minor
variations that should ideally match the same plan advice. I need to study the
plan advice matching logic more deeply to understand how it handles query
variations.
This reminded me of Erlang/Elixir's "parse transforms" - compiled code forms an
AST that registered transforms can modify via pattern matching. The concept
might be relevant here: pattern-matching portions of query ASTs to apply advice
despite syntactic variations. I'd need to think more about whether this
intersects well with the current design or if it's impractical, but it's worth
exploring.
> Attachments:
> * v5-0001-Store-information-about-range-table-flattening-in.patch
contrib/pg_overexplain/pg_overexplain.c
+ /* Advance to next SubRTInfo, if it's time. */
+ if (lc_subrtinfo != NULL)
+ {
+ next_rtinfo = lfirst(lc_subrtinfo);
+ if (rti > next_rtinfo->rtoffset)
Should the test be >= not >? Unless I am I reading this wrong, when rti ==
rtoffset, that's the first entry of the new subplan's range table. That would
mean that the current logic skips displaying the subplan name for the first RTE
of each subplan.
in src/include/nodes/plannodes.h there is:
+typedef struct SubPlanRTInfo
+{
+ NodeTag type;
+ const char *plan_name;
+ Index rtoffset;
+ bool dummy;
+} SubPlanRTInfo;
This is where I get confused, if rtoffset is an Index, then the comparison
(above) in pg_overexplain uses rti > next_rtinfo->rtoffset where rti starts at
1. If rtoffset is 0 for the first subplan, the logic might be off-by-one, no?
> This commit teaches pg_overexplain'e RANGE_TABLE option to make use
Minor nit in the commit message, "pg_overexplain'e" should be "pg_overexplain's"
> * v5-0002-Store-information-about-elided-nodes-in-the-final.patch
+/*
+ * Record some details about a node removed from the plan during setrefs
+ * procesing, for the benefit of code trying to reconstruct planner decisions
+ * from examination of the final plan tree.
+ */
Nit, "procesing" should be "processing"
> * v5-0003-Store-information-about-Append-node-consolidation.patch
src/backend/optimizer/path/allpaths.c
/* Now consider each interesting sort ordering */
foreach(lcp, all_child_pathkeys)
{
List *subpaths = NIL;
bool subpaths_valid = true;
+ List *subpath_cars = NIL;
List *startup_subpaths = NIL;
bool startup_subpaths_valid = true;
+ List *startup_subpath_cars = NIL;
List *partial_subpaths = NIL;
+ List *partial_subpath_cars = NIL;
List *pa_partial_subpaths = NIL;
List *pa_nonpartial_subpaths = NIL;
+ List *pa_subpath_cars = NIL;
I find "cars" a bit cryptic (albeit clever), I think I've decoded it properly
and it stands for "child_append_relid_sets", correct? Could you add a comment
or use a clearer name like subpath_child_relids or consolidated_relid_sets?
+accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths,
+ List
**child_append_relid_sets)
{
if (IsA(path, AppendPath))
{
@@ -2219,6 +2256,8 @@ accumulate_append_subpath(Path *path, List **subpaths,
List **special_subpaths)
if (!apath->path.parallel_aware || apath->first_partial_path ==
0)
{
*subpaths = list_concat(*subpaths, apath->subpaths);
+ *child_append_relid_sets =
+ lappend(*child_append_relid_sets,
path->parent->relids);
Is it possible that when pulling up multiple subpaths from an AppendPath, only
ONE relid set is added to child_append_relid_sets, but MULTIPLE paths are added
to subpaths? If so, that would break the correspondence between the lists
which would be bad, right?
src/include/nodes/pathnodes.h
+ * Whenever accumulate_append_subpath() allows us to consolidate multiple
+ * levels of Append paths are consolidated down to one, we store the RTI
+ * sets for the omitted paths in child_append_relid_sets. This is not necessary
+ * for planning or execution; we do it for the benefit of code that wants
+ * to inspect the final plan and understand how it came to be.
Minor: "paths are consolidated" is redundant, should be "paths consolidated" or
"allows us to consolidate".
> * v5-0004-Allow-for-plugin-control-over-path-generation-str.patch
src/backend/optimizer/path/costsize.c
+ else
+ enable_mask |= PGS_CONSIDER_NONPARTIAL;
- path->disabled_nodes = enable_seqscan ? 0 : 1;
+ path->disabled_nodes =
+ (baserel->pgs_mask & enable_mask) == enable_mask ? 0 : 1;
When parallel_workers > 0 the path is partial and doesn't need
PGS_CONSIDER_NONPARTIAL. But if parallel_workers == 0, it's non-partial and
DOES need it, right? Would this mean that non-partial paths can be disabled
even when the scan type itself (e.g., PGS_SEQSCAN) is enabled? Intentional?
> * v5-0005-WIP-Add-pg_plan_advice-contrib-module.patch
It seems this is still WIP with a solid start, I'm not going to dig too much
into it. :)
Keep it up, best.
-greg
