feat: [ansible/tasks] adding on-boot reindex in an even of collation version change - #2343
feat: [ansible/tasks] adding on-boot reindex in an even of collation version change#2343srcCraftsman wants to merge 2 commits into
Conversation
|
I don't think this should run at boot. Ordering it Separately there's a correctness problem regardless of placement with a failed The claim that the platform |
Crispy1975
left a comment
There was a problem hiding this comment.
Left a bigger note in the main PR.
hopefully addressed in b6e41b5 |
hunleyd
left a comment
There was a problem hiding this comment.
refresh_collation.sh:120,140- [CRITICAL, security] SQL/command injection → root RCE.
- A customer with plain
CREATEon a schema cancreate collation ... (version = '0')to force a "stale" match immediately, embed a newline in the name, and have the second half executed as an independentpsql -ccommand — including a\!shell escape, run as root via this PR's own sudoers grant. Fix:\gexec-based server-side execution instead of round-tripping generated text through bashread, plus a properly escapeddbname='...'wrapper before any dynamic name reaches-d.
refresh_collation.sh:131,137- A reindex failure only logs a WARN; the collation/database-default REFRESH runs anyway, erasing the only signal that index still needs rebuilding. Fix: per-database
reindex_failedflag skips the refresh when set.
- A reindex failure only logs a WARN; the collation/database-default REFRESH runs anyway, erasing the only signal that index still needs rebuilding. Fix: per-database
- `refresh_collation.sh:147
- No
pg_is_in_recovery()guard meaning every affected index on a read replica would fail with "read-only transaction." Fix: skip immediately if in recovery.
- No
- `refresh_collation.sh:131
- No
lock_timeoutonREINDEX CONCURRENTLYmeaning one idle-in-transaction client can block it forever. Fix: set viaPGOPTIONS(not a second;-joined statement, which would implicitly wrap in a transaction blockREINDEX CONCURRENTLYrefuses to run inside).
- No
refresh_collation.sh:111datallowconnwrongly excludestemplate0from the database-default refresh. Fix: drop that predicate there only.
refresh_collation.sh:134,163- Enumeration failures are indistinguishable from "nothing to do." Fix: capture and check exit status before treating empty as clean.
refresh_collation.sh:74pg_temp_%/pg_toast_temp_%not excluded which practically guarantees failed statements on busy instances. Fix: add the twoLIKEexclusions.
refresh_collation.sh:154- Non-numeric
server_version_numthrows, bash reads the error as false, script proceeds as if the version check passed. Fix: validate^[0-9]+$first.
- Non-numeric
refresh_collation.sh:17- Comment wrongly claims both
*_collation_actual_version()functions are PG15+ but only the database-level one is.
- Comment wrongly claims both
.github/workflows/publish-nix-pgupgrade-scripts.yml:9- Path filter doesn't cover this script's directory — won't reach the S3-published tarball on merge. Fix: add the path.
refresh_collation.sh:12- Header claims "the platform refreshCollationVersion task is the backstop." checked #361, no mention of any backstop/retry task there either.
refresh_collation.sh:175- every path exits 0, no machine-readable success/failure signal. #361 makes this concretely worse as it wraps the script in a real
idle/running/completed/failedstate machine keyed off the process exit code, but since the script alwaysexit 0regardless of internal failures,GET /statuswill report"state":"completed"even on a run where every reindex and refresh statement failed, with only free-textWARNlines inoutputas the only tell. A real HTTP consumer of this endpoint has a concrete false-positive to hit, not just a hypothetical one.
- every path exits 0, no machine-readable success/failure signal. #361 makes this concretely worse as it wraps the script in a real
refresh_collation.sh:76- Detection only covers
pg_index.indcollation. partial-index predicates, CHECK constraints, partition bounds aren't covered, yet get marked "refreshed" anyway. Widening topg_dependis probably wise
- Detection only covers
refresh_collation.sh- Wrong directory: rides along in the pg_upgrade tarball despite not being a pg_upgrade step.
refresh_collation.sh:128,52- collapse the 3 near-identical loop blocks into one helper
- extract the 4x-duplicated stale-collation predicate
run_sql()/retry()reimplementcommon.sh's own helpers with different semantics (noON_ERROR_STOP, flat vs exponential-backoff retry) so sourcing it needs those differences reconciled first, not a blind swap.
refresh_collation.sh- No test/self-check for this logic at all.
audit-specs/baselines/*.yml- New file has no GOSS baseline entry, unlike every sibling script.
Refresh collation versions and reindex affected indexes script added as static script under adminadpi folder
Reindex will happen only when collation version change has been detected, otherwise just exits.
How it's working: