Skip to content

feat: [ansible/tasks] adding on-boot reindex in an even of collation version change - #2343

Open
srcCraftsman wants to merge 2 commits into
developfrom
artyom/PSQL-1588-upgrade-reindex-collation
Open

feat: [ansible/tasks] adding on-boot reindex in an even of collation version change#2343
srcCraftsman wants to merge 2 commits into
developfrom
artyom/PSQL-1588-upgrade-reindex-collation

Conversation

@srcCraftsman

@srcCraftsman srcCraftsman commented Aug 5, 2026

Copy link
Copy Markdown
Member

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:

  1. Skips when server_version_num < 150000 (the *_collation_actual_version() functions are PG15+).
  2. Loops over every connectable database (pg_collation is a per-database catalog).
  3. Per database: REINDEX the affected leaf indexes in user schemas (both libc and ICU providers), then ALTER COLLATION … REFRESH VERSION for the mismatched named collations.
  4. After the loop: ALTER DATABASE … REFRESH COLLATION VERSION for every mismatched database default (shared pg_database catalog, applied from a single connection).

@srcCraftsman
srcCraftsman requested review from a team as code owners August 5, 2026 12:09
@Crispy1975

Copy link
Copy Markdown
Contributor

I don't think this should run at boot. Ordering it Before=envoy.service blocks the only path the platform has to a project's Admin API service. Everything goes via https://[addr]/admin/v1 on 443 so poll_for_active, the /system/boot/status gate and the wal-g restore callbacks all fail while an unbounded REINDEX runs, and TimeoutStartSec=86400 lets that hold for a day. Restore-from-pause is the case most likely to trigger it, since a new AMI over an existing data volume is exactly the trigger described here (also R2NP).

Separately there's a correctness problem regardless of placement with a failed REINDEX is still followed by ALTER COLLATION ... REFRESH VERSION, so we clear the version stamp on an index we didn't rebuild and lose the only signal it's stale. This is the opposite of what PSQL-1336 set out to do, and it matters most for ICU where sort order genuinely changed.

The claim that the platform refreshCollationVersion task is a REINDEX CONCURRENTLY backstop... it isn't, that task only re-stamps libc and never reindexes. INC-667 already settled on a worker job plus pg-meta rather than a host-side rollout, and on fixing the known cohort rather than a fleet-wide sweep, so I'd move this to a post-upgrade/post-restore job on the collations queue with REINDEX CONCURRENTLY, primary only, and skip the refresh for any index that failed to rebuild.

@Crispy1975 Crispy1975 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bigger note in the main PR.

@srcCraftsman srcCraftsman reopened this Aug 6, 2026
@srcCraftsman

Copy link
Copy Markdown
Member Author

Left a bigger note in the main PR.

hopefully addressed in b6e41b5
By moving script into static adminapi scripts

@srcCraftsman
srcCraftsman requested a review from Crispy1975 August 6, 2026 10:25

@hunleyd hunleyd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • refresh_collation.sh:120,140
    • [CRITICAL, security] SQL/command injection → root RCE.
    • A customer with plain CREATE on a schema can create collation ... (version = '0') to force a "stale" match immediately, embed a newline in the name, and have the second half executed as an independent psql -c command — 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 bash read, plus a properly escaped dbname='...' 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_failed flag skips the refresh when set.
  • `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.
  • `refresh_collation.sh:131
    • No lock_timeout on REINDEX CONCURRENTLY meaning one idle-in-transaction client can block it forever. Fix: set via PGOPTIONS (not a second ;-joined statement, which would implicitly wrap in a transaction block REINDEX CONCURRENTLY refuses to run inside).
  • refresh_collation.sh:111
    • datallowconn wrongly excludes template0 from 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:74
    • pg_temp_%/pg_toast_temp_% not excluded which practically guarantees failed statements on busy instances. Fix: add the two LIKE exclusions.
  • refresh_collation.sh:154
    • Non-numeric server_version_num throws, bash reads the error as false, script proceeds as if the version check passed. Fix: validate ^[0-9]+$ first.
  • refresh_collation.sh:17
    • Comment wrongly claims both *_collation_actual_version() functions are PG15+ but only the database-level one is.
  • .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/failed state machine keyed off the process exit code, but since the script always exit 0 regardless of internal failures, GET /status will report "state":"completed" even on a run where every reindex and refresh statement failed, with only free-text WARN lines in output as the only tell. A real HTTP consumer of this endpoint has a concrete false-positive to hit, not just a hypothetical one.
  • 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 to pg_depend is probably wise
  • 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() reimplement common.sh's own helpers with different semantics (no ON_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants