Harden exporter state and add follow-up TAP tests#53
Harden exporter state and add follow-up TAP tests#53iskakaushik wants to merge 1 commit intoClickHouse:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens pg_stat_ch’s background worker state handling (especially around stale PIDs and shutdown) and adds TAP coverage for new query-normalization behavior introduced in PostgreSQL 18.
Changes:
- Clear stored bgworker PID on worker shutdown and treat stale flush signal targets as “background worker not running”.
- Guard exporter failure-stat access before exporter initialization; improve shutdown logging format for 64-bit counters.
- Add TAP tests for stale bgworker flush behavior and PG18+ “squashed IN-list” query normalization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| t/031_squashed_in_list_normalization.pl | Adds PG18+ TAP coverage for squashed IN-list normalization output. |
| t/009_bgworker.pl | Adds TAP test ensuring flush reports “bgworker not running” (not stale-PID signal failure) after worker exit. |
| src/worker/bgworker.cc | Clears stored bgworker PID on exit; improves flush signaling warnings and stale-PID handling. |
| src/queue/shmem.cc | Uses UINT64_FORMAT for shutdown counter logging. |
| src/export/stats_exporter.cc | Guards consecutive-failure access when exporter isn’t initialized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (kill(bgworker_pid, SIGUSR2) != 0) { | ||
| ereport(WARNING, (errmsg("pg_stat_ch: failed to signal background worker"))); | ||
| if (errno == ESRCH) { | ||
| PschSetBgworkerPid(0); | ||
| ereport(WARNING, (errmsg("pg_stat_ch: background worker not running"))); | ||
| } else { | ||
| ereport(WARNING, (errmsg("pg_stat_ch: failed to signal background worker: %m"))); | ||
| } |
There was a problem hiding this comment.
In the kill() failure path, only ESRCH is treated as a stale PID. If the bgworker PID is reused by a non-Postgres process owned by another user, kill() can fail with EPERM; in that case the shared bgworker_pid remains set and pg_stat_ch_flush() will keep warning and never recover. Consider additionally clearing the stored PID when the PID is no longer a Postgres backend (e.g., BackendPidGetProc/IsBackendPid check) and treating that as “background worker not running”, instead of leaving a permanently stale PID.
| # The normalized form should be compact, not contain 20 separate placeholders | ||
| unlike($q, qr/\$5/, 'No excessive placeholders (list is squashed, not expanded)'); |
There was a problem hiding this comment.
This assertion doesn’t actually prove the IN-list was squashed to a single placeholder: checking that the query does not contain "$5" can still pass if the list incorrectly expands to only a few placeholders (e.g., $1..$4). It would be more robust to assert that the IN(...) clause contains exactly one $N placeholder plus the squashed comment marker (similar to the stricter regex used in the earlier numerics subtest).
| # The normalized form should be compact, not contain 20 separate placeholders | |
| unlike($q, qr/\$5/, 'No excessive placeholders (list is squashed, not expanded)'); | |
| # The normalized IN-list should contain exactly one placeholder plus | |
| # the squashed comment marker, not an expanded series of placeholders. | |
| like($q, qr/WHERE\s+id\s+IN\s*\(\s*\$\d+\s+\/\*, \.\.\. \*\/\s*\)/, | |
| 'IN list is squashed to a single placeholder'); |
| "SELECT * FROM test_squash WHERE id NOT IN (7, 8, 9)"); | ||
|
|
||
| like($q, qr{/\*, \.\.\. \*/}, 'Squashed comment marker present for NOT IN'); | ||
| unlike($q, qr/\b[7-9]\b/, 'NOT IN constants removed'); |
There was a problem hiding this comment.
This regex can match placeholder numbers as well (e.g., "$7") because word boundaries exist between "$" and a digit. That can make the test fail even when normalization is correct if placeholder numbering ever changes. Consider tightening the pattern to specifically target raw constants from the original IN list (e.g., match digits that aren’t immediately preceded by '$') or assert the exact normalized NOT IN form instead.
| unlike($q, qr/\b[7-9]\b/, 'NOT IN constants removed'); | |
| unlike($q, qr/(?<!\$)\b[7-9]\b/, 'NOT IN constants removed'); |
Summary
UINT64_FORMATin shutdown loggingTesting
./scripts/run-tests.sh ../postgres/install_tap tap 009./scripts/run-tests.sh ../postgres/install_tap tap 031