Repository navigation
Conversation
This isn't a commit that will be submitting for review, it is purely for local developer tooling while developing this patch set. Ignore it.
gcc16 (nixpkgs-unstable) links glibc 2.42 but the stable patchedGlibc in buildInputs put a 2.40 libc.so.6 first in RUNPATH, so every meson sanity-check binary failed with a GLIBC_PRIVATE symbol lookup error. The patched glibc only existed to silence the fortify-without-O warning; hardeningDisable does that without a second glibc. pg-test-one: set LD_LIBRARY_PATH to the tmp_install lib dir, as the meson test runner does, so initdb finds libpq. pg-format: put the in-tree pg_bsd_indent build on PATH.
An Index Scan that orders by an operator (ORDER BY col <-> const) computes each returned row's ORDER BY value inside the index AM and reports it in xs_orderbyvals. If the same expression also appears in the target list, the executor then evaluates it a second time in the scan's projection (a third time if the AM asked for a recheck). For an expensive distance operator that is wasted work. The AM contract only requires a non-rechecked distance to sort the same way as the operator's result, not to equal it, so the executor cannot substitute it blindly. Add an optional IndexAmRoutine callback, amcanreturnorderby(opfamily, opcintype, orderbyop): does the opclass report, for a non-rechecked leaf entry and that ordering operator, exactly the value the operator would return? It takes the operator because one opfamily can hold several ordering operators. The planner keeps the callback in IndexOptInfo, like amcostestimate, and create_indexscan_plan() asks it for each ORDER BY key, carrying one bool per key into the IndexScan. For each qualifying key, setrefs.c replaces a top-level target list expression that is equal() to that indexorderbyorig expression with Var(INNER_VAR, i), and nodeIndexscan.c stores the returned row's ORDER BY values in a virtual slot it installs as the projection's inner tuple (a scan node has no inner plan, so INNER_VAR is otherwise unused there). The values stored are the AM's for a row it reported exact, the recomputed ones for a row it asked to recheck, a reorder queue entry's copy for a queued row, and freshly recomputed ones for an EvalPlanQual test tuple. Only float4/float8 ORDER BY expressions qualify, as those are the only types index_store_float8_orderby_distances() can deliver. GiST answers yes for point_ops and SP-GiST for quad_point_ops and kd_point_ops, whose leaf distances call point_distance(), the function behind <->(point, point). box_ops doesn't qualify: its leaf distance is exact arithmetic on the box, while <->(box, point) treats a point within EPSILON of an edge's line as on it, so the two can differ in the last digits. The polygon and circle opclasses report lower bounds and are not affected, nor is any other AM or any out-of-core opclass, which all default to no. EXPLAIN VERBOSE deparses the INNER_VAR back to the ORDER BY expression and reports "Order By Values Used: N". Above such a scan, an upper node's reference to the value now prints with one more pair of parentheses, the same way any reference to a computed scan column does. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
The previous commit lets a plain Index Scan return its ORDER BY values to a target list entry that matches the ORDER BY expression, instead of evaluating that expression again. The planner didn't know that. An ORDER BY expression is always part of the scan/join target (as a sort column, if nothing else), and apply_scanjoin_target_to_paths() charges every path rows * cost(expression) when it applies that target. For the Index Scan that cost is now never paid, while Seq Scan + Sort and Bitmap Heap Scan + Sort really do pay it, once per row. With a costly ORDER BY expression the planner therefore overestimated the kNN scan by up to ~32x and picked a Sort plan that was up to 13x slower. Teach create_projection_path() and apply_projection_to_path() to cost the target a path pays for with a new helper, path_target_cost(), which for a plain IndexScan leaves out top-level target expressions the scan will take from its ORDER BY values. The test for "will take" is index_orderby_returnable() plus orderby_tlist_match(); setrefs.c's rewrite and create_indexscan_plan() use the same two, so the cost model and the plan cannot drift apart. orderby_tlist_match() accepts the expression equal() to the ORDER BY key, and also its commuted form: match_clause_to_ordering_op() turns "const <-> col" into "col <-> const" for the index, so a target entry written the original way is the same value but not equal() to the key. An expression that only contains the ORDER BY expression, e.g. (expr) * 2, isn't rewritten and is still charged. Because this makes an IndexPath cheaper relative to its siblings, apply_scanjoin_target_to_paths() can no longer assume that applying the target preserves the cost ordering of rel->pathlist and rel->partial_pathlist, which add_path_precheck() and the linitial(partial_pathlist) callers rely on. Re-sort them with a new sort_pathlist_by_cost(), a stable insertion sort that is a single pass in the usual already-sorted case. Nothing changes for an index whose opclass does not pass amcanreturnorderby, for IndexOnlyScan, for bitmap scans, or for any query without a returnable ORDER BY expression in the target. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
📜 Change history & discussion (Agora / pg.ddx.io)All prior-art links are verified. I have what I need. 🧵 Related discussion
🔗 Related commits / prior artThe patch frames itself explicitly against earlier threads (all verified):
📋 Commitfest
🧭 Context for reviewers
Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Breakpoints pinned to absolute source line numbers (heapam.c:3210, 4019, 4024, 4033, 4101, 4147; heapam_indexscan.c:182, 250, 297; indexam.c:299; execIndexing.c:370; pruneheap.c:1287, 1802, 1836, 1863, 2936) silently drift the moment any of those files change. I verified heapam_indexscan.c:182 in this tree is lp = PageGetItemId(page, offnum);, which does not match the comment's claim of "initialize bitmap accumulator from collapsed redirect" -- the line-number breakpoints are already stale. More fundamentally, this entire file is personal developer tooling that should not be committed to a PostgreSQL patch destined for pgsql-hackers/commitfest; it violates the minimal-diff/one-logical-change discipline. Move it to a local (git-ignored) location. If kept, prefer symbol-based breakpoints (e.g. break heap_prune_chain) which survive edits.
| define print_relcache | ||
| set $rel = (Relation)$arg0 | ||
| if $rel | ||
| printf "Relation: %s.%s (OID: %u)\n", $rel->rd_rel->relnamespace, $rel->rd_rel->relname.data, $rel->rd_id |
There was a problem hiding this comment.
This GDB pretty-printer is stale against the current backend. relnamespace is an Oid (confirmed in src/include/catalog/pg_class.h), so the %s format with $rel->rd_rel->relnamespace is wrong and will print garbage / error at runtime -- it should be %u. Also note rd_rel->relname.data is a NameData; the surrounding usage should be checked too. Committing a misleading debugging helper is worse than omitting it.
| set $cell = $list->head | ||
| set $i = 0 | ||
| while $cell && $i < $list->length |
There was a problem hiding this comment.
print_list walks $list->head and $cell->next, which is the pre-PG13 linked-list List layout. The modern PostgreSQL List (src/include/nodes/pg_list.h) is array-backed: it has length, max_length, and ListCell *elements with no head/next fields. This helper will fail at runtime against current backend structs. Traverse via $list->elements[$i] instead, or drop the helper.
| --dlpath="$bdir/src/test/regress" \ | ||
| --outputdir="$outdir" \ | ||
| --temp-instance="$outdir/tmp" \ | ||
| --port=40099 \ |
There was a problem hiding this comment.
pg-test-one hardcodes --port=40099. A fixed port collides with any concurrently running regress instance or another dev using the same port, causing intermittent test-setup failures. pg_regress normally picks a free port; consider omitting --port or deriving it from the worktree so parallel runs don't clash.
| bool | ||
| spgcanreturnorderby(Oid opfamily, Oid opcintype, Oid orderbyop) | ||
| { | ||
| return (get_opfamily_proc(opfamily, opcintype, opcintype, | ||
| SPGIST_LEAF_CONSISTENT_PROC) == | ||
| F_SPG_QUAD_LEAF_CONSISTENT && | ||
| get_opcode(orderbyop) == F_POINT_DISTANCE); |
There was a problem hiding this comment.
The new spgcanreturnorderby path (ordered index-only scan returning exact <-> distances from quad_point_ops/kd_point_ops leaves) ships with no regression test. The parallel GiST change added extensive coverage in src/test/regress/sql/gist.sql (notably the counting-function test that proves the ORDER BY value is served from the index rather than re-evaluated), but no SP-GiST equivalent was added in this change. Per project policy a user-visible behavior change requires tests; please add SP-GiST coverage exercising this path. Confidence: high.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
Unrelated, non-minimal change that also breaks a tree-wide convention. Every other Perl script in the PostgreSQL source uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.); switching only pgindent to #!/usr/bin/env perl introduces an inconsistency and is unrelated to the KNN/EvalPlanQual feature this patch implements. This looks like a local (Nix) environment accommodation and should be dropped to keep the diff minimal. Revert to #!/usr/bin/perl. (high confidence)
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| if (best == NULL) | ||
| return; | ||
| accumulate_append_subpath(best, &input.subpaths, NULL, | ||
| &input.child_append_relid_sets); | ||
| foreach(lp, childrel->pathlist) |
There was a problem hiding this comment.
The early return here (when a child lacks a qualifying non-partial path) bails out of the entire function, which also skips the parallel Append construction below. The parallel block searches childrel->partial_pathlist -- a different list -- so a child that can emit the extras via a partial path but not a non-partial one will silently lose a valid parallel Append. If the unordered Append can't be built, consider only skipping the unordered/ordered variants (e.g. break out of this block) rather than returning, so the parallel Append is still attempted. (Missed optimization, not a correctness bug; confidence: moderate.)
| if (target == NULL) | ||
| target = copy_pathtarget(rel->reltarget); | ||
| add_column_to_pathtarget(target, orderby, 0); |
There was a problem hiding this comment.
add_column_to_pathtarget does not deduplicate, unlike add_new_column_to_pathtarget used in indexonly_path_target(). If two ORDER BY keys yield the same returnable expression value (distinct OpExpr nodes that are equal()), the expression is appended twice, widening the tuple and causing path_target_cost() to credit it twice. Using add_new_column_to_pathtarget() here would keep the two returnable-value paths consistent and avoid the duplicate. (Edge case; confidence: low.)
| if ((extra->pgs_mask & | ||
| (PGS_NESTLOOP_MATERIALIZE | PGS_CONSIDER_NONPARTIAL)) == | ||
| (PGS_NESTLOOP_MATERIALIZE | PGS_CONSIDER_NONPARTIAL) && | ||
| inner_cheapest_total != NULL) | ||
| { | ||
| Path *extras_inner = extra->extras_inner; |
There was a problem hiding this comment.
These two guard conditions are identical except for the !ExecMaterializesOutput(inner_cheapest_total->pathtype) sub-check, which is just the existing matpath guard above. The whole block can be folded into the preceding if that computes matpath, reusing the single condition evaluation and avoiding the copy-paste of the (extra->pgs_mask & (...)) == (...) && inner_cheapest_total != NULL test. Keeping two near-identical blocks invites them to drift out of sync.
| ojrelids = bms_difference(path->parent->relids, | ||
| bms_union(jpath->outerjoinpath->parent->relids, | ||
| jpath->innerjoinpath->parent->relids)); | ||
| return free_exprs_cost(root, target, join_free_exprs(jpath), | ||
| !bms_is_empty(ojrelids)); |
There was a problem hiding this comment.
The join branch allocates two throwaway Bitmapsets (bms_union then bms_difference) on every call solely to test whether any outer-join relids are formed here. path_target_cost() is invoked from cost_index(), create_projection_path() and apply_projection_to_path(), i.e. once per candidate path during projection costing, so this is needless palloc churn on a planner hot path. You can avoid both allocations: the join forms outer joins iff path->parent->relids is a strict superset of the union of the two inputs' relids, which bms_is_subset/membership checks can answer without materializing the difference.
With the previous two commits, an ordering Index Scan returns its ORDER BY values to a target list entry that matches the ORDER BY expression, and isn't charged for it. But that only happens when the scan is the top of the scan/join tree. When the scan is below a join, its target is rel->reltarget, which holds only the Vars the rest of the query needs; the join computes the ORDER BY expression from those Vars, once per joined row, though the scan already had the value. Tom Lane pointed at this in the 2023 junk-column thread: a fix that only munges the final targetlist "leaves everything on the table as soon as there's more than one level of plan involved". Give such an IndexPath its own PathTarget: a copy of rel->reltarget with each returnable ORDER BY expression appended, but only if the query needs that expression above the scan (a top-level entry of the final targetlist, which includes sort columns). Most index paths keep sharing rel->reltarget as before. The appended expressions cost the scan nothing (cost_index() now uses path_target_cost()). Above the scan, setrefs.c's fix_join_expr() already matches whole non-Var expressions against the input targetlists before descending into them, so the join's copy becomes a reference to the scan's output with no setrefs change; and the join isn't charged for it either, since path_target_cost() leaves out of a join path's target the entries an input already emits (looking through Material and Memoize). use_physical_tlist() must not replace such a scan's targetlist with a physical one, which has only Vars; index_path_emits_orderby() tells it. Scope: plain IndexScan paths on plain base relations only. Not IndexOnlyScan (no ORDER BY value plumbing), not appendrel children (Append/MergeAppend build their own child targetlists, and the MergeAppend-over-kNN case already works through the scan/join target), and only ORDER BY expressions that pass index_orderby_returnable(). Because the join reads the value from the scan's output, an outer join nulls it correctly with the rest of that side's columns. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me Discussion: https://postgr.es/m/2935085.1703806977@sss.pgh.pa.us
An index-only scan on an index with an expression column reads that expression's value from the index: set_indexonlyscan_references() replaces a target list entry equal() to a returnable index expression with a reference to that column. But the planner still charges the scan the full cost of evaluating the expression for every row, as if it computed it. Heikki Linnakangas showed this in the 2023 junk-column thread with an index on (i, expensive(i)) and a function of COST 1000000: the index-only scan was estimated at 25 million, like the Seq Scan that really does evaluate expensive(i). path_target_cost() now also handles index-only scan paths: a top-level target expression equal() to a returnable index expression costs nothing. cost_index() already goes through path_target_cost(), so the index-only scan's estimate drops to what it actually does. This corrects the estimate of an index-only scan path that is already in the running, e.g. one kept for its pathkeys. It does not make such a path win the base relation's add_path() comparison when its only merit is the cheaper expression: at that point every path of the rel carries rel->reltarget, which holds only Vars, and the expression is charged to all of them alike afterwards. Doing that would mean giving index-only paths their own targets, as the previous commit does for ordering Index Scans; that's left for later. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me Discussion: https://postgr.es/m/356d5811-d8f0-4c29-a601-b61334337b11@iki.fi
The previous commit stopped charging an index-only scan for target
expressions it reads from the index, but that only changes the plan
when the index-only scan path is already in the running for some other
reason, such as useful pathkeys. Heikki Linnakangas's example, an index
on (i, expensive(i)) and
SELECT expensive(i) FROM atab ORDER BY expensive(i);
still got a Seq Scan and evaluated expensive() for every row. When the
rel's paths are compared, they all carry rel->reltarget, which holds
only Vars. The index-only scan costs more than the seq scan, and the
expression it would have read for free is charged to the winner later.
Give an index-only scan path its own PathTarget: rel->reltarget plus
each returnable index expression that is a top-level entry of the
query's targetlist. This is the same thing the earlier commit does for
an ordering Index Scan's ORDER BY values. Teach add_path() and
add_partial_path() to treat base-rel index paths with different targets
as incomparable, the way they treat different pathkeys, so neither
dominates the other on cost alone. When the scan/join target is
applied, the seq scan pays for evaluating the expression and the
index-only scan doesn't, so the index-only scan wins on cost when it
should. A path with its own target also needs the projection even
when the scan/join target's exprs equal the reltarget's, since its
target has extra columns.
Heikki's query now plans as Sort over an Index Only Scan: estimated
930 instead of 25000809. With a function that really costs something,
it ran about 20 times faster.
This is limited to the query's sole base relation, where the value goes
straight to the scan/join target. Below a join, the join would have to
credit the input for the value and the joinrel's add_path() would have
to keep it; that isn't attempted here.
Author: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
Discussion: https://postgr.es/m/356d5811-d8f0-4c29-a601-b61334337b11@iki.fi
The previous commit lets an index-only scan beat a seq scan when it reads an expression the query's targetlist needs, but only when the scan's rel is the only one in the query and the targetlist entry is that exact expression. Below a join the scan isn't chosen, and the expression is computed above it as before. Nor is it chosen when the expression is part of a larger one, such as expensive(i) + 1, or when the table is partitioned. Generalize the extra values an index path may emit, which 0003 added for an ordering IndexScan's ORDER BY values and the previous commit for an index-only scan's expression columns: * A join emits what its inputs emit, so the value reaches the scan/join target from any depth. fix_join_expr() already matches whole expressions in an input's targetlist, and path_target_cost() now credits them wherever they appear in an expression, as setrefs.c replaces them. Material and Memoize share their input's target. * An index-only scan emits a returnable index expression wherever it appears in the targetlist, including ORDER BY columns. Its qpquals aren't charged for one either, since set_indexonlyscan_references() rewrites them too. * For a partitioned table whose partitions all have the expression index, an unordered Append over child paths that all emit the value emits it too. create_append_child_plan() puts each child's values in the Append's order, translating them to the child, and drops any extra values a child emits for an Append that doesn't. * Hash and merge joins look only at each input's cheapest path, which rarely emits the value. Also try each input's cheapest path that does, and skip add_path_precheck() for paths over such an input, as the precheck can't see the advantage. add_path() and add_partial_path() compare the extra values like pathkeys: a path that emits a superset of another's is better, so it can dominate it, but not the other way round. The previous commit's rule kept both whenever the targets differed, which kept, for example, a seq scan that a cheaper, better-ordered kNN path dominates. The cost is planning time: every join path over an input with an extra value is kept alongside the one it would otherwise have replaced. So a value is emitted only if it costs more than 10 times cpu_operator_cost per evaluation, the same test make_sort_input_target() uses to decide an expression is worth postponing past a sort. That applies to an ordering IndexScan's ORDER BY values too, so a plain point distance is computed again above a join. Queries without such an expression plan as before. A value from the nullable side of an outer join is never used above it: the query's Vars carry varnullingrels the index's don't, so they don't match. Partial paths emit only parallel-safe values. An ordered or parallel Append still computes the expression above it. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me Discussion: https://postgr.es/m/356d5811-d8f0-4c29-a601-b61334337b11@iki.fi
…ppends The previous commit passes an index's extra values (see path_extra_exprs()) up through inner joins and an unordered Append, but stops at three places, where the expression is then computed above them: * The nullable side of an outer join. Above the join, the query's copy of the expression has the join's bit in its Vars' nullingrels, and the scan's copy doesn't. The join null-extends the scan's value along with the Vars, so the two agree on every row exactly when the expression yields NULL for NULL inputs. So for a strict expression, join_path_target() now relabels the value with the joinrel's Vars (their nullingrels) as it passes it up, and setrefs.c's fix_join_expr() matches it against the input's copy at an outer join the way it already matches Vars there: equal but for nullingrels, with the upper copy's a superset of the input's (search_indexed_tlist_for_nulled_expr()); path_target_cost() credits it the same way. A non-strict expression is still computed above the join. An index-only scan matches the query's targetlist without nullingrels, so it emits the value in the first place; an ordering IndexScan's ORDER BY values still aren't emitted on the nullable side. * Partial paths, which emitted only parallel-safe values. A worker reads the stored value; nothing evaluates the expression, so a parallel-restricted one is fine. Gather and Gather Merge now pass a partial path's values up (gather_target()), and a Gather over the cheapest partial path that emits values is considered as well as one over the cheapest partial path. * An ordered Append or MergeAppend, and a parallel Append. add_extras_append_path() now also builds those, over child paths that emit the values and have the ordering, and over the children's partial paths, collecting the children as generate_orderedappend_paths() does. The expression also has to be compared without nullingrels when a scan decides what to emit. The new strip_nullingrels() in clauses.c does that; remove_nulling_relids() can't, as it errors out on the negative special varnos (ROWID_VAR in a MERGE). Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
|
Thanks for the review. Disposition of the 22 inline comments, as of 931496f: Fixed
Already covered
Out of scope: dev tooling, not in the patch set |
There was a problem hiding this comment.
🔍 OCR found 29 issue(s).
- 25 inline, 4 in summary (inline capped at 25)
📄 src/test/regress/pg_regress.c (L1246-L1246)
This execl -> execlp change is unrelated to the index/KNN feature this patch implements, and it changes behavior in a way that should not be included. shellprog is set from SHELLPROG, which is $(SHELL) from the Makefile (normally an absolute path such as /bin/sh) or the hardcoded /bin/sh in meson.build. Since the value already contains a slash, execlp buys nothing for the shipped build; its only effect is to make the test harness search PATH for the shell when a bare name is supplied, which is a footgun (PATH-dependent shell resolution in the test runner) and appears to be a local Nix-environment convenience (cf. the added .envrc/flake.nix/shell.nix). Revert this to execl to keep the patch minimal and avoid changing process-spawn semantics. (high confidence)
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
📄 src/tools/pgindent/pgindent (L1-L1)
This shebang change (#!/usr/bin/perl -> #!/usr/bin/env perl) is unrelated to the feature in this patch and is inconsistent with the rest of the tree: every other Perl script uses #!/usr/bin/perl (e.g. src/tools/copyright.pl, src/tools/git_changelog, src/tools/mark_pgdllimport.pl, src/test/recovery/t/*). Unrelated convenience churn like this is a common rejection reason on -hackers; revert to keep the diff minimal and consistent with project convention. (high confidence)
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
📄 src/backend/optimizer/plan/createplan.c (L1273-L1273)
Internal elog() messages in this file (and the backend generally) start with a lowercase letter; e.g. "failed to assign all NestLoopParams...", "index key does not match...". This new message starts with an uppercase "Append", which is inconsistent with the convention. Suggest lowering the first letter.
💡 Suggested change
Before:
elog(ERROR, "Append child doesn't emit an Append's extra value");
After:
elog(ERROR, "Append child doesn't emit an Append's extra value");
📄 src/backend/optimizer/plan/setrefs.c (L3260-L3262)
The intended caching here does not work when itlist->tlist contains no non-Var/non-PHV entries. In that case the build loop leaves itlist->nulled_tles == NIL, so the function returns NULL -- but every subsequent call for the same itlist re-enters the if (itlist->nulled_tles == NIL) block and rescans the entire tlist, calling strip_nullingrels()/expr_nullingrels() again, defeating the comment's promise that "later calls don't strip them again". During a single fix_join_expr() pass over a large targetlist this repeats O(tlist) work per non-Var lookup. Consider a separate "already built" flag (or an empty sentinel) so the scan runs at most once per itlist.
| ParameterNames: true | ||
| DeducedTypes: true | ||
| CompileFlags: | ||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json |
There was a problem hiding this comment.
CompilationDatabase: build/ and the trailing -I../../../../src/include hardcode one developer's directory layout (a build/ dir and a specific nesting depth). This won't match other contributors' setups and reinforces that this is personal, non-portable config that should not be committed.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These absolute source-line breakpoints (e.g. heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299, heapam_indexscan.c:182) are extremely fragile: they drift with every unrelated edit and are tied to a specific in-progress "HOT indexed updates" branch. As a committed artifact they will silently point at the wrong lines. This file should not be in the tree at all (local-only tooling), but at minimum line-number breakpoints are unmaintainable.
| # ========================================================================= | ||
|
|
||
| # Predict augmented tuple size (returns 0 if t_hoff would overflow) | ||
| break heap_hot_indexed_tuple_size |
There was a problem hiding this comment.
These function-name breakpoints reference symbols that do not exist anywhere in this tree (verified: no heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, etc. in the heap sources). They belong to an unmerged patch series not present in this change, so GDB will fail to set them. This confirms the file is aspirational scaffolding for work that has not landed.
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" | ||
| mkdir -p "$build_dir" |
There was a problem hiding this comment.
rm -rf "$build_dir" (and the trash fallback) runs with a variable that can be empty: pg_clean_for_compiler defaults build_dir to $PG_BUILD_DIR, which is unset if the shell hook never ran. With an empty build_dir this becomes rm -rf "" / mkdir -p "" and, combined with the later cd-free usage, is a data-loss footgun. Guard against an empty/unset path before destructive removal.
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" | |
| mkdir -p "$build_dir" | |
| if [ -z "$build_dir" ]; then | |
| echo "Refusing to clean: build_dir is empty" >&2 | |
| return 1 | |
| fi | |
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" | |
| mkdir -p "$build_dir" |
| alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' | ||
|
|
||
| # Database management | ||
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
There was a problem hiding this comment.
pg-init does rm -rf "$PG_DATA_DIR" with no empty-variable guard. If PG_DATA_DIR is unset (shell hook not sourced), this reduces to rm -rf "" and then initdb into an empty path. Same data-loss footgun as the build-dir cleanup; guard the variable before the destructive call.
| select f.id, gist_join_pt(f.p) <-> point(5,5) as d | ||
| from gist_join_fact f join gist_join_dim j on j.id = f.id | ||
| order by gist_join_pt(f.p) <-> point(5,5) limit 3; | ||
| select nextval('gist_join_cnt') - 1 as calls_during_join; |
There was a problem hiding this comment.
Same portability concern: this join select prints full-precision float8 distances (1.4142135623730951, 2.8284271247461903). The surrounding counting-sequence checks already prove the function isn't re-evaluated; printing the raw distances adds a non-portable row to the expected output. Wrap in set extra_float_digits = 0;/reset or drop the printed distance column.
| * indexorderbytlist counts the replacements actually made (0 if none), | ||
| * for EXPLAIN. | ||
| */ |
There was a problem hiding this comment.
The comment claims indexorderbytlist is "for EXPLAIN", but the field is actually read in the executor as a behavioral flag: nodeIndexscan.c uses if (node->indexorderbytlist > 0) in ExecInitIndexScan both to force projection setup (ExecInitResultSlot/ExecAssignProjectionInfo) and to allocate iss_OrderBySlot. EXPLAIN is only a secondary consumer. Comments must describe what the code does now; please correct the "for EXPLAIN" characterization so it reflects that this count drives executor projection/slot setup (and is also displayed by EXPLAIN).
| * indexorderbytlist counts the replacements actually made (0 if none), | |
| * for EXPLAIN. | |
| */ | |
| * indexorderbytlist counts the replacements actually made (0 if none). | |
| * The executor keys slot/projection setup off it, and EXPLAIN displays it. | |
| */ |
| for (i = 0; i < node->iss_NumOrderByKeys; i++) | ||
| { | ||
| Assert(node->iss_OrderByTypByVals[i]); | ||
| oslot->tts_values[i] = vals[i]; | ||
| oslot->tts_isnull[i] = nulls[i]; | ||
| } |
There was a problem hiding this comment.
This loop processes all iss_NumOrderByKeys keys and asserts every one is pass-by-value, but that invariant is stronger than what is guaranteed. indexorderbytlist > 0 (and hence a non-NULL iss_OrderBySlot) only guarantees that at least one key was replaced in the targetlist, and index_orderby_returnable() restricts only the replaced keys to float8/float4. The non-replaced keys in a mixed multi-key ORDER BY can be any type the AM's ordering operators return -- index_store_float8_orderby_distances() explicitly handles non-float ORDER BY types. For such a key with a non-by-value type, Assert(node->iss_OrderByTypByVals[i]) fires in assert-enabled builds (i.e. all development/buildfarm builds), and in the reorder-queue caller (line ~286) the subsequently-stored Datum is a pointer that reorderqueue_pop() immediately pfrees. Core GiST/SP-GiST only expose point distance (float8), so this is not reachable with in-tree AMs, but it is reachable for third-party amcanreturnorderby AMs. Since the executor has no per-key returnable flag, consider only storing/asserting the keys that were actually replaced, or skipping non-by-value keys rather than asserting on them.
| if (path_emits_extras(subpath)) | ||
| { | ||
| rows = compute_gather_rows(subpath); | ||
| add_path(rel, (Path *) | ||
| create_gather_path(root, rel, subpath, | ||
| subpath->pathtarget, NULL, rowsp)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This loop gathers only the first partial path that emits extras, then breaks. When partial paths emit different extra-value subsets (e.g. two indexes each returning a distinct returnable expression the query needs), only one subset is ever gathered; the other extras-emitting partial path is silently dropped and the optimization is lost for it. Since add_path() treats distinct extra subsets as non-dominating (compare_path_extras returns PATHKEYS_DIFFERENT), there can legitimately be more than one such partial path worth gathering. Consider gathering each distinct extras-emitting partial path rather than breaking after the first. Low severity: affects plan quality, not correctness.
| List *indexorderbyexact; | ||
| int indexorderbytlist; |
There was a problem hiding this comment.
indexorderbyexact is planner scratch state: it is produced in createplan.c and consumed only by replace_orderby_tlist_refs() in setrefs.c (during set_plan_references, the final planning pass). It is never read by the executor or EXPLAIN (confirmed: no references under src/backend/executor or src/backend/commands). As a plain List * field of the IndexScan Node struct it will nonetheless be emitted by gen_node_support.pl into copy/out/read/equal for every index-scan plan, so it is copied and serialized (e.g. for parallel workers / stored plans) despite being dead after setrefs. Consider not persisting it in the plan node -- either pass it transiently to replace_orderby_tlist_refs(), or, if it must stay a field, annotate it to avoid serialization. Note: read_write_ignore alone is not permitted here because IndexScan is readable; it would need read_as(NIL) as well. (moderate confidence)
1 participant
An ordering index scan (
ORDER BY col <-> const) computes each row's ORDER BY value in the index AM. When the target list needs the same expression, the executor evaluates it again. This series lets the scan hand the value up instead, and teaches the planner to cost and choose plans accordingly.amcanreturnorderby(opfamily, opcintype, orderbyop); GiST point_ops, SP-GiST quad/kd point ops)The two
dev:commits under the series are local tooling and are not part of the patch set.