Conversation
|
Found 20 test failures on Blacksmith runners: Failures
...and 10 more test failures. View all on Blacksmith
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.svg)
PostgreSQL Extension Dependency Analysis: PR #2459
SummaryNo extensions had dependencies with MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Extension DependenciesPostgreSQL 17 Extension DependenciesOrioleDB 17 Extension Dependencies |
PostgreSQL Package Dependency Analysis: PR #2459
SummaryNo packages had MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Dependency ChangesExtracting PostgreSQL 15 dependencies...
Runtime Closure Size
Raw Dependency ClosurePostgreSQL 17 Dependency ChangesExtracting PostgreSQL 17 dependencies...
Runtime Closure Size
Raw Dependency Closure |
- LimitCORE=infinity, scoped to postgresql.service only (not machine-wide) so an unrelated crash still produces no core - CoredumpFilter excludes shared_buffers (an anonymous shared mapping) from every core, since it can be many GB - this is what actually bounds core size - systemd-coredump gets its own conservative storage limits (compression, size caps), and is installed explicitly (not assumed to already be present) - confirm postgres_prestart.sh never resets ulimit -c (regression guard) - testinfra: LimitCORE, coredump_filter value, storage limits, coredump dir permissions, and a real end-to-end crash test (crash produces a core, an unrelated process's crash doesn't)
- postgres/orioledb debug+source packages already ship on every AMI (postgres-env bundle) - nothing new to package here, verification only - check build-ID match: shipped postgres binary and orioledb.so vs the installed debug package - check GDB can read real source content through the source package (not just that a filename is known - info sources lists names regardless of whether the file is actually reachable on disk)
- orioledb-coredump.path (triggers on new cores) + .timer (10-min fallback sweep) both run a flock-guarded oneshot service - gdb and binutils are installed explicitly for this (scoped to OrioleDB images via is_psql_oriole, since only this processor needs them) - processor script: keeps only postgres-owned cores, extracts a bundle via GDB (cmds.gdb - matches OrioleDB's own CI debugging script: full backtrace, argv, shared libraries, registers, lock state), deletes the raw core, quarantines metadata-only after repeated failures, and enforces its own age/size retention independent of systemd-coredump's own limits - handles /usr/lib/postgresql/bin/postgres being a Nix wrapper script (not the real ELF) throughout: the executable filter, readelf, and gdb all use the real per-crash path reported by coredumpctl instead of a fixed guess - matches/deletes cores by PID, not raw file path (coredumpctl has no "rm" verb on this systemd version, and path-based matching was unreliable) - reads the active postgresql log path from current_logfiles at runtime, since this AMI logs via csvlog with no fixed filename - rolled out to OrioleDB images only for now (is_psql_oriole) - testinfra: end-to-end check that a real crash produces a diagnostic bundle and the raw core gets deleted
pgnickb
force-pushed
the
nickb/orioledb_coredump
branch
from
September 23, 2026 07:30
0ac3a3f to
e3655f2
Compare
mmlb
reviewed
Sep 23, 2026
| assert mode in ("700", "750"), ( | ||
| f"Expected /var/lib/systemd/coredump to be root-only, got mode {mode}" | ||
| ) | ||
| assert owner.startswith("root:"), ( |
Collaborator
There was a problem hiding this comment.
Why not root:root? One of the allowed modes is 750 and if this ends up with some other group that can be escaped to it can be a problem?
Contributor
Author
There was a problem hiding this comment.
But will be caught by the check below. And afaiu it isn't a problem since group can only read the filenames which isn't critical.
| host["ssh"], "cat /usr/local/bin/postgres_prestart.sh" | ||
| ) | ||
| assert result["succeeded"], f"Could not read prestart script: {result['stderr']}" | ||
| assert "ulimit -c" not in result["stdout"], ( |
Collaborator
There was a problem hiding this comment.
This worries me slightly, I'd feel slightly better with a regex for handling spaces but even then thats pretty light. Why don't we block on ulimit completely as an extra conservative check? We can handle dealing with w/o ulimit is being added when we get there...
Contributor
Author
There was a problem hiding this comment.
Totally agree on the regex. Good call.
Not sure we want to be as tight on other things ulimit though. Ultimately it's your call of course. Do you think this is the right place to do that? I'd at least rename the test then.
Collaborator
There was a problem hiding this comment.
yeah I'd say lets just do ulimit itself, its not there now so we can cross that bridge when we get there (probably good to have to discuss it then anyway)
Comment on lines
+1577
to
+1579
| new_lines = [ | ||
| line for line in after["stdout"].splitlines() if line not in before_lines | ||
| ] |
Collaborator
There was a problem hiding this comment.
Suggested change
| new_lines = [ | |
| line for line in after["stdout"].splitlines() if line not in before_lines | |
| ] | |
| new_lines = set(after["stdout"].splitlines()) - before |
Collaborator
There was a problem hiding this comment.
previous was fine but since you already setup before as a set() my mind did not like that you didn't use set here too :D
| PrivateTmp=yes | ||
| ProtectSystem=strict | ||
| ProtectHome=yes | ||
| ReadWritePaths=/var/lib/systemd/coredump /var/lib/orioledb-coredumps /run |
Collaborator
There was a problem hiding this comment.
Suggested change
| ReadWritePaths=/var/lib/systemd/coredump /var/lib/orioledb-coredumps /run | |
| ReadOnlyPaths=/var/lib/systemd/coredump | |
| ReadWritePaths=/var/lib/orioledb-coredumps /run |
Contributor
Author
There was a problem hiding this comment.
That's the coredump path. We delete from that path to enforce our own retention.
I.e. processed cores are deleted, unprocessable cores are deleted after 3 attemps, etc. See ansible/files/coredump/process-orioledb-coredumps.sh
Collaborator
There was a problem hiding this comment.
ah yeah that makes sense, seems fine then.
Comment on lines
+5
to
+6
| OnBootSec=10min | ||
| OnUnitActiveSec=10min |
Collaborator
There was a problem hiding this comment.
We already have issues with salt every 10m and this should be very rare situations, can we bump to 1/h or something instead? Also consider adding a RandomizedDelaySec=15m or something too?
Contributor
Author
There was a problem hiding this comment.
Yeah it's valid. The only risk is a crash loop scenario and coredump-ctl imposing its own retention, but in that scenario every individual coredump isn't that valuable. Will do.
| name: | ||
| - gdb | ||
| - binutils | ||
| become: true |
Collaborator
There was a problem hiding this comment.
unnecessary, I'm going to drop them all at some point and would prefer to no introduce new ones :D, ditto down the file
Suggested change
| become: true |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
feature: collect and process coredumps for orioledb instances.
What is the current behavior?
coredumpts arent' collected
What is the new behavior?
upon crash a coredump is written, then it is processed into a bundle including:
Additional context
this work is critical for orioledb team as we're about to start beta and debugging any future crashes without coredump is going to be very problematic.
One important caveat: currently only
postgresdebug symbols are shipped. Shippingorioledbdebug symbols is outside of the scope of this PR.