Improve stats diffing on restarts and promotions - #846
Draft
msakrejda wants to merge 4 commits into
Draft
Conversation
Both interval calculations cast a time.Duration to uint32 without checking its sign, so a clock that moved backwards between two collections wrapped the interval. The full snapshot one also produced a garbage interval on the very first run for a server, where PrevState.CollectedAt is the zero time.Time and the subtraction saturates: report 1 second there instead, since that snapshot has no reference point to diff against.
Cumulative statistics are tracked in memory and are not synchronized between instances, so ensure both sides of the diff came from the same instance. The collector reference point is keyed by config section alone, so when a hostname resolves to a different instance over time the two get diffed anyway. An Aurora reader endpoint does exactly that: a failover promotes the reader to writer and demotes the old writer to a reader. Every query the collector had not seen before then had its entire lifetime counter attributed to a single interval, and since deallocated query texts all share one fingerprint, those counters summed into a single <query text unavailable> record reporting billions of calls per second. To address this, capture which instance and postmaster a connection reached, from pg_postmaster_start_time() and inet_server_addr(), and compare it before diffing. On a mismatch, reset the baseline: the 1min path drops one interval of query statistics, and the full snapshot diffs against an empty reference point (already the case at startup). This also covers a plain restart of the same instance (where stats are also lost). An identity we could not determine is treated as matching anything, so a failed probe or state written by an older collector keeps diffing as before rather than discarding a reference point we have no reason to distrust.
Each snapshot type opens its own connection, so each one resolves db_host independently. However, Aurora reader endpoints follow failovers, so consecutive connections can reach different instances. Within a single minute, the query statistics connection and the full snapshot connection can disagree. Resolve the hostname once per server and reuse the address, so the connections stay separate but talk to the same IP. The address is supplied to lib/pq through a custom dialer rather than by rewriting the host parameter, because pq takes the dial address from the dialer but reads the TLS server name straight out of the connection parameters. Passing it to the dialer leaves hostname verification intact. The pin is dropped when a connection through it fails, so a replaced instance recovers on the next attempt, and expires after 30 minutes to catch the case where the pinned instance still accepts connections but is no longer what the hostname points at. Hosts written as an IP, Unix sockets, and the Cloud SQL and AlloyDB drivers (which are given an instance name rather than a hostname) are left alone. This reduces how often the instance changes under us, but the reference point is still checked against the instance identity.
msakrejda
force-pushed
the
handle-restarts-better
branch
2 times, most recently
from
July 29, 2026 18:07
ae85159 to
fa59d06
Compare
The comments said cumulative statistics are only comparable to a reference point from the same postmaster. However, a clean shutdown persists them to pg_stat/ and reloads them at startup, and pg_stat_statements does the same under pg_stat_statements.save, so counters usually do survive a restart and diffing across one would have been fine. Treating a restart as a change is still the behavior we want, because the start time alone doesn't tell us whether the shutdown was clean, whether saving was on, or (where instances share a storage volume, as Aurora replicas do) whether the file the new postmaster read was even written by this instance. Also stop repeating the claim at both call sites, and point at PostgresInstanceIdentity for the reasoning instead.
msakrejda
force-pushed
the
handle-restarts-better
branch
from
July 29, 2026 18:09
fa59d06 to
f9836d9
Compare
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
1 participant
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.
See individual commits: