-
Notifications
You must be signed in to change notification settings - Fork 1
Trying out <stdatomic.h> for port/atomics.h (opt-in alternative) [cf-7005] #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
4646ad9
3ed29c4
c918e85
eeda6aa
5d288c7
0087704
bc247c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||
| Diagnostics: | ||||||||||
| MissingIncludes: None | ||||||||||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Committing
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
This (along with
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire |
||||||||||
| InlayHints: | ||||||||||
| Enabled: true | ||||||||||
| ParameterNames: true | ||||||||||
| DeducedTypes: true | ||||||||||
| CompileFlags: | ||||||||||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json | ||||||||||
| Remove: [ -Werror ] | ||||||||||
| Add: | ||||||||||
| - -DDEBUG | ||||||||||
| - -DLOCAL | ||||||||||
| - -DPGDLLIMPORT= | ||||||||||
| - -DPIC | ||||||||||
| - -O2 | ||||||||||
| - -Wall | ||||||||||
| - -Wcast-function-type | ||||||||||
| - -Wconversion | ||||||||||
| - -Wdeclaration-after-statement | ||||||||||
| - -Wendif-labels | ||||||||||
| - -Werror=vla | ||||||||||
| - -Wextra | ||||||||||
| - -Wfloat-equal | ||||||||||
| - -Wformat-security | ||||||||||
| - -Wimplicit-fallthrough=3 | ||||||||||
| - -Wmissing-format-attribute | ||||||||||
| - -Wmissing-prototypes | ||||||||||
| - -Wno-format-truncation | ||||||||||
| - -Wno-sign-conversion | ||||||||||
| - -Wno-stringop-truncation | ||||||||||
| - -Wno-unused-const-variable | ||||||||||
| - -Wpointer-arith | ||||||||||
| - -Wshadow | ||||||||||
| - -Wshadow=compatible-local | ||||||||||
| - -fPIC | ||||||||||
| - -fexcess-precision=standard | ||||||||||
| - -fno-strict-aliasing | ||||||||||
| - -fvisibility=hidden | ||||||||||
| - -fwrapv | ||||||||||
| - -g | ||||||||||
| - -std=c11 | ||||||||||
| - -I. | ||||||||||
| - -I../../../../src/include | ||||||||||
|
Comment on lines
+42
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The include path
Suggested change
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,23 @@ | ||||||
| source_up | ||||||
| watch_file flake.nix | ||||||
| use flake | ||||||
| use project_steering postgresql | ||||||
| use project_mcp postgresq | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the environment name:
Suggested change
|
||||||
| use aws | ||||||
|
Comment on lines
+1
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Comment on lines
+1
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file must not be part of the patch. Additionally, line 5 has a typo:
Comment on lines
+1
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a personal |
||||||
|
|
||||||
| # libumem is automatically loaded via LD_PRELOAD in the shell environment | ||||||
| # All programs in this shell will use libumem instead of glibc malloc | ||||||
| # Environment variables: | ||||||
| # UMEM_DEBUG=audit,default | ||||||
| # UMEM_LOGGING=transaction,contents,fail | ||||||
|
|
||||||
| # Configure libumem with AUDIT mode (more aggressive checking) | ||||||
| #export UMEM_DEBUG="audit,contents,guards" # audit=catch more bugs, guards=detect overruns | ||||||
| #export UMEM_LOGGING="transaction,contents,fail" | ||||||
|
|
||||||
| #export MESON_EXTRA_SETUP="-Db_coverage=true" | ||||||
| #export GENINFO_OPTIONS="--ignore-errors inconsistent,gcov" | ||||||
| #export LCOV_OPTIONS="--ignore-errors inconsistent,gcov" | ||||||
|
Comment on lines
+18
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Large blocks of commented-out |
||||||
|
|
||||||
| #export CFLAGS="-Wall -Wextra -Wconversion -Wdouble-promotion -Wno-unused-parameter -Wno-unused-function -Wno-sign-conversion -fsanitize-trap --werror" | ||||||
| # -fsanitize=undefined,address,undefined,thread | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,156 @@ | ||||||||||
| # HOT Indexed Updates — GDB breakpoints for code review | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a personal debugging artifact and should not be committed. Beyond violating the minimal-diff discipline (a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ASCII-only violation: this line (and the section headers on lines 15, 38, 52, 81, 98, 106, 117, 140, 148) use a non-ASCII em-dash (U+2014). PostgreSQL source and diffs must be ASCII only. Use a plain hyphen
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII em-dash (U+2014) characters are used throughout the comment block (e.g. "HOT Indexed Updates —", "UPDATE DECISION —"). The project standard requires ASCII only in source and diffs. Replace em-dashes with an ASCII hyphen/dash.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a developer-local debugging artifact and must not be committed to a PostgreSQL patch. A top-level |
||||||||||
| # | ||||||||||
| # Usage: gdb -x .gdbinit <postgres-binary> | ||||||||||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire
Recommendation: drop this file (and the sibling dev-only files) from the patch entirely. |
||||||||||
| # Or from gdb: source .gdbinit | ||||||||||
| # | ||||||||||
| # These breakpoints cover the major code paths introduced or modified by | ||||||||||
| # the HOT indexed updates patch series. They are organized by subsystem | ||||||||||
| # to make it easy to enable/disable groups during debugging. | ||||||||||
| # | ||||||||||
| # Tip: to skip to a specific subsystem, disable all then enable selectively: | ||||||||||
| # disable breakpoints | ||||||||||
| # enable 1 2 3 # just the update-decision group | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 1. UPDATE DECISION — heap_update() HOT/HOT-indexed/non-HOT choice | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII em-dash characters (U+2014) appear in comment lines throughout the file (e.g. "HOT Indexed Updates — GDB breakpoints", "UPDATE DECISION — heap_update()"). This violates PostgreSQL's ASCII-only rule for source and diffs. Use a plain hyphen/dash instead.
Suggested change
|
||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Main entry: heap_update | ||||||||||
| break heapam.c:3210 | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded absolute line-number breakpoints (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936) are extremely brittle: any edit to those files shifts the lines and silently moves the breakpoint to unrelated code, producing misleading debug sessions. Function-name breakpoints (as used elsewhere in this same file) are the maintainable pattern. |
||||||||||
|
|
||||||||||
| # HOT decision block: pure HOT vs HOT indexed vs non-HOT | ||||||||||
| # Line 4019: pure HOT (no indexed columns changed) | ||||||||||
| # Line 4024: HOT indexed path (non-catalog, some indexed columns changed) | ||||||||||
| # Line 4031: predict augmented tuple size | ||||||||||
| # Line 4033: size+space check before creating augmented tuple | ||||||||||
| break heapam.c:4019 | ||||||||||
| break heapam.c:4024 | ||||||||||
| break heapam.c:4033 | ||||||||||
|
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breakpoints keyed to hard-coded line numbers (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936) are inherently fragile: inserting or removing any unrelated line above them silently shifts them to the wrong statement, and they cannot be validated in review. Combined with the fact that the underlying feature code is absent from the tree, these line targets are unverifiable. |
||||||||||
|
|
||||||||||
| # Set HEAP_INDEXED_UPDATED flag on new tuple before page insertion | ||||||||||
| break heapam.c:4101 | ||||||||||
|
|
||||||||||
| # Restore HEAP_INDEXED_UPDATED on old tuple (only if it previously had it) | ||||||||||
| break heapam.c:4147 | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 2. TUPLE CREATION — building the augmented tuple with embedded bitmap | ||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Predict augmented tuple size (returns 0 if t_hoff would overflow) | ||||||||||
| break heap_hot_indexed_tuple_size | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These breakpoints reference symbols and file:line locations for a "HOT indexed updates" feature that does not exist in this tree. I searched the whole codebase: |
||||||||||
|
|
||||||||||
| # Create augmented tuple with embedded modified-column bitmap | ||||||||||
| break heap_hot_indexed_create_tuple | ||||||||||
|
|
||||||||||
| # Serialize Bitmapset into raw bytes in tuple header | ||||||||||
| break heap_hot_indexed_serialize_bitmap | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 3. BITMAP UTILITIES — raw bitmap operations for chain following | ||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Compute raw bitmap byte size from natts | ||||||||||
| break heap_hot_indexed_bitmap_raw_size | ||||||||||
|
|
||||||||||
| # Check if tuple header has room for bitmap between null bitmap and data | ||||||||||
| break heap_hot_indexed_has_bitmap_space | ||||||||||
|
|
||||||||||
| # Read HOT indexed bitmap from tuple header (returns Bitmapset) | ||||||||||
| break heap_hot_indexed_read_bitmap | ||||||||||
|
|
||||||||||
| # Fast overlap check: does tuple's raw bitmap overlap with indexed_attrs? | ||||||||||
| break heap_hot_indexed_bitmap_overlaps_raw | ||||||||||
|
|
||||||||||
| # OR a tuple's raw bitmap into an accumulator buffer | ||||||||||
| break heap_hot_indexed_bitmap_or_raw | ||||||||||
|
|
||||||||||
| # Check if accumulated raw bitmap overlaps with indexed_attrs | ||||||||||
| break heap_hot_indexed_accum_overlaps | ||||||||||
|
|
||||||||||
| # Merge bitmaps from dead tuples into a target tuple on the page | ||||||||||
| break heap_hot_indexed_merge_bitmaps_raw | ||||||||||
|
|
||||||||||
| # Deserialize raw bytes back to Bitmapset | ||||||||||
| break heap_hot_indexed_deserialize_bitmap | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 4. INDEX SCAN — HOT chain following with stale-entry detection | ||||||||||
| # src/backend/access/heap/heapam_indexscan.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Main HOT chain search with indexed update awareness | ||||||||||
| break heap_hot_search_buffer | ||||||||||
|
|
||||||||||
| # Redirect-with-data: initialize bitmap accumulator from collapsed redirect | ||||||||||
| break heapam_indexscan.c:182 | ||||||||||
|
|
||||||||||
| # Accumulate bitmap from INDEXED_UPDATED tuple in chain | ||||||||||
| break heapam_indexscan.c:250 | ||||||||||
|
|
||||||||||
| # Stale entry detection: accumulated bitmap overlaps this index's attrs | ||||||||||
| break heapam_indexscan.c:297 | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 5. INDEX SCAN SETUP — indexed_attrs bitmap computation | ||||||||||
| # src/backend/access/index/indexam.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Compute indexed_attrs for HOT indexed update chain following | ||||||||||
| break indexam.c:299 | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 6. INDEX INSERTION — skip unchanged indexes for HOT indexed updates | ||||||||||
| # src/backend/executor/execIndexing.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Entry: insert/update index tuples | ||||||||||
| break ExecInsertIndexTuples | ||||||||||
|
|
||||||||||
| # Index skip decision: skip indexes whose attrs don't overlap modified set | ||||||||||
| break execIndexing.c:370 | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 7. PRUNING — chain collapsing and redirect-with-data | ||||||||||
| # src/backend/access/heap/pruneheap.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # Main prune function | ||||||||||
| break heap_page_prune_and_freeze | ||||||||||
|
|
||||||||||
| # Per-chain pruning entry | ||||||||||
| break heap_prune_chain | ||||||||||
|
|
||||||||||
| # Chain collapsing: collect bitmaps from dead INDEXED_UPDATED intermediates | ||||||||||
| break pruneheap.c:1802 | ||||||||||
|
|
||||||||||
| # OR dead tuple bitmaps into combined bitmap | ||||||||||
| break pruneheap.c:1836 | ||||||||||
|
|
||||||||||
| # Record redirect-with-data for execute phase | ||||||||||
| break pruneheap.c:1863 | ||||||||||
|
|
||||||||||
| # Execute phase: apply redirect-with-data entries on the page | ||||||||||
| break pruneheap.c:1287 | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 8. WAL REPLAY — recovery of HOT indexed updates | ||||||||||
| # src/backend/access/heap/heapam_xlog.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # WAL replay for XLOG_HEAP2_INDEXED_UPDATE | ||||||||||
| break heap_xlog_indexed_update | ||||||||||
|
|
||||||||||
| # ========================================================================= | ||||||||||
| # 9. WAL LOGGING — writing HOT indexed update records | ||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||
| # ========================================================================= | ||||||||||
|
|
||||||||||
| # WAL logging for heap updates (handles indexed_update flag) | ||||||||||
| break log_heap_update | ||||||||||
|
|
||||||||||
| # Serialize redirect-with-data into WAL record (pruneheap.c) | ||||||||||
| break pruneheap.c:2936 | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Node modules | ||
| scripts/ai-review/node_modules/ | ||
| # Note: package-lock.json should be committed for reproducible CI/CD builds | ||
|
|
||
| # Logs | ||
| scripts/ai-review/cost-log-*.json | ||
| scripts/ai-review/*.log | ||
|
|
||
| # OS files | ||
| .DS_Store | ||
| Thumbs.db | ||
|
|
||
| # Editor files | ||
| *.swp | ||
| *.swo | ||
| *~ | ||
| .vscode/ | ||
| .idea/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
.clangdfile is personal editor/LSP configuration, not part of the change's stated purpose (the atomics/spin work in the rest of the diff). Committing personal tooling config into the tree is a top rejection reason on pgsql-hackers: it is unrelated churn, bloats the patch, and causes needless merge conflicts. The author's own.local-gitignore(which listsbuild/,.direnv/,.cache/,NOTES) and.github/docs/pristine-master-policy.mdconfirm these dev-setup files (.clangd,.envrc,.gdbinit,pg-aliases.sh,flake.nix,.vscode/) are meant for a separate personal branch, not master. Keep the patch minimal: drop this file from the change (add it to a local excludesFile instead).