Repository navigation
Conversation
e777a6e to
650f621
Compare
16e0007 to
331cd76
Compare
9558f42 to
05c4e60
Compare
ae8af13 to
9f584af
Compare
b142c27 to
94e88c7
Compare
0d87b2f to
d4f607c
Compare
571be3b to
03facc1
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 OCR found 27 issue(s).
- 25 inline, 2 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 src/tools/pgindent/pgindent (L1-L1)
This shebang change is unrelated to the feature and breaks the tree-wide convention: every other Perl script under src/ uses #!/usr/bin/perl (e.g. genbki.pl, gen_node_support.pl, mark_pgdllimport.pl, copyright.pl); there are zero #!/usr/bin/env perl shebangs in the tree. This is a local developer-environment convenience (consistent with the Nix files added in this PR) that will be rejected on -hackers as unrelated churn. Revert to keep the diff minimal.
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
📄 src/backend/commands/copyto.c (L881-L883)
Minor: this path does two separate syscache lookups per child -- get_rel_relkind(child) at the top of the loop and now get_rel_persistence(child) here, plus get_rel_name(child) again in each error branch. Since the relcache entry is being looked up repeatedly for the same OID, consider fetching the tuple once (e.g. via SearchSysCache1(RELOID)) and reading relkind/relpersistence/relname from it, consistent with how hot loops over inheritors avoid redundant lsyscache calls. Low severity / maintainability only.
| jobs: | ||
| review: |
There was a problem hiding this comment.
This caller invokes a reusable workflow that authenticates to AWS Bedrock via GitHub OIDC (per the header comment: "Auth is GitHub OIDC"). OIDC token minting requires id-token: write, which is never granted by default and cannot be requested inside the reusable workflow -- it must be declared by the caller. Without a permissions: block granting id-token: write, the aws-actions/configure-aws-credentials call in the reusable workflow will fail with "Credentials could not be loaded" / missing OIDC token. The review workflow also needs pull-requests: write (and likely issues: write) to post review comments. Add an explicit least-privilege permissions: block at the workflow level.
| jobs: | |
| review: | |
| permissions: | |
| contents: read | |
| id-token: write | |
| pull-requests: write | |
| issues: write | |
| jobs: | |
| review: |
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 |
There was a problem hiding this comment.
The external reusable workflow is pinned to the mutable tag @v1. A tag can be force-moved in the gburd/ci-workflows repo, so a compromise of that repo (or a maintainer) would let attacker-controlled logic run with this fork's AWS OIDC role and GITHUB_TOKEN on every PR. Pin third-party/external reusable workflows to a full commit SHA (optionally with a # v1.x trailing comment) rather than a tag.
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 | |
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@<full-commit-sha> # v1 |
|
|
||
| on: | ||
| pull_request: | ||
| # Note: no draft filter — drafts are reviewed too. |
There was a problem hiding this comment.
Non-ASCII em-dash (U+2014) in this comment. PostgreSQL requires ASCII-only in source and diffs (no em-dashes, smart quotes, or ellipses). Replace with a plain hyphen or --.
| # Note: no draft filter — drafts are reviewed too. | |
| # Note: no draft filter -- drafts are reviewed too. |
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
This caller lacks a permissions: block. The reusable workflow authenticates to Bedrock via OIDC (needs id-token: write, never granted by default and only requestable from the caller) and, per the header comment, "opens/updates one issue here" (needs issues: write). Without these, OIDC auth and/or issue creation will fail. Add an explicit least-privilege permissions: block.
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| permissions: | |
| contents: read | |
| id-token: write | |
| issues: write | |
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
|
|
||
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
External reusable workflow pinned to the mutable tag @v1. Pin to a full commit SHA so a force-moved tag in gburd/ci-workflows cannot silently change the code that runs with this repo's OIDC/token access.
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@<full-commit-sha> # v1 |
| ereport(ERROR, | ||
| errcode(ERRCODE_INSUFFICIENT_RESOURCES), | ||
| errmsg("could not find an unpinned shared buffer")); |
There was a problem hiding this comment.
This error-message rewording (and the matching one in localbuf.c) is unrelated to the modified_idx_attrs / HOT table-AM refactor that this change set is about. Mixing an independent buffer-exhaustion message/errcode change into this patch violates the "minimal diff, one logical change per commit" discipline and will draw a split-it request on -hackers. Recommend moving these two hunks to a separate patch.
Confidence: high.
| ereport(ERROR, | ||
| (errcode(ERRCODE_INSUFFICIENT_RESOURCES), | ||
| errmsg("no empty local buffer available"))); | ||
| errmsg("could not find an unpinned local buffer"))); |
There was a problem hiding this comment.
This message rewording is unrelated to the table-AM/HOT refactor that is the purpose of this change set. Like the freelist.c hunk, it should be split into a separate, self-contained patch rather than bundled here (patch hygiene: one logical change per commit).
Confidence: high.
| /* Unlogged partitions are inaccessible during recovery. */ | ||
| if (RecoveryInProgress() && | ||
| get_rel_persistence(child) == RELPERSISTENCE_UNLOGGED) |
There was a problem hiding this comment.
This COPY-TO-during-recovery guard for unlogged (partitioned) tables is unrelated to the HOT-update / modified_idx_attrs work in the other file of this change (repack.c). Bundling two independent concerns in one patch is a common pgsql-hackers rejection reason; split it into its own commit/patch with its own rationale and thread reference. (moderate confidence)
| ereport(ERROR, | ||
| errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | ||
| errmsg("cannot access temporary or unlogged relations during recovery"), | ||
| errdetail("Partition \"%s\" is an unlogged table in partitioned table \"%s\".", | ||
| relation_name, RelationGetRelationName(rel))); |
There was a problem hiding this comment.
This check runs inside the foreach_oid loop before the "Exclude tables with no data" step, so it also fires on intermediate partitioned tables (RELKIND_HAS_PARTITIONS), which are themselves persistence UNLOGGED-capable but hold no data. For a permanent root with an unlogged intermediate partitioned level, this errors with errdetail "...is an unlogged table..." even though that relation stores no rows. Consider only checking leaf relations (e.g. test relkind / move the check after the no-data exclusion) so the detail message isn't misleading. (low confidence)
|
|
||
| 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 HOT-update / SupportRequestSubattr feature this PR implements, and it changes behavior in a risky way. shellprog comes from SHELLPROG, defined as $(SHELL) (Make) or /bin/sh (meson) -- normally an absolute path. execl() runs exactly that file; execlp() performs a $PATH search when the argument has no slash. The replacement only alters behavior if shellprog has no slash, in which case the shell is resolved from the environment's PATH -- a PATH-injection footgun that did not exist before. This looks like a local Nix/direnv convenience (matching the flake.nix/shell.nix/.envrc files added elsewhere in this PR). Revert it to keep the diff minimal and avoid the behavior change.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
- Rebased on upstream 5ffb8c7. - 0006: the narrowing no longer compares a column an index stores again: it is changed exactly when it is still in the changed set (IndexDatumsDiffer() takes the set), so its values are not fetched a second time. Only expression datums are compared again. - hot_updates: a write-back through an expression that returns the value as is, evaluated for a predicate column's change (HOT after 0006).
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (along with .envrc, .gdbinit, .local-gitignore, .mcp.json, flake.nix, shell.nix, and pg-aliases.sh) is personal developer tooling and does not belong in a PostgreSQL contribution destined for pgsql-hackers/commitfest. It violates the "minimal diff" discipline: a patch must not bundle unrelated scaffolding at the repo root. The repo already uses per-directory .gitignore files, and this change even ships its own .local-gitignore mechanism (git config core.excludesFile .local-gitignore) -- use that to keep these files local and untracked rather than committing them. Including them will draw immediate rejection on the list.
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These file+line breakpoints are already stale against the code in this same change. For example break heapam.c:3210 is annotated "Main entry: heap_update", but heap_update()'s definition is at ~line 3278 in the modified file; the "HOT decision block" at 4019/4024/4033 no longer matches -- the HOT decision is now if (hot_allowed) at ~line 4174. Likewise break execIndexing.c:370 is annotated "Index skip decision" but line 370 is return true; inside the predicate-qual helper, not any skip decision. Line-number breakpoints are inherently fragile: they silently point at the wrong code as the patch evolves and will mis-direct any debugging session. If this file were to be kept at all (it should not be committed -- see .clangd), breakpoints should be keyed to function names, not line numbers.
| @@ -0,0 +1,15 @@ | |||
| { | |||
| "mcpServers": { | |||
| "postgresq": { | |||
There was a problem hiding this comment.
The server key "postgresq" is missing a trailing 'l' and looks like a typo for "postgresql". The same truncated name is referenced from .envrc via use project_mcp postgresq, so the two are at least internally consistent, but the identifier itself is almost certainly wrong. If intentional, it is highly confusing; if a typo, it names the MCP server incorrectly.
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| use project_steering c aws | ||
| use project_steering postgresql | ||
| use project_mcp postgresq | ||
| use project_mcp context7 |
There was a problem hiding this comment.
source_up relies on a parent-directory .envrc (the comment points at ~/ws/postgres/.envrc), and the custom use project_steering / use project_mcp directives depend on direnv functions and MCP servers that exist only on one developer's machine. Combined with the hard-coded host paths in shell.nix/pg-aliases.sh (e.g. /tmp/test-db-$(basename $PWD), sudo tee /proc/sys/kernel/core_pattern), this ties the committed tree to a single developer's layout and is not portable. This file should not be committed -- keep it local via the .local-gitignore mechanism.
| (github.event_name == 'issue_comment' && github.event.issue.pull_request && | ||
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) |
There was a problem hiding this comment.
The issue_comment trigger fires for any comment starting with /open-code-review, @open-code-review, or /pg-history, but there is no gate on the comment author. Any user who can comment on a PR (not only maintainers) can start an AI review and drive Bedrock spend. Add an author_association gate on the if: condition, e.g. require the commenter to be OWNER/MEMBER/COLLABORATOR:
(github.event_name == 'issue_comment' && github.event.issue.pull_request &&
contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) &&
(startsWith(...)))
If gating is intended to live downstream in gburd/ci-workflows, note that the reusable workflow is not vendored in this repo, so it cannot be verified here. Confidence: high.
| /* Temporary and unlogged relations are inaccessible during recovery. */ | ||
| if (!RelationIsPermanent(rel) && RecoveryInProgress()) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | ||
| errmsg("cannot access temporary or unlogged relations during recovery"))); |
There was a problem hiding this comment.
This new user-visible error path (currtid/currtid2 on a standby) has no test coverage. The accompanying TAP additions in src/test/recovery/t/001_stream_rep.pl only exercise the COPY TO guard in copyto.c, not this one. Per project policy a user-visible behavior change should include a test covering the error path, e.g. a currtid2(...) call against an unlogged/temp relation on $node_standby_1 asserting the same qr/cannot access temporary or unlogged relations during recovery/ message. Confidence: moderate.
|
|
||
| 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 SUBATTR support-function feature that the rest of this patch implements; it is unrelated churn and should be split out (or dropped).
Functionally it is also a footgun. shellprog comes from SHELLPROG, which is defined as $(SHELL) in src/test/regress/GNUmakefile and hardcoded /bin/sh in meson.build -- i.e. normally an absolute path. For an absolute path execlp is identical to execl, so this buys nothing. The only case it changes behavior is when $(SHELL) is a bare command name with no slash (e.g. make SHELL=sh), and then execlp resolves the shell through the ambient $PATH instead of failing. That is a non-deterministic, PATH-injection hazard in the test harness for no benefit. Revert to execl.
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 rest of the patch and breaks the tree-wide convention. Every other Perl script in the PostgreSQL source (e.g. genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 others) uses the fixed #!/usr/bin/perl shebang. Switching only pgindent to #!/usr/bin/env perl makes it the sole outlier and is gratuitous churn that will draw a rejection on -hackers. If a case is to be made for /usr/bin/env perl, it should be a separate, deliberate, tree-wide patch discussed on its own; it does not belong bundled into this change. Revert this line.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| if (!RelationIsPermanent(rel) && RecoveryInProgress()) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | ||
| errmsg("cannot access temporary or unlogged relations during recovery"))); |
There was a problem hiding this comment.
Style nit (low confidence): this new ereport uses the legacy extra-parentheses wrapping ereport(ERROR, (errcode(...), errmsg(...))), but every other ereport in this same function (the RELKIND_HAS_STORAGE check just above, and the two in currtid_for_view) uses the modern parenthesis-free form. For a minimal patch that reads as if always written that way, match the surrounding style.
| if (!RelationIsPermanent(rel) && RecoveryInProgress()) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | |
| errmsg("cannot access temporary or unlogged relations during recovery"))); | |
| if (!RelationIsPermanent(rel) && RecoveryInProgress()) | |
| ereport(ERROR, | |
| errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | |
| errmsg("cannot access temporary or unlogged relations during recovery")); |
| UPDATE subattr_two SET d = jsonb_set(d, '{c}', '0'); | ||
| -- both rows HOT: the index's value didn't change | ||
| SELECT pg_stat_get_xact_tuples_hot_updated('subattr_two'::regclass) AS hot; |
There was a problem hiding this comment.
This test's HOT-count assertions do not actually exercise the SupportRequestSubattr path in a way that a regression could be caught in normal (non-assert) builds. IndexUnaffected() in nodeModifyTable.c, when the subattr proof does not apply, falls back to IndexEntryChanges(), which evaluates the expression index and compares its datums byte-for-byte. For both UPDATEs here the fallback reaches the same HOT classification as the proof path:
jsonb_set(d,'{c}','0'):two_keys(d,'a','b')is unchanged, so the fallback also treats the index as unaffected -> HOT.jsonb_set(d,'{b}','0'): the value changes, so both paths -> not HOT.
So pg_stat_get_xact_tuples_hot_updated() returns 2 / 2 with the support function reverted too; the EXPLAIN/index-scan check for '2/0' likewise passes via the fallback. Per the test-discipline rule (a test that still passes with the feature reverted is worthless), the happy-path assertions here do not catch a regression in the support-function path on a non-assert buildfarm animal. The comment itself concedes the real verification only happens under USE_ASSERT_CHECKING / amcheck. Consider either dropping the redundant HOT-count assertions or adding coverage that is observably different only when the support function is consulted. (moderate confidence)
…ap_update compares slots - 0008: hashcomparedatums() says "different" without hashing when the type's default btree opclass has equalimage true for the index's collation; the executor only asks about values that differ once detoasted. Exclusion-constraint collision tests use interval spans of 1 and 2^32 microseconds (int8 is no longer asked). - 0002: simple_heap_update() puts the new tuple in a slot and uses table_slot_modified_idx_attrs(); HeapUpdateModifiedIdxAttrs() is now assert-only (heap_update()'s cross-check). - Rebased on upstream.
There was a problem hiding this comment.
🔍 OCR found 26 issue(s).
- 25 inline, 1 in summary (inline capped at 25)
📄 src/tools/pgindent/pgindent (L1-L1)
This change breaks the tree-wide convention: every other Perl script in the PostgreSQL source tree (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 others) uses #!/usr/bin/perl. After this edit, pgindent would be the sole outlier using #!/usr/bin/env perl.
The change has no stated functional purpose and is unrelated to any other work in this patch set, so it is unnecessary churn that will draw a rejection on -hackers. If a env-based shebang were genuinely desired, it would need to be a deliberate, tree-wide decision discussed on the list, not a one-off edit to a single file. Revert this to #!/usr/bin/perl to keep the diff minimal and consistent.
💡 Suggested change
Before:
+#!/usr/bin/env perl
After:
+#!/usr/bin/perl
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
This file (and the whole .clangd/.envrc/.gdbinit/.local-gitignore/.mcp.json/flake.nix/shell.nix/pg-aliases.sh group) is developer-local environment scaffolding with no relationship to the stated code change. It is unrelated churn that violates PostgreSQL patch hygiene (minimal diff) and would be rejected on pgsql-hackers; it should not be committed to the repository. Beyond that, the hardcoded source line numbers here are already wrong: heapam.c:3210 lands inside heap_delete's cleanup (not heap_update), and heapam.c:4019/:4024/:4033 land inside heap_lock_tuple's visibility-map/WAL code, not the described HOT decision block. Line-number breakpoints will keep silently drifting, making this actively misleading.
| break heap_hot_indexed_tuple_size | ||
|
|
||
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These break targets reference functions that do not exist in the source tree (e.g. heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_read_bitmap). The only occurrence of these symbols in the repo is this .gdbinit file itself. The breakpoints will never resolve, confirming this script is stale, personal tooling that does not belong in the patch.
| @@ -0,0 +1,15 @@ | |||
| { | |||
| "mcpServers": { | |||
| "postgresq": { | |||
There was a problem hiding this comment.
The JSON key "postgresq" is misspelled (missing trailing l); it should be "postgresql". The same truncated name appears in .envrc (use project_mcp postgresq), so the two are self-consistent but the name is almost certainly unintended.
| "postgresq": { | |
| "postgresql": { |
| # 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 relies on local-machine assumptions that are not reproducible for anyone else: source_up reads a parent ~/ws/postgres/.envrc that is not part of the repo, and use project_steering / use project_mcp are custom direnv functions defined nowhere in this change. Running direnv allow on a fresh checkout will error. This is personal tooling, not shareable repo config, and should not be committed.
| use flake | ||
| use project_steering c aws | ||
| use project_steering postgresql | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
project_mcp postgresq carries the same truncated name as the .mcp.json key; if it is meant to reference the MCP server defined there, both should read postgresql.
| while (isspace((unsigned char) *p)) | ||
| p++; | ||
| if (*p == '-' || *p == '+') | ||
| p++; |
There was a problem hiding this comment.
key_may_be_position() uses isspace() from <ctype.h> with the current locale. The leading comment claims answering "yes too often" is the safe direction (an over-reported overlap only costs an unnecessary index entry), which is true for soundness. But locale-dependent isspace() is a portability smell in the backend: on some locales/platforms it may classify bytes >0x7F as space, and the behavior differs from strtol()'s skip set in subtle ways across platforms. Prefer scanner_isspace()/the backend's locale-independent helpers to keep the heuristic deterministic across the supported platforms. (low confidence that this causes a bug, but it is the kind of locale dependence the tree avoids in parse/scan paths.)
| WITH current_tuple AS (SELECT ctid FROM hot_test WHERE id = 1) | ||
| SELECT has_hot_chain('hot_test', current_tuple.ctid) AS has_chain, | ||
| chain_position, print_hot_chain.ctid, lp_flags, t_ctid | ||
| FROM current_tuple, LATERAL print_hot_chain('hot_test', current_tuple.ctid); |
There was a problem hiding this comment.
The expected output hardcodes concrete ctids ((0,1), (0,4)) that depend on exact tuple placement within the page. With fillfactor=50 and these fixed small tuples this should be deterministic, but it couples the test to heap layout details (tuple alignment/padding). If future changes to tuple header size or alignment shift placement, the test breaks in a way unrelated to HOT behavior. Consider asserting the structural property (chain length / has_hot_chain result) rather than literal ctids where possible. (low confidence; acceptable as-is but worth noting.)
| /* We fill in schema names below, so work on a copy */ | ||
| schemaElts = copyObject(schemaElts); |
There was a problem hiding this comment.
This new copyObject deep-copies the entire schemaElts list, but the function's header comment (lines 4150-4152) still reads "we copy any CREATE TABLE subcommands that we might modify" and transformCreateSchemaCreateTable still does a redundant flat-copy of each CreateStmt to "not damage the input data structure." The header comment is now stale/misleading: the whole list is copied up front, not just CREATE TABLE subcommands. Please update the header comment to reflect that the entire list is copied so a reader isn't misled. (moderate confidence)
| * analyzes the elements of a CREATE SCHEMA statement | ||
| * | ||
| * This presently has two responsibilities. We verify that no subcommands | ||
| * This presently has two responsibilities. We ensure that no subcommands |
There was a problem hiding this comment.
This reword ("verify" -> "ensure") is unneeded churn and is slightly less precise: this first responsibility is still purely a verification (it raises an error on a schema-name mismatch); the new fill-in behavior falls under the schema-name handling described below. Per the minimal-diff discipline, prefer leaving the untouched wording as-is unless the change is required. (low confidence)
| * This presently has two responsibilities. We ensure that no subcommands | |
| * This presently has two responsibilities. We verify that no subcommands |
|
|
||
| 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 SupportRequestSubattr feature this patch adds and looks like scope creep. SHELLPROG is $(SHELL) from the Makefile and "/bin/sh" under meson; both are absolute paths, for which execlp behaves identically to execl (PATH is only searched when the name has no slash). So this is a behavioral no-op in the normal case and a silent PATH-search footgun in the unusual case where SHELLPROG is a bare name. Either drop this hunk from the feature patch (minimal-diff discipline), or split it into its own commit with a justification for why the test shell should be resolved via PATH. Confidence: high.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| on: | ||
| pull_request: |
There was a problem hiding this comment.
No top-level permissions block is declared. A caller workflow that invokes a reusable workflow via uses: can declare top-level permissions, and those are passed down to constrain the reusable workflow's GITHUB_TOKEN. Without it, this job inherits the repo/org default token scopes (often broad write), while it is triggered by pull_request and issue_comment on PR content. Declare least-privilege here (the sibling pg-ci.yml sets permissions: contents: read). The AI review likely needs pull-requests: write to post comments; grant exactly that and nothing more.
Note: permissions and timeout-minutes cannot be set on a reusable-workflow uses: job itself, so the per-job timeout to bound Bedrock spend must be enforced inside gburd/ci-workflows/.../ocr-review.yml (not verifiable from this repo). (high confidence on the missing top-level permissions)
| on: | |
| pull_request: | |
| permissions: | |
| contents: read | |
| pull-requests: write | |
| on: | |
| pull_request: |
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 |
There was a problem hiding this comment.
The cross-repo reusable workflow is pinned to the mutable major tag @v1. Tags can be moved or hijacked; if the upstream repo or tag is compromised, this workflow runs attacker-controlled logic with whatever token scopes it gets, on every PR. For a third-party reusable workflow, pin to a full commit SHA (optionally with # v1 trailing comment) and bump deliberately. (high confidence)
|
|
||
| on: | ||
| pull_request: | ||
| # Note: no draft filter — drafts are reviewed too. |
There was a problem hiding this comment.
Non-ASCII em-dash in this comment. The PostgreSQL source standard is ASCII-only in source and diffs (no smart quotes, em-dashes, or ellipses). Replace the em-dash with a regular hyphen/dashes. (high confidence)
| # Note: no draft filter — drafts are reviewed too. | |
| # Note: no draft filter -- drafts are reviewed too. |
|
|
||
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
Same mutable-tag concern as the review workflow: the cross-repo reusable workflow is pinned to @v1. Pin to a full commit SHA to prevent a moved/hijacked tag from silently changing the executed logic. Also note no top-level permissions is declared; this notify-only job opens/updates an issue, so it needs issues: write and nothing else -- declare it explicitly rather than inheriting broad defaults. (high confidence)
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
This job runs unattended on a 3-hourly timer, performs git rebase against upstream, and then git push --force-with-lease. If the rebase hits a conflict, git rebase exits non-zero and (under set -euo pipefail) the step fails before the push -- good. But the repo is left in a half-rebased state only on the ephemeral runner, so that is harmless. The real reliability gap is the missing timeout-minutes: a stuck git fetch/rebase (e.g. network stall) can hang the runner indefinitely. Add a job-level timeout-minutes bound. (moderate confidence)
| sync: | |
| runs-on: ubuntu-latest | |
| steps: | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| steps: |
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (together with .envrc, .gdbinit, .local-gitignore, .mcp.json, flake.nix, shell.nix, pg-aliases.sh and the .github/workflows/fork-* files) is personal developer/CI scaffolding. The repo's own root .gitignore states the project policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Tracking these in the tree violates the minimal-diff discipline and would be an immediate rejection on pgsql-hackers. Keep them out of the patch via a personal/local ignore instead of committing them. [high confidence]
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
These hardcoded source-line breakpoints are already stale against the current tree. heapam.c:3210 is the tail of heap_delete() (heap_freetuple(old_key_tuple)), not "Main entry: heap_update"; heapam.c:4019/4024/4033 land in a heap_lock_tuple WAL block, not the claimed "HOT decision" / "augmented tuple size check". Line-number breakpoints drift silently with every edit and will resolve to the wrong code. [high confidence]
| break heap_hot_indexed_tuple_size | ||
|
|
||
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These function-name breakpoints reference symbols that do not exist in the tree (code_search for heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, etc. returns no matches), and the cited files heapam_indexscan.c / indexam.c paths plus their line numbers come from an unmerged HOT-indexed-updates patch series. As committed this .gdbinit targets code that isn't present, so most of these breakpoints will fail to resolve. [high confidence]
| 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.
These recursive deletes are guarded only by environment variables that may be empty. If pg-aliases.sh is sourced outside the Nix shell that exports PG_DATA_DIR/PG_BUILD_DIR/PG_INSTALL_DIR, the expansions collapse to e.g. rm -rf "" or operate on an unintended cwd-relative path, risking data loss. Add a non-empty guard (e.g. [ -n "$PG_DATA_DIR" ] || return 1) before any trash/rm -rf. [medium confidence]
|
|
||
| # 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.
Same unguarded-variable hazard: if PG_BUILD_DIR/PG_INSTALL_DIR are unset, this becomes rm -rf "" "". Guard that both variables are non-empty before the recursive delete. [medium confidence]
| patches = | ||
| (oldAttrs.patches or []) | ||
| ++ [ | ||
| ./glibc-no-fortify-warning.patch |
There was a problem hiding this comment.
patchedGlibc references ./glibc-no-fortify-warning.patch, but that patch file is not present at the repo root (file_find confirms it is missing). flake evaluation of the dev shell (nix develop) will fail with a missing-path error. Either add the patch file to the repo or drop the override. [high confidence]
| config.allowUnfree = true; | ||
| }; | ||
|
|
||
| libumemPkg = libumem.packages.${system}.libumem; |
There was a problem hiding this comment.
This pins libumem to a codeberg tag via git+https, which is only reproducible if the resolved flake.lock is committed. No flake.lock is present at the repo root (file_find /flake.lock returns not found), so the tag ref and nixpkgs revisions are unpinned and the environment can silently change if the tag is re-pointed. Commit the generated flake.lock alongside flake.nix. [high confidence]
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
The sync job has no timeout-minutes. It runs unattended on a 3-hourly timer and performs network operations (git fetch from upstream, git push). If any step hangs (e.g. network stall), the job can run until the runner's 6-hour hard limit, needlessly consuming Actions minutes. Add a job-level timeout-minutes (a few minutes is ample for a rebase/push).
| sync: | |
| runs-on: ubuntu-latest | |
| steps: | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| steps: |
| TM_FailureData *tmfd, LockTupleMode lockmode, | ||
| const Bitmapset *modified_idx_attrs, bool hot_allowed); |
There was a problem hiding this comment.
The exported heap_update() signature changes (LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes -> LockTupleMode lockmode, const Bitmapset *modified_idx_attrs, bool hot_allowed), and the tuple_update table-AM callback in tableam.h gains a modified_idx_attrs parameter. Both are API/ABI breaks: any out-of-tree extension calling heap_update() directly, and any third-party table AM implementing tuple_update, will fail to compile against this header and is ABI-incompatible. This is fine for a HEAD-targeted feature but must not be backpatched, and the AM-API change should be called out in the commit message / AM docs so TAM authors update their callbacks and recompute HOT/lock decisions from the new modified_idx_attrs.
| /* We fill in schema names below, so work on a copy */ | ||
| schemaElts = copyObject(schemaElts); |
There was a problem hiding this comment.
This header comment is now stale. With the new upfront copyObject(schemaElts), the whole element list is privately copied, so the claim "we copy any CREATE TABLE subcommands that we might modify" no longer describes the strategy (and the per-CreateStmt flat-copy in transformCreateSchemaCreateTable is now redundant). Please update this note to reflect that the entire input list is copied up front so subcommand schema names can be filled in without modifying the caller's parse tree.
| * This presently has two responsibilities. We ensure that no subcommands | ||
| * are trying to create objects outside the new schema. We also attempt to |
There was a problem hiding this comment.
This reword from "verify" to "ensure" slightly reduces accuracy for the sentence it sits in: the first responsibility described here ("no subcommands are trying to create objects outside the new schema") is still a check that errors on mismatch -- "verify" was the better verb for that clause. The actual behavioral change (now filling in the schema name when absent) is a separate responsibility not captured by this edit. Consider reverting this cosmetic reword to keep the diff minimal, or reword the paragraph to actually describe the new set-or-check behavior. Low confidence / low priority.
| slot = MakeTupleTableSlot(RelationGetDescr(relation), &TTSOpsBufferHeapTuple, 0); | ||
| bslot = (BufferHeapTupleTableSlot *) slot; |
There was a problem hiding this comment.
This adds, on every simple_heap_update() call, an extra buffer read (heap_fetch does ReadBuffer + share lock + unlock; heap_update then reads the same block again), two TupleTableSlot allocations, and a full tuple deform/compare via table_slot_modified_idx_attrs(). simple_heap_update() is the hot path for every catalog tuple update (CatalogTupleUpdate/WithInfo), so this is a measurable per-update regression in DDL/catalog-heavy transactions versus the old single-pass-under-one-lock design.
At minimum, short-circuit when the relation has no indexes: in that case modified_idx_attrs is always NULL, hot_allowed is always true, and none of the fetch/slot/compare work is needed. Guard with relation->rd_rel->relhasindex (or an empty INDEX_ATTR_BITMAP_INDEXED) before doing the slot/heap_fetch dance.
| * (see the header comment), so both reads see the same version; if one | ||
| * happens anyway, heap_update() reports it. | ||
| */ | ||
| slot = MakeTupleTableSlot(RelationGetDescr(relation), &TTSOpsBufferHeapTuple, 0); |
There was a problem hiding this comment.
Style inconsistency: the old slot is built with MakeTupleTableSlot(..., 0) while the new slot a few lines below uses MakeSingleTupleTableSlot(...). They are identical (MakeSingleTupleTableSlot is just MakeTupleTableSlot(tupdesc, ops, 0)), and both are released with ExecDropSingleTupleTableSlot. Use MakeSingleTupleTableSlot for both so the create/drop pairing reads consistently.
| slot = MakeTupleTableSlot(RelationGetDescr(relation), &TTSOpsBufferHeapTuple, 0); | |
| slot = MakeSingleTupleTableSlot(RelationGetDescr(relation), &TTSOpsBufferHeapTuple); |
| ereport(ERROR, | ||
| errcode(ERRCODE_INSUFFICIENT_RESOURCES), | ||
| errmsg("could not find an unpinned shared buffer")); |
There was a problem hiding this comment.
Minor style inconsistency: the surrounding buffer code (including the matching call in localbuf.c modified in this same change) uses the parenthesized ereport form ereport(ERROR, (errcode(...), errmsg(...))). For consistency within the subsystem, consider wrapping the arguments in parentheses here too. Not a correctness issue — low confidence/low priority nit.
| ereport(ERROR, | |
| errcode(ERRCODE_INSUFFICIENT_RESOURCES), | |
| errmsg("could not find an unpinned shared buffer")); | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INSUFFICIENT_RESOURCES), | |
| errmsg("could not find an unpinned shared buffer"))); |
| * The contents of this array are mostly generated from the CLDR project's | ||
| * windowsZones.xml file. See src/timezone/README for more information. |
There was a problem hiding this comment.
This comment points to src/timezone/README, but no such file exists in the tree (there is no README under src/timezone/, nor any committed CLDR/windowsZones generator or source). A dangling doc reference like this will mislead future maintainers who try to regenerate the table. Either add the referenced README (documenting the CLDR windowsZones.xml generation process, as the comment claims) together with any generator script in the same patch, or drop/reword the reference to describe how the array is actually maintained. (high confidence)
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change makes pgindent 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 ~30 others) uses #!/usr/bin/perl. Changing only this one file to #!/usr/bin/env perl is an unrelated, non-minimal change that breaks that convention and does not fit the stated purpose of the surrounding patch.
If the goal is to support Perl installed outside /usr/bin (e.g. via a Nix/direnv environment, as the other added files in this change suggest), that is a tree-wide policy decision that must be proposed and discussed on pgsql-hackers and applied consistently to all scripts in one dedicated patch -- not slipped into a single file. As-is, this hunk should be dropped from the patch.
|
|
||
| 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.
Changing execl() to execlp() is an unexplained, unrelated change in a security-sensitive code path. With the current build definitions SHELLPROG is always an absolute path ($(SHELL) under make, hardcoded "/bin/sh" under meson), so for a name containing a slash execlp behaves identically to execl and there is no benefit. The only case it differs is a bare program name, where execlp performs a PATH search -- introducing PATH-lookup/injection surface for zero gain. Unless there is a stated reason (e.g. a planned non-absolute SHELLPROG), revert to execl to keep the diff minimal and avoid the footgun. (moderate confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
|
📊 OCR posted 22/23 inline comment(s). 1 could not be posted
|
No description provided.