Repository navigation
Conversation
📜 Change history & discussion (Agora / pg.ddx.io)I have everything needed. 🧵 Related discussion
🔗 Related commits / prior art
📋 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.
|
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)
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (along with .envrc, .gdbinit, pg-aliases.sh, flake.nix, shell.nix) is personal developer-environment tooling and does not belong in a PostgreSQL contribution destined for pgsql-hackers/commitfest. It is unrelated to the backend change (the HOT indexed-updates series reflected in the other modified files) and violates the minimal-diff discipline; a reviewer will reject the patch for bundling local scaffolding. The repo's own root .gitignore states that such files 'should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude' -- that is where this belongs, not committed to the tree.
| # Redirect-with-data: initialize bitmap accumulator from collapsed redirect | ||
| break heapam_indexscan.c:182 |
There was a problem hiding this comment.
Absolute source line-number breakpoints (heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299, etc.) are extremely fragile: any edit to these files silently shifts the breakpoint to unrelated code. None of these files are in this changeset, so the line numbers are unverified and almost certainly already stale. If such a helper is kept at all (it should not be committed), it must use function-name/symbol breakpoints, never raw line numbers.
| if $context | ||
| printf "MemoryContext: %s\n", $context->name | ||
| printf " type: %s, parent: %p\n", $context->methods->name, $context->parent | ||
| printf " total: %zu, free: %zu\n", $context->mem_allocated, $context->freep - $context->freeptr |
There was a problem hiding this comment.
print_mcxt references $context->freep and $context->freeptr, which do not exist in MemoryContextData (src/include/nodes/memnodes.h has no freep/freeptr fields). This command will error out when invoked. These pretty-printers encode struct layout that has drifted from the current tree -- a gdbinit that fails to load/print is a maintenance trap. (Also applies to print_proc below.)
| set $proc = (PGPROC*)$arg0 | ||
| if $proc | ||
| printf "PGPROC: pid=%d, database=%u\n", $proc->pid, $proc->databaseId | ||
| printf " waiting: %d, waitStatus: %d\n", $proc->waiting, $proc->waitStatus |
There was a problem hiding this comment.
print_proc reads $proc->waiting, but PGPROC (src/include/storage/proc.h) has no 'waiting' field. Also waitStatus is a ProcWaitStatus enum, not an int, so the %d format and field access assume a layout that no longer matches. This printer will fail when used.
|
|
||
| alias pg-start='ulimit -c unlimited && "$PG_INSTALL_DIR/bin/postgres" -D "$PG_DATA_DIR" -k "$PG_DATA_DIR"' | ||
|
|
||
| alias pg-stop='pkill -f "postgres.*-D.*$PG_DATA_DIR" || true' |
There was a problem hiding this comment.
pg-stop (and the many pgrep -f callers) use an unanchored regex 'postgres.-D.$PG_DATA_DIR'. Since PG_DATA_DIR is /tmp/test-db-, another worktree whose basename is a substring, or any process whose command line merely contains this string, will match and be killed. This is a footgun that can terminate the wrong backend/session. Match more precisely (e.g. anchor the -D argument and the exact data dir).
| if (es->verbose && ((IndexScan *) plan)->indexorderbytlist > 0) | ||
| ExplainPropertyInteger("Order By Values Used", NULL, | ||
| ((IndexScan *) plan)->indexorderbytlist, | ||
| es); |
There was a problem hiding this comment.
The verbose EXPLAIN field exposes a raw internal counter (the number of targetlist entries that replace_orderby_tlist_refs() rewrote), labeled "Order By Values Used". This is an opaque implementation detail: it reports how many ORDER BY values the tlist reuses, not which ones, so a user cannot act on the number. Consider whether this is better surfaced as a boolean-style flag (e.g. shown only when the scan returns its ORDER BY values) rather than a count, or dropped entirely, since the deparsed targetlist already reflects the reuse. Low confidence / design-taste point for the broader feature.
| Oid typ = exprType((Node *) orderby); | ||
|
|
||
| if (typ != FLOAT8OID && typ != FLOAT4OID) | ||
| return false; |
There was a problem hiding this comment.
index_orderby_returnable() is an exported, generic-looking API (takes opfamily/opcintype/orderbyop), but it hardcodes a restriction to FLOAT8OID/FLOAT4OID. The AM's amcanreturnorderby callback is the real authority on whether a non-rechecked ordering value equals the operator's result; this extra type gate silently excludes any (future) AM that could return a non-float ordering value, and the coupling is invisible to the callback. Consider dropping the hardcoded type test and letting amcanreturnorderby decide, or at minimum document in the header why only float types are accepted. (confidence: moderate)
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the stated purpose of this PR (the GiST/SP-GiST index-scan and optimizer path work in the other changed files). It is unnecessary churn and should be dropped from the patch per the minimal-diff rule.
More importantly, it breaks tree-wide convention: every other Perl script in the PostgreSQL source — including its peers in src/tools/ (copyright.pl, git_changelog, mark_pgdllimport.pl) — uses #!/usr/bin/perl. Switching only pgindent to #!/usr/bin/env perl makes it the sole outlier and would be rejected on -hackers. If a env-based shebang were genuinely wanted, it would have to be a separate, tree-wide change with its own justification. Revert this line.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| create table gist_ios_par (i int); | ||
| insert into gist_ios_par select g from generate_series(1, 10000) g; | ||
| create index on gist_ios_par (i, gist_ios_restricted(i)); | ||
| vacuum analyze gist_ios_par; |
There was a problem hiding this comment.
Portability/determinism risk under the parallel schedule. These three non-temp tables (gist_ios_par, gist_ios_gm, gist_ios_ppart) produce expected EXPLAIN output that shows Parallel Index Only Scan (gist.out lines 1108, 1134, 1168-1170). The choice of an Index Only Scan over a plain Index Scan depends on pg_class.relallvisible, i.e. how many pages VACUUM could mark all-visible. For a non-temp table in the gist parallel group, a concurrently running regress session holding an open snapshot holds back OldestXmin, so vacuum analyze here may be unable to set the all-visible bit on all pages. When that happens the planner prefers a (Parallel) Index Scan and the EXPLAIN node name changes, failing the diff.
The added comments pin enable_seqscan/enable_bitmapscan off and address Parallel Append child ordering, but nothing forces IOS over a plain parallel Index Scan (there is no enable_indexscan = off, and that choice is not knob-controlled anyway). This is exactly why the IOS tests just above use temp tables. Parallel scans do require non-temp tables, so consider making the assertions robust to the IOS-vs-IndexScan flip (e.g. assert on behavior via the counting function / row-equality rather than on the exact EXPLAIN node), or otherwise guarantee all-visible marking is deterministic here.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
Unrelated change. Switching execl() -> execlp() has nothing to do with the kNN / amcanreturnorderby feature this patch tests. SHELLPROG is always defined as an absolute path ($(SHELL) in GNUmakefile, /bin/sh in meson.build), so execlp is behaviorally equivalent in standard builds and only differs if shellprog is ever a bare name resolved via PATH (apparently a convenience for non-standard dev environments such as the Nix setup added elsewhere in this change). Per minimal-diff / one-logical-change-per-commit discipline, this belongs in a separate patch, not bundled with a planner/executor feature. If it is kept, it should be justified on its own and ideally carry a comment explaining why PATH lookup is now wanted.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
This .gdbinit pins breakpoints to absolute source line numbers, and they are already wrong/fragile. Verified against the current tree: heapam.c:4019 is a RelationGetBufferForTuple() call (not the claimed "pure HOT" decision), heapam.c:4024 is a VM-pin, heapam.c:4033 is a comment; heapam_indexscan.c:182 is lp = PageGetItemId(...) (not a "bitmap accumulator"), :250 is skip = false;, and :297 is a return inside an unrelated function. Any edit above these lines silently moves the breakpoint to the wrong code -- a debugging footgun. Line-number breakpoints like these should not be committed; use symbol-based breakpoints at minimum, and preferably keep this file out of the tree entirely (see the scope concern below).
| # ========================================================================= | ||
|
|
||
| # 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 symbols do not exist anywhere in the tree. A codebase search finds no heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_* helpers, heap_xlog_indexed_update, or the HEAP_INDEXED_UPDATED flag; pruneheap.c contains the standard pruning code, not the "redirect-with-data"/"collapsed redirect" logic described here. GDB will fail to set these breakpoints ("Function ... not defined"). This .gdbinit describes a "HOT indexed updates" patch series that is not present in this change set -- it references code that was never added, so it is dead scaffolding.
| 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.
Destructive data-loss footgun. If this file is sourced outside the Nix shellHook (which is the only place PG_DATA_DIR is exported), $PG_DATA_DIR is empty and rm -rf "$PG_DATA_DIR" becomes rm -rf "" while pg-stop's pkill -f "postgres.*-D.*$PG_DATA_DIR" degrades to postgres.*-D.*, matching and killing every local postgres process. The script has no set -u and no guard asserting these variables are non-empty before destructive operations. Guard destructive aliases, e.g. : "${PG_DATA_DIR:?PG_DATA_DIR not set}" before the rm/pkill.
| ulimit -c unlimited | ||
|
|
||
| # Local git excludes | ||
| git config core.excludesFile .local-gitignore 2>/dev/null || true |
There was a problem hiding this comment.
Entering the dev shell unconditionally runs git config core.excludesFile .local-gitignore, which silently overwrites any pre-existing repo-local core.excludesFile with no way to restore it on shell exit (POLA violation). There is no committed .local-gitignore or root .gitignore in the tree, so this is the only mechanism intended to hide these dev files -- yet the dev files themselves are committed to the tree rather than being ignored. Either add .local-gitignore with these paths (and do not commit the dev files), or drop this side effect; mutating the user's git config as a side effect of nix develop/direnv is surprising.
| gccPkg = pkgs-unstable.gcc16; | ||
| gdbPkg = pkgs-unstable.gdb; | ||
| llvmPkgs = pkgs-unstable.llvmPackages_22; |
There was a problem hiding this comment.
Reproducibility/ABI hazard: the base nixpkgs is nixos-25.05 (stable) yet the compiler and core toolchain are pulled from nixpkgs-unstable (gcc16, llvmPackages_22, gdb, meson, ninja), while the PostgreSQL build/runtime libraries (readline, openssl, icu, lz4, zstd, libuuid, libkrb5, liburing, ...) come from stable pkgs. gcc16-from-unstable links against unstable glibc, while those libraries link against stable glibc; the flake comments themselves document a prior glibc 2.42-vs-2.40 GLIBC_ABI mismatch with libumem. This cross-channel mix can break the build non-deterministically depending on which glibc wins at link/load time.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to the GiST/KNN EvalPlanQual feature this patch implements, and should be dropped from the series (minimal-diff discipline on -hackers). Functionally it also changes behavior: shellprog comes from SHELLPROG, which is $(SHELL) in the Makefiles and /bin/sh in meson.build -- normally absolute paths. execlp adds a $PATH search for relative shell paths, which is a behavior change motivated by a local dev environment (note the added flake.nix/shell.nix/.envrc Nix tooling), not by this patch. Using $PATH to resolve the shell in the regression driver is also a mild footgun (test shell now depends on environment). Confidence: high.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change breaks PostgreSQL's convention and is an unrelated tweak. Every other Perl script in the tree uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.) - this is the only file using #!/usr/bin/env perl. Changing it here introduces inconsistency and is unrelated to the GiST/planner work that is the subject of this change set, so it fails the minimal-diff rule. If a portability improvement for the interpreter path is genuinely wanted, it belongs in its own patch that updates all Perl scripts consistently and comes with a -hackers discussion. Revert this hunk.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
Beyond being unrelated churn, execlp introduces a security footgun the original execl did not have: execlp performs a PATH lookup when the program name contains no slash. shellprog is SHELLPROG (normally an absolute path, e.g. /bin/sh), so this is inert today, but if SHELLPROG is ever a bare name the shell chosen becomes PATH-dependent in the test harness. Revert to execl (as the confirmed finding also recommends dropping this line entirely).
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| ORDER BY p <-> point(3, 4), a <-> '2004-10-26 08:55:08' LIMIT 3; | ||
| SELECT p <-> point(3, 4) AS d, a <-> '2004-10-26 08:55:08' AS da |
There was a problem hiding this comment.
The final query orders only by p <-> point(3,4) then a <-> '2004-10-26 08:55:08', with no deterministic tie-breaker, and all three output rows share distance 0 on the primary key. Ordering therefore rests entirely on the interval distance. If two rows in the p=(3,4) group were equidistant from the reference timestamp (the static data has timestamps on both sides of it), or shared an interval value, LIMIT 3 row selection/order would be non-deterministic and the test could flake on the buildfarm across platforms. It passes for this exact data file because the three smallest distances happen to be distinct, but consider adding an explicit tie-breaker (e.g. a third ORDER BY column) to make the stability independent of the data. (low confidence)
| INSERT INTO timestamppt SELECT point(i % 7, i % 11), a | ||
| FROM (SELECT row_number() OVER (ORDER BY a) AS i, a FROM timestamptmp) s; |
There was a problem hiding this comment.
The row-to-point mapping is built from row_number() OVER (ORDER BY a), but a is not a total order in this data: data/timestamp.data contains 56 NULLs, 8 -infinity and 16 infinity values (plus any finite duplicates). row_number() breaks ties within each equal-key group arbitrarily (by physical/scan order), so the i assigned to a given timestamp inside those groups is non-deterministic. That makes point(i % 7, i % 11) -> row assignment non-reproducible across platforms/scan orders, which in turn can change which rows land in the p=(3,4) group and hence the committed da values in expected/timestamp.out. It happens to be stable today only because the p=(3,4) i-values (59,136,...,598) avoid the -infinity block (i=1..8) and the one that falls in the infinity/NULL tail (i=598) yields a distance that sorts last. This is fragile. Make the mapping deterministic, e.g. add a total-order tiebreaker (ORDER BY a, ctid is still not stable; better to generate data independent of a, or filter out NULL/infinite a before numbering). (moderate confidence)
| jobs: | ||
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
The sync job runs real git operations (fetch/rebase/push) directly on the runner and has no timeout-minutes. A git rebase that hits unexpected state (conflicts, prompts) or a slow fetch can hang the runner until the 6-hour hard limit, and the concurrency group has no cancel-in-progress so a stuck run blocks every subsequent 3-hourly schedule. This fork's own pg-ci.yml sets timeout-minutes on every job; this job should too (a few minutes is ample for a rebase+push).
| jobs: | |
| sync: | |
| runs-on: ubuntu-latest | |
| steps: | |
| jobs: | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: |
| git rebase upstream/master | ||
| git push --force-with-lease |
There was a problem hiding this comment.
git rebase upstream/master can stop mid-rebase on a conflict, leaving .git in a rebase-in-progress state. Under set -e the step fails (good, it won't push garbage), but there is no git rebase --abort cleanup and no notification. Since this runs unattended every 3 hours, a conflicting upstream change will produce a silently failing job on every run with no signal beyond the Actions tab. Consider aborting the rebase on failure and/or surfacing the failure (the error annotation at line 43 only covers the non-CI-changes guard, not the rebase itself).
| if git diff --name-only upstream/master...HEAD | | ||
| grep -v '^\.github/workflows/fork-'; then |
There was a problem hiding this comment.
The branch-guard grep -v '^\.github/workflows/fork-' only anchors the start of the path, not the end. Any new top-level path that happens to share the .github/workflows/fork- prefix (e.g. a stray .github/workflows/fork-notes.txt) is silently treated as fork CI and force-rebased on a timer, which is exactly the case this guard exists to prevent. If the intent is strictly fork-*.yml workflow files, anchor the pattern end too, e.g. grep -vE '^\.github/workflows/fork-[^/]*\.ya?ml$'.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (along with .envrc, .gdbinit, pg-aliases.sh, flake.nix, shell.nix) is personal developer tooling committed to the repo root. For a fork whose PRs become pgsql-hackers patches, these are unrelated-change noise that violates the minimal/atomic-diff discipline and will draw immediate rejection.
Critically, the fork's own .github/workflows/fork-sync-upstream.yml rebase policy errors out on any master file other than .github/workflows/fork-*.yml (grep -v '^\.github/workflows/fork-'). These root-level dev files are exactly what that guard rejects, so they do not belong on master. They should live on a feature branch or in a personal (git-ignored) location, not in a patch.
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These breakpoints target a "HOT indexed updates" patch series that is not present in this tree. Verified that heap_hot_indexed_create_tuple and heap_xlog_indexed_update do not exist anywhere in the source, so these symbol breakpoints will fail to resolve. The file is misleading and unmaintainable as committed.
| create table gist_ios_par (i int); | ||
| insert into gist_ios_par select g from generate_series(1, 10000) g; | ||
| create index on gist_ios_par (i, gist_ios_restricted(i)); | ||
| vacuum analyze gist_ios_par; |
There was a problem hiding this comment.
The parallel index-only-scan tests create non-temp tables (gist_ios_par, gist_ios_gm, gist_ios_ppart*) inside the gist test, which runs in a large parallel group (parallel_schedule line 69). The plan-shape assertions here depend on VACUUM ANALYZE marking pages all-visible, which the comment itself admits can race against snapshots held by concurrent sessions in the group. The author mitigates this two ways: (1) enable_seqscan/enable_bitmapscan are off and the expression is only available from the covering index, so an index-only scan dominates a plain index scan regardless of allvisfrac; (2) the Parallel Append children have a 4x size difference (2000 vs 8000 rows) so their cost order is stable. Both mitigations look sound, so the EXPLAIN output should be deterministic. Confidence that this is robust: moderate. Worth a maintainer's eye on the buildfarm, since non-temp tables carrying all-visibility-dependent EXPLAIN output under the parallel schedule is historically a flakiness source; if any plan flips, prefer forcing the scan via a planner GUC rather than relying on allvisfrac.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the stated purpose of the patch (the GiST/index-scan feature work in the rest of the changeset) and should be dropped. Beyond violating the "minimal diff" rule, it breaks a tree-wide convention: every other Perl script in PostgreSQL uses #!/usr/bin/perl (e.g. src/tools/copyright.pl, git_changelog, mark_pgdllimport.pl, and all the generators under src/backend and src/common). Switching only pgindent to #!/usr/bin/env perl makes it the sole outlier with no justification. It would also silently pick up whichever perl is first on PATH, which is a behavior change pgindent authors deliberately avoid. Revert to #!/usr/bin/perl.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp switch is unrelated to the GiST KNN / EvalPlanQual feature this series implements; it is a dev-environment (Nix: .envrc/flake.nix/shell.nix) accommodation smuggled into the feature patch. Bundling unrelated changes is the top rejection reason on pgsql-hackers -- split it into its own commit (or drop it). Separately, this is a behavioral change, not a cleanup: SHELLPROG is $(SHELL) (make) / "/bin/sh" (meson), i.e. an absolute path, for which execlp is a no-op; but if it is ever a bare name, execlp performs a PATH search and the test harness would exec whichever shell is first on PATH -- a surprising, PATH-dependent exec in the regress driver (footgun/POLA). If a PATH-resolved shell is genuinely required, justify it and reference the -hackers discussion. Confidence: high.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
Beyond being unrelated to this GiST KNN/EvalPlanQual series, this execl -> execlp switch is a behavioral regression. shellprog is SHELLPROG, a fixed absolute path (/bin/sh, see src/test/regress/meson.build and the Makefile). With an absolute path, execlp behaves identically to execl, so the only observable effect of this change is to enable a $PATH search of the shell name -- which is exactly the Nix-environment accommodation, not a test requirement. Resolving the shell via the inherited $PATH instead of the hardcoded /bin/sh is a footgun: test behavior now depends on the caller's environment and a PATH-injected sh can be picked up. Revert to execl and keep any environment-specific shell resolution out of the test harness. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| -- The planner must not charge an ordering Index Scan for evaluating a | ||
| -- targetlist entry it takes from its ORDER BY values. With a costly ORDER BY | ||
| -- expression, the kNN scan should win over Seq Scan + Sort even when the | ||
| -- filter is selective, since the Sort plan really does evaluate it per row. |
There was a problem hiding this comment.
The comment claims this test shows "the kNN scan should win over Seq Scan + Sort even when the filter is selective", but enable_seqscan is still off here (set at line 54 and not reset until line 379). With Seq Scan disabled it gets disable_cost added, so the Index Scan is chosen trivially regardless of the costing change being tested; this EXPLAIN would produce the same plan even with the feature reverted, so it does not catch a regression in the per-row charge. The actual regression-catching check is the numeric gist_costly_total(...) < 10000 assertion below. Either re-enable seqscan here so the comparison is real, or reword the comment to describe what this EXPLAIN actually demonstrates (plan shape only).
| source_up | ||
| watch_file flake.nix | ||
| use flake |
There was a problem hiding this comment.
Developer-local direnv file referencing personal paths (~/ws/postgres/.envrc) and private helpers (source_up, pg_cflags, use project_steering ..., use project_mcp postgresq). This is personal workflow scaffolding that does not belong in a pgsql-hackers patch; the root .gitignore documents ignoring such files via $GIT_DIR/info/exclude. Note also use project_mcp postgresq looks like a truncated/typo'd argument. Remove from the patch.
Confidence: high.
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Line-numbered breakpoints are already wrong and are a committed personal-workflow file that does not belong in a pgsql-hackers patch (root .gitignore documents using $GIT_DIR/info/exclude for such files).
Verified against the current tree: break heapam.c:3210 is claimed to be "Main entry: heap_update", but heapam.c:3210 is the closing brace of heap_delete. Line-pinned breakpoints silently bind to unrelated code the moment any line shifts, so the file is actively misleading even to its author.
Furthermore, the symbols this file documents do not exist anywhere in the tree: heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, XLOG_HEAP2_INDEXED_UPDATE, and heap_xlog_indexed_update appear ONLY inside .gdbinit. This couples an infrastructure/tooling commit to unlanded feature commits, violating the atomic/bisectable-commit rule: the file documents code that is not present in this change.
Confidence: high.
| ParameterNames: true | ||
| DeducedTypes: true | ||
| CompileFlags: | ||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json |
There was a problem hiding this comment.
This is a developer-local workflow file, not PostgreSQL feature code. The root .gitignore explicitly documents the policy: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." A patch destined for pgsql-hackers/a commitfest must not commit per-author tooling. Additionally the include paths -I. and -I../../../../src/include and CompilationDatabase: build/ are tied to one author's worktree layout and will not resolve for other contributors. Remove from the patch; keep it under a local exclude mechanism.
Confidence: high.
| @@ -0,0 +1,63 @@ | |||
| { | |||
| description = "PostgreSQL development environment"; | |||
There was a problem hiding this comment.
This Nix flake defines a personal developer environment and does not belong in a PostgreSQL patch for pgsql-hackers/a commitfest. PostgreSQL's build system is autoconf/meson; the project does not use Nix flakes, and the root .gitignore documents keeping local-workflow files under $GIT_DIR/info/exclude. Remove from the patch.
Separately (if this were to live anywhere), the libumem input is pinned to a release tag rather than a locked rev+hash in-expression, but reproducibility is governed by flake.lock here; the dominant issue is that the whole file is out-of-scope local tooling.
Confidence: high.
| pg_clean_for_compiler() { | ||
| local current_compiler="$(basename $CC)" | ||
| local build_dir="${1:-$PG_BUILD_DIR}" |
There was a problem hiding this comment.
This ~700-line shell helper is personal workflow tooling, not PostgreSQL feature code, and does not belong in a patch for pgsql-hackers/a commitfest (root .gitignore documents using $GIT_DIR/info/exclude for such files).
Footgun: the destructive aliases depend entirely on environment variables that are only exported by shell.nix's shellHook. Sourced outside the Nix dev shell, $PG_BUILD_DIR/$PG_INSTALL_DIR/$PG_DATA_DIR are unset, and pg_clean_for_compiler then runs trash "$build_dir" || rm -rf "$build_dir" with an empty path. More dangerous: pg-full-clean / pg-init below run rm -rf / trash on these same unset variables. This is a data-loss hazard if ever sourced in the wrong context.
Confidence: high (that it is local tooling); moderate (on exact blast radius of the unset-var rm).
| -- An index-only scan isn't charged for evaluating a target expression it | ||
| -- reads from a returnable index expression column: it reads the stored | ||
| -- value instead. (Not GiST-specific; btree returns expression columns.) |
There was a problem hiding this comment.
By the author's own admission ("Not GiST-specific; btree returns expression columns"), this entire ~200-line index-only-scan block -- plus the parallel (Gather/Gather Merge/Parallel Append) and partition-wise cases below -- is general planner/executor coverage, not GiST coverage. Burying btree IOS-of-expression tests in gist.sql harms discoverability and bloats a suite that already carries GiST-specific tests. Consider moving the non-GiST portions to a more general planner/IOS test file (e.g. a dedicated test in the parallel schedule) so the coverage is found where reviewers expect it.
| create function gist_ios_total(q text) returns float8 language plpgsql as | ||
| $$ declare j json; begin | ||
| execute 'explain (format json) ' || q into j; | ||
| return (j->0->'Plan'->>'Total Cost')::float8; | ||
| end $$; |
There was a problem hiding this comment.
gist_ios_total is a near-identical copy of gist_costly_total (defined at line 348): both run EXPLAIN (format json) and return the top plan's Total Cost. Define one helper once and reuse it to avoid the copy-paste, per DRY. (They are created and dropped in disjoint sections, so a single helper created earlier and dropped at the end would work.)
| tlist_exprs = get_tlist_exprs(root->processed_tlist, true); | ||
| if (!bms_is_empty(root->outer_join_rels)) | ||
| tlist_exprs = (List *) strip_nullingrels((Node *) tlist_exprs); |
There was a problem hiding this comment.
DRY: this inlines exactly what the static helper query_tlist_exprs() in pathnode.c does for a simple base rel (get_tlist_exprs + strip_nullingrels when outer-join rels exist). indexonly_path_target() already funnels through query_tlist_exprs() for the same purpose. Either export and reuse query_tlist_exprs() here, or factor the shared logic, so the two copies can't drift. (moderate confidence)
| if (all_kept) | ||
| { | ||
| Path *inputs[2] = {outer_path, inner_path}; |
There was a problem hiding this comment.
The fast-path cost delta assumes each input emits a disjoint set of extras so the two per-input deltas never double-count. That holds only because add_new_column_to_pathtarget() dedups via equal() and base-rel Vars from the two sides can't be equal. If all_kept is true but an expr from inner_extras was equal() to one already added from outer_extras (so the second add was a no-op), this adds both inputs' width/cost deltas while target gained only one column, over-charging the path. Worth an Assert that list_length(target->exprs) - list_length(joinrel->reltarget->exprs) equals the number of extras actually appended, to catch a future violation of the disjointness assumption. (low confidence)
| explain (verbose, costs off) | ||
| select gist_ios_costly(a.i), d.tag from gist_ios_cost a | ||
| join gist_ios_dim d on d.i = a.i; | ||
| set enable_hashjoin = off; |
There was a problem hiding this comment.
This explain (verbose, costs off) asserts a specific join algorithm (the expected file records Merge Join) while enable_hashjoin/enable_mergejoin are both still enabled here (they are only turned off at the following lines). The merge-vs-hash cost decision for these two ~1.4k/10k-row inputs is close and known to flip across the buildfarm (different page counts, costing, architectures), which would churn this expected output. The point of this test is that the Index Only Scan emits the value and the join reads it -- that is independent of the join method. Pin the join method (e.g. set enable_mergejoin = off/set enable_hashjoin = off to force one shape) so the plan is deterministic. The same concern applies to the later join EXPLAINs that assert Merge Join / Hash Join with methods still enabled (the gist_ios_wide case and the two gist_ios_part cases).
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
This .envrc depends on personal direnv stdlib helpers (source_up, pg_cflags, use project_steering, use git_policy allow-force-push, use project_mcp) that are not present in the repo, so direnv will error out for anyone else. Also use project_mcp postgresq looks like a truncated/typo'd argument ("postgresq"). This is leaked personal config and should not be committed; per the repo's own .gitignore (lines 1-3), local-workflow files belong in $GIT_DIR/info/exclude or ~/.gitexclude, not in the tree.
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These symbolic breakpoints target functions/records that do not exist anywhere in the tree: heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_search_buffer, heap_xlog_indexed_update, log_heap_update with indexed_update handling, plus macros HEAP_INDEXED_UPDATED/XLOG_HEAP2_INDEXED_UPDATE referenced in comments. I searched .c/.h and found no matches. GDB will silently fail to set these (Function "..." not defined), making the file a broken/aspirational artifact. Combined with the hardcoded line numbers below (heapam.c:3210, heapam.c:4019, heapam_indexscan.c:182, pruneheap.c:1802, indexam.c:299, execIndexing.c:370), which drift to the wrong code on any edit, this debugging aid is actively misleading.
|
|
||
| # Clean commands | ||
| alias pg-clean='ninja -C "$PG_BUILD_DIR" clean' | ||
| 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"' |
There was a problem hiding this comment.
Destructive rm -rf fallback on unguarded variables: if PG_BUILD_DIR/PG_INSTALL_DIR are empty or unset (e.g. the alias is sourced outside the Nix shell that exports them), this expands to rm -rf "" "", and for pg-init to rm -rf "". While rm -rf "" is a no-op in most shells, relying on that is a footgun; other sites like pg_clean_for_compiler take ${1:-$PG_BUILD_DIR} and could receive a partially-set path. Add a guard (e.g. : "${PG_BUILD_DIR:?}") before the destructive expansion. Separately, trash is used here and in the valgrind/asan wrappers but is not declared in shell.nix postgreSQLDeps, so the rm -rf fallback is the common path.
| --dlpath="$bdir/src/test/regress" \ | ||
| --outputdir="$outdir" \ | ||
| --temp-instance="$outdir/tmp" \ | ||
| --port=40099 \ |
There was a problem hiding this comment.
Hardcoded --port=40099 collides across concurrent worktrees/runs: PG_DATA_DIR is per-worktree (/tmp/test-db-$(basename $PWD)) so multiple shells can run pg-test-one simultaneously, but they will all try to bind the same fixed port and the second run fails with an address-in-use error. Derive the port from the worktree (as PG_DATA_DIR is) or let pg_regress pick a free port.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (and .envrc, .gdbinit, flake.nix, shell.nix, pg-aliases.sh) is personal developer scaffolding unrelated to the HOT-indexed-updates change. None of it is referenced by any build/test machinery (no meson.build / Makefile mentions). PostgreSQL's minimal-diff discipline rejects unrelated files in a patch, and the repo's own root .gitignore (lines 1-3) explicitly states that local-workflow/editor files should be ignored via $GIT_DIR/info/exclude or ~/.gitexclude rather than committed. These should be dropped from the patch destined for pgsql-hackers.
| sysstat | ||
| ccache | ||
| cppcheck | ||
| compdb |
There was a problem hiding this comment.
trash is used by pg-aliases.sh (pg-full-clean, pg-init, pg_clean_for_compiler, and the valgrind/asan wrapper cleanup) as the preferred deletion tool, but it is not listed in postgreSQLDeps, so inside this dev shell the scripts always fall through to the raw rm -rf fallback. If the intent is to use trash as a safety net, declare it here (e.g. trash-cli); otherwise the trash ... || rm -rf pattern is dead and should be simplified to plain rm -rf with proper variable guards.
| libumem = libumemPkg; | ||
| }; | ||
|
|
||
| environment.localBinInPath = true; |
There was a problem hiding this comment.
environment.localBinInPath is a NixOS module option, not a valid flake output. Placed here inside the per-system attrset returned by eachDefaultSystem, it does nothing (it is not part of the flake output schema and is never consumed by any NixOS module evaluation). It is dead configuration and will be silently ignored; remove it to avoid misleading readers into thinking it affects PATH.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the feature this patch implements (GiST/SP-GiST index-scan optimizer work) and should not be in it. Minimal-diff discipline: unrelated churn is a top rejection reason on -hackers.
It also breaks tree-wide convention. Every other Perl script in the source tree uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.); with this change pgindent becomes the sole outlier, which pgindent's own repo consistency and the buildfarm would flag.
The switch to env perl appears to be a side effect of the author's local Nix dev environment (the companion .envrc/flake.nix/shell.nix additions in this PR), where perl is not at /usr/bin/perl. That is a local-workflow concern and must not be committed to the tree. Revert this hunk.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl->execlp change is unrelated to the KNN/EvalPlanQual feature this patch implements and should be dropped (minimal-diff discipline). shellprog comes from SHELLPROG, defined as $(SHELL) in src/test/regress/GNUmakefile and /bin/sh in meson.build -- an absolute path, for which execl already works. execlp only adds PATH-searching for relative program names, which nothing in this patch introduces; it looks like an accommodation for a local (Nix) dev shell (cf. the added .envrc/flake.nix/shell.nix). Changing the behavior of the regression harness on all platforms to support one contributor's environment is exactly the kind of unrelated hunk that gets a patch rejected on -hackers. Revert this line. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
|
Follow-up on the review rounds since my last comment. Tip is now ce7034e; CI is green on all 10 jobs, and the last OCR pass found nothing in the patch code (its 9 comments are all on the dev-tooling commits). Fixed
Not changed
The one CI failure in between was environmental and passed on re-run: an |
382904a to
14fea8a
Compare
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (and the sibling .envrc, .gdbinit, pg-aliases.sh, flake.nix, shell.nix, flake.lock) is personal developer-environment scaffolding that does not belong in a patch destined for pgsql-hackers. The repo's own top-level .gitignore header states that "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude" -- not committed. Committing these guarantees rejection on the list as a non-minimal diff and creates needless merge conflicts. Keep them in a local gitignore / untracked worktree instead. This is the dominant issue for the whole change.
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These absolute line-number breakpoints (heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299, etc.) are inherently stale-prone. heapam.c is not even part of this change series, and any edit above a cited line silently shifts the breakpoint to unrelated code, giving a false sense of debugging coverage. Line-number breakpoints are unmaintainable in a tree under active development; prefer function-name breakpoints (as already done elsewhere in this file, e.g. break heap_hot_search_buffer) or drop the file entirely since it is personal tooling.
| ulimit -c unlimited | ||
|
|
||
| # Local git excludes | ||
| git config core.excludesFile .local-gitignore 2>/dev/null || true |
There was a problem hiding this comment.
This unconditionally rewrites the user's per-repository git config on every nix develop / direnv entry, with no guard or opt-out. Pointing core.excludesFile at .local-gitignore silently hides files from git status (POLA violation) and can mask artifacts the user expects to see. At minimum make this opt-in, or rely on $GIT_DIR/info/exclude which does not require mutating tracked-affecting config.
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
These direnv helpers (source_up, pg_cflags, use project_steering ..., use git_policy ..., use project_mcp ...) depend on a private global direnv library and a parent ~/ws/postgres/.envrc that no one else has; anyone else entering the repo gets direnv errors. This file is machine-specific and non-reproducible. Also note postgresq on the last line is a truncated typo (missing trailing l), which would break the helper even on the author's own machine.
|
|
||
| # Clean commands | ||
| alias pg-clean='ninja -C "$PG_BUILD_DIR" clean' | ||
| 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"' |
There was a problem hiding this comment.
These destructive aliases depend on $PG_DATA_DIR / $PG_BUILD_DIR / $PG_INSTALL_DIR, which are only exported by shell.nix's shellHook. commonHookTail sources this file via ./pg-aliases.sh; if it is ever sourced outside the Nix dev shell these variables expand empty, turning e.g. rm -rf "$PG_DATA_DIR" into rm -rf "" or operating on an unintended CWD-relative path. This is a data-loss footgun -- guard each alias with a [ -n "$PG_DATA_DIR" ] / ${PG_DATA_DIR:?} check before any rm/trash.
| if (es->verbose && ((IndexScan *) plan)->indexorderbytlist > 0) | ||
| ExplainPropertyInteger("Order By Values Used", NULL, | ||
| ((IndexScan *) plan)->indexorderbytlist, | ||
| es); |
There was a problem hiding this comment.
This exposes an internal setrefs.c counter (the number of targetlist expressions rewritten to INNER_VAR) as a user-visible EXPLAIN property. A bare "replacements made" count is an implementation detail, not plan semantics a user can act on, and it's an odd thing to surface in EXPLAIN output. Consider whether this belongs in the committed patch at all, or whether a boolean/semantically-named property would be clearer. If kept, it needs documentation in doc/src/sgml/ (not in this review group) since it is user-visible and will appear in every verbose plan of an ordering IndexScan. (low-to-moderate confidence; design/taste, not a correctness bug.)
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
Unrelated, no-op change bundled into the KNN/amcanreturnorderby EvalPlanQual feature patch. shellprog is always SHELLPROG, which is $(SHELL) (make) or /bin/sh (meson) — an absolute path, and it is never reassigned. execlp only performs a PATH search when the program name contains no slash; for an absolute path it behaves identically to execl. So this hunk changes nothing functionally and does not belong in this feature patch. Drop it to keep the diff minimal; if a PATH search for the shell is genuinely wanted, it is a separate change needing its own justification and portability analysis (and shellprog would need to actually be set to a bare name somewhere).
Confidence: high.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the stated purpose of the change set and breaks the tree-wide convention: every other Perl script in the PostgreSQL tree (30+ files, e.g. genbki.pl, gen_node_support.pl, mark_pgdllimport.pl, copyright.pl, git_changelog) uses #!/usr/bin/perl. Making pgindent the sole outlier with #!/usr/bin/env perl is unrelated churn that will draw a rejection on -hackers for not being a minimal diff. Separately, /usr/bin/env perl is deliberately avoided in this tree because it resolves perl from PATH, which is less reproducible/secure than the fixed interpreter path. Please drop this hunk.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| create function gist_ios_total(q text) returns float8 language plpgsql as | ||
| $$ declare j json; begin | ||
| execute 'explain (format json) ' || q into j; | ||
| return (j->0->'Plan'->>'Total Cost')::float8; | ||
| end $$; |
There was a problem hiding this comment.
gist_ios_total(text) is a byte-for-byte copy of gist_costly_total(text) defined earlier (both extract 'Plan'->>'Total Cost' from explain (format json)). Since the two live in separate sub-blocks and each is dropped before the other is created, consider defining a single helper once and dropping it at the end, to avoid the copy-paste. Minor; not blocking.
| else if (var->varno == INNER_VAR && IsA(dpns->plan, IndexScan) && | ||
| ((IndexScan *) dpns->plan)->indexorderbytlist > 0) |
There was a problem hiding this comment.
This branch dereferences dpns->plan via IsA() guarded only by var->varno == INNER_VAR. Every neighboring branch instead gates on a NULL-safe tlist pointer (dpns->outer_tlist / inner_tlist / index_tlist), which is implicitly NULL only when dpns->plan is NULL. get_variable()/get_name_for_var_field() both explicitly handle dpns->plan == NULL, so resolve_special_varno can be entered in a no-plan (parse-tree) deparse context. If an INNER_VAR Var ever reaches here with dpns->plan == NULL, IsA() segfaults on the NULL deref. In practice INNER_VAR Vars only appear in plan trees, so this is defensive rather than a confirmed crash, but to match the surrounding NULL-safe pattern the condition should guard the plan pointer first.
| else if (var->varno == INNER_VAR && IsA(dpns->plan, IndexScan) && | |
| ((IndexScan *) dpns->plan)->indexorderbytlist > 0) | |
| else if (var->varno == INNER_VAR && dpns->plan != NULL && | |
| IsA(dpns->plan, IndexScan) && | |
| ((IndexScan *) dpns->plan)->indexorderbytlist > 0) |
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
This entire file is developer-local scaffolding and must not be committed to the tree. It hardcodes absolute line-number breakpoints (e.g. heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299) and symbol breakpoints (heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, heap_xlog_indexed_update, ...) for an unrelated "HOT indexed updates" patch series. None of these functions exist in this review group (searching the tree finds them only inside this .gdbinit), so the breakpoints are both unrelated to any file here and immediately stale. Line-number breakpoints also drift the instant surrounding code changes. This file should be in a local git-excludes file, not tracked.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
Developer-local editor configuration. It pins personal include paths (-I../../../../src/include) and a build/ compile-commands dir that only make sense in the author's worktree layout. This does not belong in a PostgreSQL patch destined for pgsql-hackers; it is non-minimal, non-portable clutter that will cause needless churn. Track it via a local git-excludes file instead.
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
This direnv file sources machine-specific, author-local helpers (source_up pulling ~/ws/postgres/.envrc, use project_steering postgresql aws c, use project_mcp postgresq) that are not reproducible outside the author's machine. The use project_mcp postgresq argument also looks truncated/misspelled (postgresq vs postgresql). This file must not be committed.
| 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.
Destructive data-loss footgun: pg-init runs rm -rf "$PG_DATA_DIR" gated only on an environment variable that may be empty/unset if the Nix/direnv hook did not run. With an empty PG_DATA_DIR, this (and similarly pg-full-clean's rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR") can target unintended paths. These unguarded destructive operations should not ship in a tracked file; at minimum they need [ -n "$PG_DATA_DIR" ] guards.
| @@ -0,0 +1,706 @@ | |||
| # PostgreSQL Development Aliases | |||
There was a problem hiding this comment.
This 700-line shell helper library is personal developer workflow tooling (build/debug/benchmark aliases) unrelated to any PostgreSQL feature. It should not be tracked in the tree; move it to a local git-excludes file.
| /* | ||
| * When the plan's targetlist reads the ORDER BY values (indexorderbytlist | ||
| * > 0), the values of the row being returned, as a virtual slot installed | ||
| * as the projection's inner tuple. | ||
| */ | ||
| TupleTableSlot *iss_OrderBySlot; |
There was a problem hiding this comment.
Minor documentation consistency: the struct's leading field-description block (the OrderByTypByVals ... PscanLen list just above) documents every other iss_* field but omits iss_OrderBySlot. For consistency with the established convention in this struct, add a one-line entry there (e.g. OrderBySlot ORDER BY values of returned row, for the targetlist). Low severity; the inline comment is otherwise clear and correct.
| if (IsA(path, IndexPath) && path->pathtype == T_IndexOnlyScan) | ||
| return free_exprs_cost(root, target, | ||
| indexonly_free_exprs((IndexPath *) path), | ||
| false); |
There was a problem hiding this comment.
For every index-only scan path, indexonly_free_exprs() walks the entire index->indextlist and builds a fresh List, even when the index has no returnable non-Var expression columns (the common case). This runs here via path_target_cost() and again via indexonly_qual_cost() in cost_index() for the same path. Consider early-outing when index->indexprs == NIL (mirroring the guard in indexonly_path_target()) before allocating, to avoid needless palloc churn on the hot index-costing path for ordinary index-only scans. (low confidence; bounded by index width, but it is unconditional per-path allocation on a hot path)
| * Used to compare an expression read below an outer join with the query's | ||
| * copy above it (see relabel_extra_for_join() in pathnode.c). |
There was a problem hiding this comment.
The strip_nullingrels comment references relabel_extra_for_join() in pathnode.c, but the actual callers of this helper are query_tlist_exprs() in pathnode.c (line 1410) and fix_join_expr/fix_upper_expr paths in setrefs.c (lines 3263, 3280); relabel_extra_for_join() does not call it. Point the reader at a real caller so the doc doesn't drift from the code. (moderate confidence)
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the KNN/EvalPlanQual feature this PR implements, and it breaks tree-wide convention: every other Perl script under src/ uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.). Switching just this one script to #!/usr/bin/env perl introduces an inconsistency and looks like a stray change from a local Nix/dev-environment setup (cf. the added flake.nix/shell.nix). Drop this hunk from the feature patch; if the env-based shebang is genuinely wanted, it belongs in a separate, standalone patch that converts the scripts consistently and is discussed on its own.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl->execlp change is unrelated to the KNN/EvalPlanQual feature and should be split out of the patch. Functionally it is also a behavior change: SHELLPROG is defined as an absolute path ($(SHELL) in GNUmakefile, "/bin/sh" in meson.build), for which execl and execlp are equivalent. execlp only differs when shellprog has no '/', in which case it searches PATH -- i.e. this only helps a non-standard/overridden SHELLPROG (looks tied to the added Nix dev-environment files). Making the test harness resolve its shell via PATH is a footgun (PATH-dependent, non-reproducible test runs). Drop this hunk from the feature patch.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| # Inherit shared CFLAGS from ~/ws/postgres/.envrc, load this worktree's | ||
| # flake, then layer the postgres agent context (helpers need flake's PATH + | ||
| # write into this worktree's dir). | ||
| source_up |
There was a problem hiding this comment.
This entire file (and the sibling .clangd/.gdbinit/flake.nix/shell.nix/pg-aliases.sh) is personal developer scaffolding that must not be committed to the PostgreSQL tree. The root .gitignore explicitly states local-workflow files should be excluded via $GIT_DIR/info/exclude or /.gitexclude, not committed. This file in particular hardcodes a private path (/ws/postgres/.envrc) and references non-public direnv extensions (use project_steering, use project_mcp, use git_policy allow-force-push) that are meaningless to anyone else and will not evaluate. A pgsql-hackers patch must be minimal; drop all of these from the submission.
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Line-number breakpoints are fragile and already wrong. heapam.c:3210 is not the entry of heap_update(): in the current tree line 3210 is the closing brace of a function epilogue (return TM_Ok; }), so this break lands on the wrong statement. Any edit above these lines silently drifts every file:line break here (heapam.c:4019/4024/4033/4101/4147, pruneheap.c:1802/1836/1863/1287/2936, indexam.c:299, heapam_indexscan.c:182/250/297, execIndexing.c:370). Prefer symbol-based breakpoints over hardcoded line numbers.
| # 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 (heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_bitmap_* , heap_hot_indexed_read_bitmap, heap_xlog_indexed_update, etc. appear only in this .gdbinit). The heap-side "HOT indexed updates" functions this file is meant to debug were never added by this change set, so GDB will fail to set these breakpoints. This file does not belong in the patch and is non-functional against the actual source.
| CompileFlags: | ||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json |
There was a problem hiding this comment.
These include hacks (-I., -I../../../../src/include) and CompilationDatabase: build/ are workstation-specific and will not match other contributors' layouts; MissingIncludes: None additionally masks real include errors. This is editor-only config that should not ship in a pgsql-hackers patch.
| pg-disable-cores() { | ||
| ulimit -c 0 | ||
| if ! [ -w /proc/sys/kernel/core_pattern ]; then | ||
| echo "Restoring kernel.core_pattern to 'core' (requires sudo)..." | ||
| echo "core" | sudo tee /proc/sys/kernel/core_pattern >/dev/null || { |
There was a problem hiding this comment.
pg-enable-cores/pg-disable-cores silently escalate to root (sudo tee) and mutate the host-global sysctl kernel.core_pattern, and pg-disable-cores restores it to the hardcoded literal "core" rather than the value captured before pg-enable-cores ran. Anyone sourcing this file can leave the host in a different global state than before. This is a footgun that should not be committed.
| create index on gist_ios_ppart (i, gist_ios_restricted(i)); | ||
| vacuum analyze gist_ios_ppart; | ||
| explain (verbose, costs off) | ||
| select gist_ios_restricted(i) from gist_ios_ppart; |
There was a problem hiding this comment.
Same determinism concern: Parallel Append + Parallel Index Only Scan over non-temp partitions depends on all-visible state and child-path cost ordering, both sensitive to concurrent snapshots in the gist parallel group. The following count(*), sum(v) check already proves correctness independent of plan shape.
| * nonzero, and EXPLAIN shows it. | ||
| */ | ||
| List *indexorderbyexact pg_node_attr(read_write_ignore, read_as(NIL)); | ||
| int indexorderbytlist; |
There was a problem hiding this comment.
indexorderbytlist is an int count (it's only ever tested with > 0, printed via ExplainPropertyInteger, and assigned nrepl in setrefs.c), but the tlist suffix is the planner's long-standing convention for a List * (target list). A reader scanning this struct will reasonably expect a List and be misled. Consider a name that reflects it being a count, e.g. indexorderbynrepl or norderbyvals, so the type and the name agree.
| int indexorderbytlist; | |
| int indexorderbyvalcount; |
| SELECT point(i % 7, i % 11), '2004-10-26 08:55:08'::timestamp + i * interval '37 min' | ||
| FROM generate_series(1, 600) i; | ||
| CREATE INDEX timestamppt_idx ON timestamppt USING gist (p, a); | ||
| SET enable_indexonlyscan = off; |
There was a problem hiding this comment.
Minor style inconsistency with the rest of this directory. Every other btree_gist SQL test writes SET <guc>=<value>; with no spaces around = (e.g. SET enable_seqscan=off; at line 23 of this same file). For consistency, prefer SET enable_indexonlyscan=off; and RESET enable_indexonlyscan; stays as is.
| SET enable_indexonlyscan = off; | |
| SET enable_indexonlyscan=off; |
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to this PR's purpose (KNN / amcanreturnorderby EvalPlanQual re-check) and must not be bundled here. shellprog is SHELLPROG, which is always an absolute path ($(SHELL) from the makefile, /bin/sh from meson), so an absolute path is passed. execl executes exactly that pathname; execlp adds a $PATH search that is pointless for an absolute path and introduces a footgun: if the configured shell path ever lacks a slash, pg_regress would run whatever shell comes first on $PATH instead of the configured one. This looks like an accommodation for a local Nix dev environment (cf. the added flake.nix/shell.nix/.envrc) where /bin/sh may be absent; that does not belong in a feature patch destined for -hackers. Revert this line. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the PR's purpose (GiST/SPGiST index-scan and planner/optimizer work) and should be dropped from the patch. It is also inconsistent with the rest of the tree: every other Perl script in the repository (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~35 others) uses the canonical #!/usr/bin/perl. Changing this single file to #!/usr/bin/env perl makes pgindent the sole outlier and introduces inconsistency without justification. Unrelated churn like this is a top rejection reason on -hackers; revert this line to keep the diff minimal. If a /usr/bin/env convention is genuinely desired, it belongs in a separate, tree-wide patch with its own discussion.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
[high confidence] This file should not be committed. .clangd, .envrc, .gdbinit, pg-aliases.sh, flake.nix, and shell.nix are all personal developer tooling with no bearing on the HOT-indexed-updates feature. A patch destined for pgsql-hackers/commitfest must be minimal and contain only files the feature requires; a local Nix dev shell, direnv config, and a .gdbinit of personal breakpoints are exactly the kind of unrelated churn that gets a patch rejected on the list. The tree's own root .gitignore already documents the convention: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Keep these out of the commit (local exclude), rather than in the tree.
Additionally, -DPGDLLIMPORT= and the extra warning flags (-Wconversion, -Wfloat-equal, -Wshadow, -Werror=vla) diverge from PostgreSQL's standard meson build and will produce misleading local diagnostics; redefining the ABI-critical PGDLLIMPORT macro even just for clangd is a footgun.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
[high confidence] Line-number breakpoints are fragile and already partly stale. Breakpoints pinned to absolute source lines (heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299, execIndexing.c:370, heapam_indexscan.c:182, etc.) silently shift onto the wrong statement after any edit to those files -- a debugging footgun that rots immediately. Beyond that, several symbolic breakpoints here reference functions that do not exist anywhere in this tree: heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, heap_hot_indexed_read_bitmap, and heap_xlog_indexed_update have no definitions in any .c/.h (grep returns only this file). So the file is already out of sync with the code it claims to debug. This is personal tooling that should not be committed (see .gitignore convention); keep it local via $GIT_DIR/info/exclude.
|
|
||
| # Clean commands | ||
| alias pg-clean='ninja -C "$PG_BUILD_DIR" clean' | ||
| 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"' |
There was a problem hiding this comment.
[moderate confidence] Same unguarded destructive-removal concern as pg-init: pg-full-clean runs trash/rm -rf against $PG_BUILD_DIR and $PG_INSTALL_DIR, which are empty when the script is sourced outside the Nix shell. Add an explicit non-empty guard before deleting.
| 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.
[moderate confidence] Destructive rm -rf on unguarded variables is a data-loss footgun. PG_DATA_DIR/PG_BUILD_DIR/PG_INSTALL_DIR are only set by shell.nix's commonHookTail. If this script is sourced outside the Nix dev shell (it is a plain *.sh and the aliases evaluate at call time), those variables expand to empty and rm -rf "$PG_DATA_DIR" / trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" become rm -rf "" or worse. Guard these targets (e.g. [ -n "$PG_DATA_DIR" ] || return 1) before any destructive removal. This also reinforces that the file is local-only tooling that should not be committed to the PostgreSQL tree.
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
[high confidence] Non-portable personal config. source_up reaches into a personal ~/ws/postgres/.envrc, and pg_cflags, use project_steering postgresql aws c, and use project_mcp postgresq depend on private direnv library functions that are not defined anywhere in this repo (grep for project_steering/project_mcp matches only this file). Committed, this file fails direnv allow for every other developer and leaks a personal workspace layout. Note also the typo postgresq (missing trailing l). This file should be kept local, not committed.
| ulimit -c unlimited | ||
|
|
||
| # Local git excludes | ||
| git config core.excludesFile .local-gitignore 2>/dev/null || true |
There was a problem hiding this comment.
[moderate confidence] Dangling reference to a file not in the tree. This sets core.excludesFile .local-gitignore, but .local-gitignore does not exist anywhere in the repository (it is referenced only here). Any developer entering the shell will have git's excludes file silently point at a missing path. This ties into the broader issue that shell.nix and its sibling tooling files are personal dev config that should be kept local (per the root .gitignore convention) rather than committed.
| gccPkg = pkgs-unstable.gcc16; | ||
| gdbPkg = pkgs-unstable.gdb; | ||
| llvmPkgs = pkgs-unstable.llvmPackages_22; |
There was a problem hiding this comment.
[low confidence] Moving toolchain pins hurt reproducibility. The dev shell is built on gcc16 and llvmPackages_22 taken from nixpkgs-unstable, which float as the unstable channel advances; combined with inputs tracked by branch URL in flake.nix, reproducibility relies entirely on the committed flake.lock. For PostgreSQL development this unstable toolchain can also surface compiler diagnostics/behavior that do not match the buildfarm's supported compilers. Not an evaluation failure, but worth pinning to a stable toolchain for a shared tree -- and this is another reason the whole dev-shell set belongs in a local-only config rather than the committed tree.
| if (es->verbose && ((IndexScan *) plan)->indexorderbynrefs > 0) | ||
| ExplainPropertyInteger("Order By Values Used", NULL, | ||
| ((IndexScan *) plan)->indexorderbynrefs, | ||
| es); |
There was a problem hiding this comment.
indexorderbynrefs counts the number of targetlist entries that were rewritten to read the scan's ORDER BY values, not the number of distinct ORDER BY keys. If the same ORDER BY expression appears more than once in the targetlist, this count can exceed the number of ORDER BY keys, so the label "Order By Values Used" is slightly misleading. More fundamentally, surfacing a raw internal replacement counter is of marginal value to users compared to, say, indicating which ORDER BY keys are served directly from the index. Consider whether this verbose field is worth exposing at all, and if so, reword the label to describe what the number actually is.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to the KNN/GiST feature in this PR and should be dropped from the patch. shellprog is SHELLPROG, which is $(SHELL) from the Makefile (or hardcoded /bin/sh in meson.build) -- always an absolute path. execlp only differs from execl by performing a PATH search for non-absolute program names, which is a no-op for an absolute path. This looks like a leftover accommodation for a local Nix environment (cf. the added flake.nix/shell.nix/.envrc where /bin/sh may be absent); it does not belong in the committed tree. pgsql-hackers routinely rejects patches that bundle unrelated environment-specific changes. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is unrelated to the PR's purpose and breaks the tree-wide convention. Every other Perl script in the PostgreSQL source tree (30+ scripts, e.g. src/backend/catalog/genbki.pl, src/tools/copyright.pl, src/tools/mark_pgdllimport.pl) uses #!/usr/bin/perl. Making pgindent the sole file using #!/usr/bin/env perl is a gratuitous, non-minimal change that adds no functionality for this patch and will draw rejection on pgsql-hackers. Revert this hunk to keep the diff minimal and consistent.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
2331fe2 to
179431c
Compare
a5fff94 to
4d8b013
Compare
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (and the sibling .envrc, .gdbinit, pg-aliases.sh, flake.nix, shell.nix) is developer-local scaffolding unrelated to the HOT indexed-updates feature. The root .gitignore explicitly states: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing these violates the minimal-diff discipline and bundles unrelated concerns into the feature patch; this will be an immediate rejection on pgsql-hackers. Move them to a personal dotfiles/local-ignore, not the patch.
| - -DPGDLLIMPORT= | ||
| - -DPIC |
There was a problem hiding this comment.
-DPGDLLIMPORT= blanks out the Windows export annotation in the editor. Reviewers rely on seeing PGDLLIMPORT to verify that extern variables crossing module/extension boundaries are correctly exported for MSVC; masking it defeats exactly the check this project needs. Combined with -Wconversion/-Wfloat-equal/-Wshadow (not in PostgreSQL's real flag set), this surfaces a flood of false-positive diagnostics that do not reflect the buildfarm.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These line-number breakpoints target files that the feature itself modifies (heapam.c, pruneheap.c, indexam.c, execIndexing.c). Line numbers drift the instant those files change, so this file is stale on arrival. Worse, it references symbols that do not exist anywhere in the tree (verified: heap_hot_indexed_create_tuple, heap_hot_search_buffer, heap_xlog_indexed_update have no definitions) and a source file heapam_indexscan.c plus WAL record XLOG_HEAP2_INDEXED_UPDATE that are not in this review group. Hardcoded line/symbol breakpoints do not belong in a committed patch.
| set $slot = (TupleTableSlot*)$arg0 | ||
| if $slot | ||
| printf "TupleTableSlot: %s\n", $slot->tts_ops->name | ||
| printf " empty: %d, shouldFree: %d\n", $slot->tts_empty, $slot->tts_shouldFree |
There was a problem hiding this comment.
print_slot reads $slot->tts_empty and $slot->tts_shouldFree, but TupleTableSlot has no such fields on this tree -- these are flag bits in tts_flags (TTS_FLAG_EMPTY / TTS_FLAG_SHOULDFREE). This macro will error at runtime against the modern struct, misleading anyone debugging.
| if $context | ||
| printf "MemoryContext: %s\n", $context->name | ||
| printf " type: %s, parent: %p\n", $context->methods->name, $context->parent | ||
| printf " total: %zu, free: %zu\n", $context->mem_allocated, $context->freep - $context->freeptr |
There was a problem hiding this comment.
print_mcxt reads $context->freep - $context->freeptr, but MemoryContextData on this tree has no freep/freeptr fields (it tracks mem_allocated). The freep - freeptr expression is pre-modern layout and will fail at runtime. The $list->head/$cell->next traversal in print_list similarly assumes the pre-v13 List layout, which no longer matches the flat-array List. These stale helper macros reduce the tooling's value.
| 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.
rm -rf "$PG_DATA_DIR" (and the analogous trash/rm -rf "$build_dir", pg-full-clean) are destructive deletes driven entirely by env vars. If PG_DATA_DIR / PG_BUILD_DIR is unset or empty (e.g. the alias is sourced outside the nix shell that exports them), this becomes rm -rf "" or an operation on an unintended path. Guard with an explicit non-empty / expected-prefix check (e.g. [ -n "$PG_DATA_DIR" ] && case "$PG_DATA_DIR" in /tmp/*) ...;; esac) before any recursive delete.
| # Build helpers shared by every variant. | ||
| # ============================================================ | ||
| pg_clean_for_compiler() { | ||
| local current_compiler="$(basename $CC)" |
There was a problem hiding this comment.
basename $CC is unquoted and $CC may carry a full store path plus flags; $CC with spaces word-splits and basename receives multiple args. Quote it: local current_compiler; current_compiler="$(basename "$CC")". (Also note local x="$(...)" masks the subshell exit status.)
| 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.
print_relcache prints $rel->rd_rel->relnamespace (an OID) with %s, which will crash/garbage-print rather than show a namespace name, and reads $rel->rd_id which does not exist -- the field is $rel->rd_rel->oid / $rel->rd_rel->relname. This is the same class of stale-struct GDB macro already flagged for print_slot/print_mcxt/print_list and will error at runtime against the current tree.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to the KNN/amcanreturnorderby EvalPlanQual feature this patch implements, and should be dropped from the patch (minimal-diff discipline). shellprog is SHELLPROG, which is always an absolute path ($(SHELL) from the makefiles, /bin/sh from meson). Since the argument contains a slash, execlp performs no PATH search and behaves identically to execl for the upstream build, so this is a functional no-op here. It only has an effect in a local dev environment where /bin/sh is not present and a PATH lookup is wanted (consistent with the flake.nix/shell.nix/.envrc dev-setup files in this changeset) -- that belongs in the developer's local tree, not in a committed patch. Confidence: high.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change is inconsistent with the rest of the tree and is unrelated to the stated purpose of the patch (which, per the other changed files, concerns GIST/SP-GiST index scan and planner work). Every other Perl script in the PostgreSQL source tree uses the hardcoded #!/usr/bin/perl form -- including the sibling tools src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, and src/tools/git_changelog, as well as all generator scripts under src/backend and src/common/unicode. Changing only this one file to #!/usr/bin/env perl introduces an inconsistency and is unrelated churn that will draw an objection on -hackers (minimal-diff rule). This looks like a local developer convenience (likely tied to the added flake.nix/shell.nix/.envrc Nix environment, where perl lives on PATH rather than at /usr/bin/perl) that should not be committed. Revert this line.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
|
v13 prepared at 4d8b013. Rebased onto master (upstream 73a28d2), and it includes the two cleanups deferred from earlier rounds:
Validated before pushing:
CI: 9 of 10 jobs passed. macOS was cancelled again with "job was not acquired by Runner", which is GitHub's macOS capacity. The latest review pass raised nothing on the patch code. |
aa0f0c9 to
6f4c64e
Compare
Squashes the four CI commits this fork carried on top of upstream into
one, and cuts the carry from 3776 lines to ~1000.
Removed:
- .github/{README,QUICKSTART}.md and .github/docs/* (2273 lines). Beyond
being bulk we rebase hourly, they had drifted into being wrong: they
described a daily (not hourly) sync, Claude 3.5 Sonnet via the direct
Anthropic API (we run Opus on Bedrock), a static IAM user (we use
OIDC), and a Windows dependency-builder workflow that the same commit
which documented it had already deleted. 16 referenced paths did not
exist. An AI reviewer reading this repo ingests that as truth.
- sync-upstream-manual.yml (252 lines). sync-upstream.yml already has
workflow_dispatch; the sole behavioral difference was a force_push
toggle whose "false" setting cannot work after a rebase anyway.
- .github/.gitignore, which only ignored scripts/ai-review/, a directory
that does not exist here.
Rewritten:
- sync-upstream.yml -> fork-sync-upstream.yml, 260 lines to 47. git
rebase already handles the fast-forward, no-op and diverged cases that
the ahead/behind arithmetic hand-rolled, so all that goes away, as do
the issue open/comment/close machinery (a scheduled-run failure is
already emailed to the one consumer, and the run log carries what the
issue body would) and the "dev setup|dev v[0-9]" commit allowlist,
which matched zero commits on master and had since it was written.
Sync drops to every three hours: hourly meant 24 full-history clones
of an 840MB repo per day, each force-push also re-triggering pg-ci.yml
on master, to collect a handful of upstream commits.
Renamed:
- ocr-review.yml, ocr-model-check.yml -> fork-*.yml. Upstream owns
.github/ too (it has touched pg-ci.yml three times this year), so
fork-owned workflows now live under a name prefix upstream will not
collide with, making the rebase structurally conflict-free and letting
the sync guard match fork-owned paths exactly rather than allowing all
of .github/.
Unchanged: the OCR review workflows' logic and .github/ocr/* (rule.json,
context.md, litellm.yaml, pg-history.py) - the part that carries value.
The OCR review workflows and their config were the last bulk this fork carried on top of upstream: ~970 lines rebased onto postgres/postgres every few hours, editable only by amending a commit that gets force- pushed. They are now reusable (workflow_call) workflows in the sidecar repo gburd/ci-workflows, and master carries two caller stubs instead. Carry after this: 3 files, ~110 lines, one commit. The sidecar is also where the value is: ocr/rule.json and ocr/context.md (the PostgreSQL committer-grade review corpus) become normal reviewable files with normal PRs and history, rather than a blob inside a commit that is rewritten on every sync. Mechanics worth knowing: - The review jobs check ci-workflows out at github.job_workflow_sha, so the rules always come from the same commit as the logic using them, and pinning @v1 (or a sha) in the stub pins both. This replaces the old "git show origin/<default_branch>:.github/ocr/..." dance, which existed only because the config had to be read off a branch other than the PR's. - AWS settings are passed as explicit inputs rather than read from vars inside the reusable workflow. vars does resolve across the boundary, but the contract stays visible at the call site. - A reusable workflow does not change the OIDC subject: it stays repo:gburd/postgres:*, so the ocr-bedrock-ci trust policy is unchanged. - ci-workflows must stay public; callers read it with GITHUB_TOKEN. The sync guard narrows to .github/workflows/fork-* accordingly, and sync-upstream keeps needing SYNC_TOKEN: the stubs are still workflow files, which GITHUB_TOKEN may not push.
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.
box_closept_point(), behind <->(point, box), <->(box, point) and ##(point, box), found the closest point by projecting the point onto each edge's line with the generic line-segment code. That code compares slopes and intersections with the EPSILON fuzz, so for a point near an edge's line it can pick the wrong edge or corner. The point (0.9999996, 2) is 1 from the box ((0,0),(1,1)), but the operator said 1.00000000000008, and (1e-300, -1e-300) is 1e-300 from ((2,2),(0,0)), but the operator said 1.41421356237e-300, the distance to the corner. The GiST and SP-GiST box opclasses compute the distance to a box their own way, exactly, and order a nearest-neighbor scan by that. So an index scan could return rows in an order the operator disagrees with: for the point above, a box 1.00000000000004 away came after the box the operator put at 1.00000000000008. A box's edges are parallel to the axes, so the closest point is the point clamped to the box, and the distance follows from that with no line arithmetic. Do that, and have GiST's and SP-GiST's distance to a box (computeDistance() for non-leaf entries and point_box_distance()) call the same function, so the index orders by the operator's own distance. SP-GiST's version now errors on overflow, as GiST's and the operator always did, and returns NaN when any coordinate is NaN. The geometry regression test changes in four rows, all corrections: the 1e-300 distance above, its closest point, and the closest points to (1e+300,Infinity) and (Infinity,1e+300), which are now the box's top right corner rather than its top left. This is independent of the rest of the series, which uses it to let the box opclasses return their distances. Author: Greg Burd <greg@burd.me> Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
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). Both answer yes for box_ops too, whose distances the previous commit made the operator's own. 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", the number of ORDER BY keys the targetlist reads this way. 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. The documentation describes the line, in the EXPLAIN chapter and with nearest-neighbor searches. 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
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. * add_path_precheck() and add_partial_path_precheck() can't see the values, so they would reject a join path over such an input that add_path() keeps. New add_path_precheck_extras() and add_partial_path_precheck_extras() take the values the path will emit, and let an existing path dominate it only if that path emits them all as well. The old functions keep their signatures. 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. The extras-aware prechecks reject most of the others before they are built. Still, 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
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
Out-of-scope developer artifact. This .gdbinit is personal debugging scaffold for a HOT indexed-updates patch series and does not belong in the PostgreSQL tree. The root .gitignore explicitly documents the intended workflow: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing per-developer tooling is a top rejection reason on pgsql-hackers. This file (and .clangd, .envrc, flake.nix, shell.nix, pg-aliases.sh) should be ignored locally, not tracked.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These line-number breakpoints are already wrong against this tree. heapam.c:3210 is the closing brace of heap_delete(), not the heap_update entry; heapam.c:4019 is a RelationGetBufferForTuple() argument, not the "pure HOT" decision. Line-number breakpoints are inherently fragile — any edit silently points them at unrelated code, making the aid actively misleading. Besides being out of scope, these must not be committed.
| # ========================================================================= | ||
|
|
||
| # 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 (heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, etc.). They belong to a HOT indexed-updates patch that is not present here, so GDB will fail to set the breakpoints. This confirms the file is stale, unverifiable scaffold that should not be in the repo.
| # ========================================================================= | ||
|
|
||
| # WAL replay for XLOG_HEAP2_INDEXED_UPDATE | ||
| break heap_xlog_indexed_update |
There was a problem hiding this comment.
heap_xlog_indexed_update (and the XLOG_HEAP2_INDEXED_UPDATE macro named in the comment above) do not exist anywhere in the tree. The WAL-replay breakpoint cannot resolve, reinforcing that this .gdbinit targets code that is not part of this change.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
Unrelated, inconsistent churn to a committed tree tool. Every other Perl script in the tree uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, mark_pgdllimport.pl, and ~25 others). Switching only pgindent to #!/usr/bin/env perl diverges from the established convention and has nothing to do with the HOT indexed-updates feature. If the tree's shebang policy is to be changed, that is a separate patch applied consistently across all scripts — not a one-file drive-by in a feature patch.
| Add: | ||
| - -DDEBUG | ||
| - -DLOCAL | ||
| - -DPGDLLIMPORT= |
There was a problem hiding this comment.
Out-of-scope developer artifact. This .clangd is editor/LSP configuration unrelated to the HOT indexed-updates feature and should not be tracked in the PostgreSQL tree (see root .gitignore: local-workflow files belong in $GIT_DIR/info/exclude or ~/.gitexclude). Separately, -DPGDLLIMPORT= silently blanks the macro for the IDE; since PGDLLIMPORT is the Windows/MSVC export gate, this diverges from real build semantics and would mask IDE diagnostics about missing export markers on any new exported externs the series introduces.
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
Out-of-scope developer artifact. This .envrc is a personal direnv configuration (references ~/ws/postgres, custom use project_steering/use git_policy/use project_mcp helpers that only exist in the author's setup) and is unrelated to the feature. It must not be committed to the tree; ignore it locally instead. Note the trailing project_mcp postgresq also looks truncated/typo'd.
|
|
||
| inputs = { | ||
| nixpkgs.url = "github:NixOS/nixpkgs/nixos-25.05"; | ||
| nixpkgs-unstable.url = "github:nixos/nixpkgs/nixpkgs-unstable"; |
There was a problem hiding this comment.
Out-of-scope developer artifact. This Nix dev-environment flake is personal tooling unrelated to the HOT indexed-updates feature and does not belong in the PostgreSQL tree. On reproducibility: tracking nixpkgs-unstable (a moving channel) alongside stable nixpkgs 25.05 is the dual-glibc hazard the comments themselves call out for libumem; fine for a local dev shell, but another reason this belongs outside the repo rather than committed.
| { | ||
| pkgs, | ||
| pkgs-unstable, | ||
| system, | ||
| libumemPkg, | ||
| }: let |
There was a problem hiding this comment.
Out-of-scope developer artifact. This 705-line Nix dev-shell definition is personal build/debug tooling unrelated to the feature and should not be tracked in the tree. Note it hardcodes host-specific paths (e.g. /tmp/test-db-..., ~/.ccache, ~/.gdb_history_postgres) and wires in .local-gitignore via git config core.excludesFile, confirming these tooling files are meant to be developer-local, not committed.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to the KNN/EvalPlanQual feature this PR implements and should be dropped from the patch. SHELLPROG is defined as an absolute path ($(SHELL) in GNUmakefile / /bin/sh in meson.build), so execl resolves it directly. Switching to execlp makes shell resolution depend on PATH, which is a behavior change that reduces determinism of the test harness and is a footgun. This looks like a leftover from the author's local Nix dev environment (the .envrc/flake.nix/shell.nix files added in the same push, where /bin/sh lives under /nix/store/...). Unrelated hunks like this are a top rejection reason on -hackers; revert it to keep the diff minimal. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
|
Rebased onto master (upstream f8cabd1, 30 commits newer). Tip is e748c83. The rebase was clean and v13's patches are unchanged. Validated before pushing:
CI: 9 of 10 jobs passed. macOS was cancelled again with "job was not acquired by Runner", which is GitHub's macOS capacity. The review pass raised nothing on the patch code. |
551e9eb to
9507402
Compare
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.