diff --git a/.changeset/@accounter_server-4363-dependencies.md b/.changeset/@accounter_server-4363-dependencies.md new file mode 100644 index 000000000..2cfa5f9f2 --- /dev/null +++ b/.changeset/@accounter_server-4363-dependencies.md @@ -0,0 +1,8 @@ +--- +"@accounter/server": patch +--- +dependencies updates: + - Updated dependency [`@opentelemetry/exporter-trace-otlp-http@0.222.0` ↗︎](https://www.npmjs.com/package/@opentelemetry/exporter-trace-otlp-http/v/0.222.0) (from `0.221.0`, in `dependencies`) + - Updated dependency [`@opentelemetry/resources@2.11.0` ↗︎](https://www.npmjs.com/package/@opentelemetry/resources/v/2.11.0) (from `2.10.0`, in `dependencies`) + - Updated dependency [`@opentelemetry/sdk-node@0.222.0` ↗︎](https://www.npmjs.com/package/@opentelemetry/sdk-node/v/0.222.0) (from `0.221.0`, in `dependencies`) + - Updated dependency [`googleapis@178.0.0` ↗︎](https://www.npmjs.com/package/googleapis/v/178.0.0) (from `176.0.0`, in `dependencies`) diff --git a/.changeset/postgres-18-upgrade-record.md b/.changeset/postgres-18-upgrade-record.md new file mode 100644 index 000000000..e5ea55fe2 --- /dev/null +++ b/.changeset/postgres-18-upgrade-record.md @@ -0,0 +1,20 @@ +--- +'@accounter/server': patch +--- + +Add `yarn db:reindex-trgm` and record the Postgres 16 → 18 production upgrade. + +`scripts/reindex-trigram-indexes.ts` rebuilds every GIN trigram index, always `CONCURRENTLY`. +It discovers the indexes from `pg_index`/`pg_opclass` at runtime rather than from a hardcoded +list — the set grew from six to seven when `idx_financial_entities_name_trgm` was added, and a +copied list silently skips the new one. `CONCURRENTLY` is not optional: only that seventh index +was originally built concurrently, so a plain `REINDEX` would take `ACCESS EXCLUSIVE` on +`charges`, `transactions` and `documents`. + +Also reports the collation provider (which decides whether the reindex is required at all), +detects invalid indexes and `_ccnew` leftovers from a failed rebuild, disables +`statement_timeout` for the session, and re-reads the catalog afterwards rather than trusting +the absence of errors. + +Docs-only otherwise: the upgrade audit, the runbook with its execution record, and the +before/after catalog snapshots. diff --git a/docs/operations/postgres-18-baselines/16-catalog-snapshot.txt b/docs/operations/postgres-18-baselines/16-catalog-snapshot.txt new file mode 100644 index 000000000..d4757382a --- /dev/null +++ b/docs/operations/postgres-18-baselines/16-catalog-snapshot.txt @@ -0,0 +1,184 @@ +Pager usage is off. +Timing is off. +================ 1. server version and collation provider ================ + version +------------------------------------------------------------------------------- + PostgreSQL 16.14 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 13.2.0, 64-bit + + datname | datlocprovider | datcollate | datctype | datcollversion +-------------------+----------------+------------+------------+---------------- + accounter_prod_db | c | en_US.utf8 | en_US.utf8 | 2.38 + +================ 2. extensions and versions ================ + extname | extversion | schema +---------+------------+------------ + pg_trgm | 1.6 | public + plpgsql | 1.0 | pg_catalog + +================ 3. RLS coverage summary ================ + tables | rls_enabled | rls_forced +--------+-------------+------------ + 101 | 60 | 60 + +---- tables WITHOUT RLS. has_owner_id = true means tenant data with no policy ---- + relname | owner | has_owner_id +---------------------------------------------+---------------------+-------------- + salaries | prod_group | t + amex_creditcard_transactions | prod_group | f + api_key_permission_overrides | prod_group | f + api_keys | prod_group | f + audit_logs | prod_group | f + bank_discount_transactions | prod_group | f + business_trips_tax_variables | prod_group | f + business_users | prod_group | f + businesses_green_invoice_match | prod_group | f + cal_creditcard_transactions | prod_group | f + countries | prod_group | f + crypto_currencies | prod_group | f + crypto_exchange_rates | prod_group | f + depreciation_categories | prod_group | f + etana_account_transactions | prod_group | f + etherscan_transactions | prod_group | f + exchange_rates | prod_group | f + invitations | prod_group | f + isracard_creditcard_transactions | prod_group | f + kraken_ledger_records | prod_group | f + kraken_trades | prod_group | f + max_creditcard_transactions | prod_group | f + migration | prod_group | f + permissions | prod_group | f + poalim_cad_account_transactions | prod_group | f + poalim_deposits_account_transactions | prod_group | f + poalim_deposits_account_transactions_manual | prod_group | f + poalim_eur_account_transactions | prod_group | f + poalim_foreign_account_transactions | prod_group | f + poalim_gbp_account_transactions | prod_group | f + poalim_ils_account_transactions | prod_group | f + poalim_swift_account_transactions | prod_group | f + poalim_usd_account_transactions | prod_group | f + recovery | prod_group | f + role_permissions | prod_group | f + roles | prod_group | f + super_admins | accounter_prod_user | f + transactions_raw_list | prod_group | f + user_permission_overrides | prod_group | f + users | prod_group | f + vat_value | prod_group | f + +---- tables WITH RLS enabled but NOT forced (owner escapes its own policy) ---- + relname | owner +---------+------- + +================ 4. table ownership distribution ================ + owner | relkind | count +---------------------+---------+------- + accounter_prod_user | r | 17 + accounter_prod_user | v | 2 + prod_group | r | 84 + prod_group | v | 1 + +================ 5. views: security_invoker must stay UNSET ================ + relname | owner | reloptions +-------------------------------------+---------------------+------------ + extended_business_trip_transactions | accounter_prod_user | + extended_charges | accounter_prod_user | + extended_tags | prod_group | + +================ 6. roles and attributes ================ + rolname | rolsuper | rolbypassrls | rolcreatedb | rolcreaterole | rolcanlogin | rolinherit +------------------------+----------+--------------+-------------+---------------+-------------+------------ + accounter_admin | f | t | t | t | t | t + accounter_prod_user | f | f | f | f | t | t + accounter_staging_user | f | f | f | f | t | t + azuresu | t | t | t | t | t | t + prod_group | f | f | f | f | f | t + +---- role memberships (INHERIT TRUE is why FORCE RLS is load-bearing) ---- + member | granted_role | admin_option | inherit_option +---------------------+------------------------+--------------+---------------- + accounter_admin | accounter_prod_user | t | f + accounter_admin | accounter_staging_user | t | f + accounter_admin | azure_pg_admin | t | t + accounter_admin | pg_read_all_settings | t | t + accounter_admin | pg_read_all_stats | t | t + accounter_admin | pg_stat_scan_tables | t | t + accounter_admin | prod_group | t | f + accounter_admin | prod_group | f | t + accounter_prod_user | prod_group | f | t + +================ 7. RLS helper functions: owner + SECURITY DEFINER ================ + proname | owner | security_definer | provolatile | proconfig +----------------------------+---------------------+------------------+-------------+-------------------------- + get_current_auth_type | prod_group | t | s | {search_path=pg_catalog} + get_current_business_id | prod_group | t | s | {search_path=pg_catalog} + get_current_business_scope | accounter_prod_user | t | s | {search_path=pg_catalog} + get_current_user_id | prod_group | t | s | {search_path=pg_catalog} + +================ 8. tenant_isolation policies ================ + policy_count | polcmd | polpermissive +--------------+--------+--------------- + 62 | * | t + 1 | a | t + 50 | d | f + 3 | r | t + 4 | w | f + 1 | w | t + +---- representative policy definitions ---- + tablename | policyname | permissive | cmd | qual | with_check +--------------------+------------------------------+-------------+--------+------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------- + charges | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + charges | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + documents | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + documents | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + financial_entities | allow_bootstrap_owner_update | PERMISSIVE | UPDATE | (id = accounter_schema.get_current_business_id()) | (owner_id = id) + financial_entities | allow_bootstrap_root | PERMISSIVE | ALL | (id = accounter_schema.get_current_business_id()) | ((id = accounter_schema.get_current_business_id()) OR (owner_id = accounter_schema.get_current_business_id())) + financial_entities | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + financial_entities | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + tags | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + tags | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + transactions | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + transactions | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + +================ 9. trigram indexes and validity ================ + schema | index_name | table_name | indisvalid | indisready | size +------------------+----------------------------------+--------------------+------------+------------+--------- + accounter_schema | idx_charges_desc_trgm | charges | t | t | 1568 kB + accounter_schema | idx_docs_desc_trgm | documents | t | t | 2960 kB + accounter_schema | idx_docs_remarks_trgm | documents | t | t | 2192 kB + accounter_schema | idx_docs_serial_trgm | documents | t | t | 920 kB + accounter_schema | idx_financial_entities_name_trgm | financial_entities | t | t | 464 kB + accounter_schema | idx_trans_src_ref_trgm | transactions | t | t | 3416 kB + accounter_schema | idx_trans_src_trgm | transactions | t | t | 4656 kB + +---- any INVALID index anywhere in the schema (planner silently ignores these) ---- + relname | indisvalid | indisready +---------+------------+------------ + +================ 10. expression indexes (own statistics, need ANALYZE) ================ + index_name | table_name | definition +---------------------------------------------------+----------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + amex_creditcard_transactions_dedup_uindex | amex_creditcard_transactions | CREATE UNIQUE INDEX amex_creditcard_transactions_dedup_uindex ON accounter_schema.amex_creditcard_transactions USING btree (card, COALESCE(full_purchase_date, full_purchase_date_outbound), full_payment_date, COALESCE(payment_sum, payment_sum_outbound), voucher_number, COALESCE(voucher_number_ratz, voucher_number_ratz_outbound), supplier_id, current_payment_currency, COALESCE(full_supplier_name_heb, full_supplier_name_outbound), more_info, deal_sum) NULLS NOT DISTINCT + idx_documents_owner_vat_report_date | documents | CREATE INDEX idx_documents_owner_vat_report_date ON accounter_schema.documents USING btree (owner_id, COALESCE(vat_report_date_override, date)) + email_ingestion_alias_routing_alias_active_unique | email_ingestion_alias_routing | CREATE UNIQUE INDEX email_ingestion_alias_routing_alias_active_unique ON accounter_schema.email_ingestion_alias_routing USING btree (lower(alias)) WHERE (is_active = true) + isracard_creditcard_transactions_dedup_uindex | isracard_creditcard_transactions | CREATE UNIQUE INDEX isracard_creditcard_transactions_dedup_uindex ON accounter_schema.isracard_creditcard_transactions USING btree (card, COALESCE(full_purchase_date, full_purchase_date_outbound), full_payment_date, COALESCE(payment_sum, payment_sum_outbound), voucher_number, COALESCE(voucher_number_ratz, voucher_number_ratz_outbound), supplier_id, current_payment_currency, COALESCE(full_supplier_name_heb, full_supplier_name_outbound), more_info, deal_sum) NULLS NOT DISTINCT + idx_transactions_owner_effective_debit_date | transactions | CREATE INDEX idx_transactions_owner_effective_debit_date ON accounter_schema.transactions USING btree (owner_id, COALESCE(debit_date_override, debit_date)) + +================ 11. row counts for the main tenant tables ================ + t | count +----------------+------- + charges | 26302 + documents | 9313 + ledger_records | 24131 + transactions | 29343 + +================ 12. database size ================ + db_size +--------- + 105 MB + +================ end of snapshot ================ + captured_at_utc +------------------------------- + 2026-08-31 16:34:13.341847+00 + diff --git a/docs/operations/postgres-18-baselines/PROD18-catalog-snapshot.txt b/docs/operations/postgres-18-baselines/PROD18-catalog-snapshot.txt new file mode 100644 index 000000000..832728495 --- /dev/null +++ b/docs/operations/postgres-18-baselines/PROD18-catalog-snapshot.txt @@ -0,0 +1,184 @@ +Pager usage is off. +Timing is off. +================ 1. server version and collation provider ================ + version +------------------------------------------------------------------------------ + PostgreSQL 18.6 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 13.2.0, 64-bit + + datname | datlocprovider | datcollate | datctype | datcollversion +-------------------+----------------+------------+------------+---------------- + accounter_prod_db | c | en_US.utf8 | en_US.utf8 | 2.38 + +================ 2. extensions and versions ================ + extname | extversion | schema +---------+------------+------------ + pg_trgm | 1.6 | public + plpgsql | 1.0 | pg_catalog + +================ 3. RLS coverage summary ================ + tables | rls_enabled | rls_forced +--------+-------------+------------ + 101 | 60 | 60 + +---- tables WITHOUT RLS. has_owner_id = true means tenant data with no policy ---- + relname | owner | has_owner_id +---------------------------------------------+---------------------+-------------- + salaries | prod_group | t + amex_creditcard_transactions | prod_group | f + api_key_permission_overrides | prod_group | f + api_keys | prod_group | f + audit_logs | prod_group | f + bank_discount_transactions | prod_group | f + business_trips_tax_variables | prod_group | f + business_users | prod_group | f + businesses_green_invoice_match | prod_group | f + cal_creditcard_transactions | prod_group | f + countries | prod_group | f + crypto_currencies | prod_group | f + crypto_exchange_rates | prod_group | f + depreciation_categories | prod_group | f + etana_account_transactions | prod_group | f + etherscan_transactions | prod_group | f + exchange_rates | prod_group | f + invitations | prod_group | f + isracard_creditcard_transactions | prod_group | f + kraken_ledger_records | prod_group | f + kraken_trades | prod_group | f + max_creditcard_transactions | prod_group | f + migration | prod_group | f + permissions | prod_group | f + poalim_cad_account_transactions | prod_group | f + poalim_deposits_account_transactions | prod_group | f + poalim_deposits_account_transactions_manual | prod_group | f + poalim_eur_account_transactions | prod_group | f + poalim_foreign_account_transactions | prod_group | f + poalim_gbp_account_transactions | prod_group | f + poalim_ils_account_transactions | prod_group | f + poalim_swift_account_transactions | prod_group | f + poalim_usd_account_transactions | prod_group | f + recovery | prod_group | f + role_permissions | prod_group | f + roles | prod_group | f + super_admins | accounter_prod_user | f + transactions_raw_list | prod_group | f + user_permission_overrides | prod_group | f + users | prod_group | f + vat_value | prod_group | f + +---- tables WITH RLS enabled but NOT forced (owner escapes its own policy) ---- + relname | owner +---------+------- + +================ 4. table ownership distribution ================ + owner | relkind | count +---------------------+---------+------- + accounter_prod_user | r | 17 + accounter_prod_user | v | 2 + prod_group | r | 84 + prod_group | v | 1 + +================ 5. views: security_invoker must stay UNSET ================ + relname | owner | reloptions +-------------------------------------+---------------------+------------ + extended_business_trip_transactions | accounter_prod_user | + extended_charges | accounter_prod_user | + extended_tags | prod_group | + +================ 6. roles and attributes ================ + rolname | rolsuper | rolbypassrls | rolcreatedb | rolcreaterole | rolcanlogin | rolinherit +------------------------+----------+--------------+-------------+---------------+-------------+------------ + accounter_admin | f | t | t | t | t | t + accounter_prod_user | f | f | f | f | t | t + accounter_staging_user | f | f | f | f | t | t + azuresu | t | t | t | t | t | t + prod_group | f | f | f | f | f | t + +---- role memberships (INHERIT TRUE is why FORCE RLS is load-bearing) ---- + member | granted_role | admin_option | inherit_option +---------------------+------------------------+--------------+---------------- + accounter_admin | accounter_prod_user | t | f + accounter_admin | accounter_staging_user | t | f + accounter_admin | azure_pg_admin | t | t + accounter_admin | pg_read_all_settings | t | t + accounter_admin | pg_read_all_stats | t | t + accounter_admin | pg_stat_scan_tables | t | t + accounter_admin | prod_group | t | f + accounter_admin | prod_group | f | t + accounter_prod_user | prod_group | f | t + +================ 7. RLS helper functions: owner + SECURITY DEFINER ================ + proname | owner | security_definer | provolatile | proconfig +----------------------------+---------------------+------------------+-------------+-------------------------- + get_current_auth_type | prod_group | t | s | {search_path=pg_catalog} + get_current_business_id | prod_group | t | s | {search_path=pg_catalog} + get_current_business_scope | accounter_prod_user | t | s | {search_path=pg_catalog} + get_current_user_id | prod_group | t | s | {search_path=pg_catalog} + +================ 8. tenant_isolation policies ================ + policy_count | polcmd | polpermissive +--------------+--------+--------------- + 62 | * | t + 1 | a | t + 50 | d | f + 3 | r | t + 4 | w | f + 1 | w | t + +---- representative policy definitions ---- + tablename | policyname | permissive | cmd | qual | with_check +--------------------+------------------------------+-------------+--------+------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------- + charges | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + charges | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + documents | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + documents | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + financial_entities | allow_bootstrap_owner_update | PERMISSIVE | UPDATE | (id = accounter_schema.get_current_business_id()) | (owner_id = id) + financial_entities | allow_bootstrap_root | PERMISSIVE | ALL | (id = accounter_schema.get_current_business_id()) | ((id = accounter_schema.get_current_business_id()) OR (owner_id = accounter_schema.get_current_business_id())) + financial_entities | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + financial_entities | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + tags | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + tags | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + transactions | tenant_isolation | PERMISSIVE | ALL | (owner_id = ANY (accounter_schema.get_current_business_scope())) | (owner_id = accounter_schema.get_current_business_id()) + transactions | tenant_isolation_delete | RESTRICTIVE | DELETE | (owner_id = accounter_schema.get_current_business_id()) | + +================ 9. trigram indexes and validity ================ + schema | index_name | table_name | indisvalid | indisready | size +------------------+----------------------------------+--------------------+------------+------------+--------- + accounter_schema | idx_charges_desc_trgm | charges | t | t | 832 kB + accounter_schema | idx_docs_desc_trgm | documents | t | t | 1392 kB + accounter_schema | idx_docs_remarks_trgm | documents | t | t | 1288 kB + accounter_schema | idx_docs_serial_trgm | documents | t | t | 912 kB + accounter_schema | idx_financial_entities_name_trgm | financial_entities | t | t | 472 kB + accounter_schema | idx_trans_src_ref_trgm | transactions | t | t | 1352 kB + accounter_schema | idx_trans_src_trgm | transactions | t | t | 2912 kB + +---- any INVALID index anywhere in the schema (planner silently ignores these) ---- + relname | indisvalid | indisready +---------+------------+------------ + +================ 10. expression indexes (own statistics, need ANALYZE) ================ + index_name | table_name | definition +---------------------------------------------------+----------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + amex_creditcard_transactions_dedup_uindex | amex_creditcard_transactions | CREATE UNIQUE INDEX amex_creditcard_transactions_dedup_uindex ON accounter_schema.amex_creditcard_transactions USING btree (card, COALESCE(full_purchase_date, full_purchase_date_outbound), full_payment_date, COALESCE(payment_sum, payment_sum_outbound), voucher_number, COALESCE(voucher_number_ratz, voucher_number_ratz_outbound), supplier_id, current_payment_currency, COALESCE(full_supplier_name_heb, full_supplier_name_outbound), more_info, deal_sum) NULLS NOT DISTINCT + idx_documents_owner_vat_report_date | documents | CREATE INDEX idx_documents_owner_vat_report_date ON accounter_schema.documents USING btree (owner_id, COALESCE(vat_report_date_override, date)) + email_ingestion_alias_routing_alias_active_unique | email_ingestion_alias_routing | CREATE UNIQUE INDEX email_ingestion_alias_routing_alias_active_unique ON accounter_schema.email_ingestion_alias_routing USING btree (lower(alias)) WHERE (is_active = true) + isracard_creditcard_transactions_dedup_uindex | isracard_creditcard_transactions | CREATE UNIQUE INDEX isracard_creditcard_transactions_dedup_uindex ON accounter_schema.isracard_creditcard_transactions USING btree (card, COALESCE(full_purchase_date, full_purchase_date_outbound), full_payment_date, COALESCE(payment_sum, payment_sum_outbound), voucher_number, COALESCE(voucher_number_ratz, voucher_number_ratz_outbound), supplier_id, current_payment_currency, COALESCE(full_supplier_name_heb, full_supplier_name_outbound), more_info, deal_sum) NULLS NOT DISTINCT + idx_transactions_owner_effective_debit_date | transactions | CREATE INDEX idx_transactions_owner_effective_debit_date ON accounter_schema.transactions USING btree (owner_id, COALESCE(debit_date_override, debit_date)) + +================ 11. row counts for the main tenant tables ================ + t | count +----------------+------- + charges | 26358 + documents | 9381 + ledger_records | 24176 + transactions | 29357 + +================ 12. database size ================ + db_size +--------- + 99 MB + +================ end of snapshot ================ + captured_at_utc +------------------------------- + 2026-09-02 09:57:16.709766+00 + diff --git a/docs/operations/postgres-18-baselines/README.md b/docs/operations/postgres-18-baselines/README.md new file mode 100644 index 000000000..07d143dc0 --- /dev/null +++ b/docs/operations/postgres-18-baselines/README.md @@ -0,0 +1,174 @@ +# Postgres 16 → 18 upgrade baselines + +Before/after evidence for the major version upgrade. See `docs/operations/postgres-18-upgrade.md` +for the procedure these files support. + +The point of everything here is **paired** captures: a fact recorded on 16 and the same fact +recorded on 18. A capture with no counterpart proves nothing, and after the upgrade the 16 side can +never be obtained again. + +## Contents + +| File | What it is | Committed | +| ----------------------------- | ---------------------------------------------------------------- | ---------------------- | +| `pre-upgrade-catalog.psql` | Catalog snapshot script. Metadata only, re-runnable | yes | +| `capture-plans.psql` | Plan capture script (dedup + date-range queries) | yes | +| `rehearsal-command-sheet.md` | The 11-step rehearsal procedure, as executed | yes | +| `16-catalog-snapshot.txt` | Production catalog **before** the upgrade (2026-08-31, PG 16.14) | yes — no customer data | +| `PROD18-catalog-snapshot.txt` | Production catalog **after** the upgrade (2026-09-02, PG 18.6) | yes — no customer data | + +**The raw `EXPLAIN` captures are deliberately not committed and were deleted after use.** They +contained real bank account numbers, branch numbers and security identifiers — see the redaction +rule below. Every conclusion drawn from them is recorded in the Findings section of this file and in +`../postgres-18-upgrade.md`; the raw output had no remaining value proportional to the risk. + +Regenerate them any time with `capture-plans.psql` if a future comparison needs them. Note that +script's section 3 now reports **counts only** for exactly this reason: an earlier version printed +sample rows, which is how customer data ended up in a file in the first place. + +## Redaction rule — read before committing any plan capture + +The catalog snapshot is metadata and contains no customer data. **Query plans are different.** + +An `EXPLAIN` of the free-text search contains the search term. `auto_explain` output contains query +text and can contain bound parameter values — real business names, amounts, document numbers, +counterparty names. None of that belongs in a git repository. + +Before committing any `*-dedup-*` or `*-auto-explain-*` file: + +1. **Drive the captures with synthetic search terms you invented.** Do not search for a real + customer or supplier name to generate a plan. +2. **Replace UUIDs with stable placeholders** — `OWNER_A`, `OWNER_B`, `CHARGE_1`. Stable so the 16 + and 18 files stay diffable. +3. **Strip literal amounts, serial numbers, and free-text values** from the captured SQL. +4. Keep the plan _shape_: node types, index names, row estimates vs actual, loops, buffer counts. + That is the entire basis for the comparison; literal values contribute nothing to it. + +If a capture cannot be scrubbed without destroying its meaning, keep it out of the repo and store it +wherever your other production artifacts live. + +## Capturing the catalog snapshot + +```sh +printf 'accounter_admin password: ' +read -rs PGPASSWORD +echo +docker exec -i -e PGPASSWORD="$PGPASSWORD" accounter-dev-db-1 \ + psql --host=$PROD_HOST \ + --username=accounter_admin --dbname=accounter_prod_db \ + < docs/operations/postgres-18-baselines/pre-upgrade-catalog.psql \ + > docs/operations/postgres-18-baselines/16-catalog-snapshot.txt 2>&1 +unset PGPASSWORD +``` + +Run as `accounter_admin`, not `accounter_prod_user`: the app role has no RLS session variables set, +so `get_current_business_id()` raises and section 11's row counts would error instead of returning. + +After the upgrade, re-run the identical script against the upgraded server, write +`18-catalog-snapshot.txt`, and diff: + +```sh +diff -u 16-catalog-snapshot.txt 18-catalog-snapshot.txt +``` + +**Expected** differences: `version()`, extension versions, index sizes, row counts, +`captured_at_utc`. + +**Anything else is a finding** — in particular any change to RLS coverage, `FORCE` flags, view +`reloptions`, role attributes, memberships, or function ownership. + +### `datcollversion` is a finding, not an expected difference + +It is tempting to wave this one through alongside `version()`. Don't. `datcollversion` is the glibc +collation version the database was built against, and if the upgrade lands the server on a different +base image it changes — which means **every index whose ordering depends on collation is potentially +mis-sorted**: all btree indexes on `text`/`varchar`, unique constraints on text columns, any string +comparison. That is a much larger job than the seven trigram indexes, and it is invisible unless you +compare these two files. + +Baseline captured on production 2026-08-31: **`datcollversion = 2.38`**, `datlocprovider = 'c'` +(libc), `datcollate = en_US.utf8`. + +If the 18 snapshot shows a different value, see § _1b. The collation version_ in +[`../postgres-18-upgrade.md`](../postgres-18-upgrade.md): reindex first, then +`ALTER DATABASE … REFRESH COLLATION VERSION` — never the other way round, which would hide the +problem rather than fix it. + +Note this cannot be rehearsed against the local dev container: `postgres:18-alpine` uses musl and +reports no collation version at all, so `datcollversion` comes back empty there. Only a real Azure +instance — i.e. the PITR-restored rehearsal server — gives a meaningful reading. + +## Findings from the 16 baseline (2026-08-31) + +### 1. `CREATE INDEX CONCURRENTLY` leaves expression indexes without statistics + +The first plan capture (kept as `16-plans-scope*-PRE-ANALYZE.txt`) was taken hours after #4331 +deployed and shows wildly wrong estimates for the two `COALESCE` indexes — **51 estimated vs 1671 +actual** and **38 vs 1162**. Expression indexes carry their own statistics, `CREATE INDEX` does not +populate them, and autoanalyze had not yet crossed its change threshold. + +`ANALYZE accounter_schema.transactions; ANALYZE accounter_schema.documents;` corrected them to 1189 +and 1714. The current `16-plans-scope*.txt` are post-`ANALYZE` and are the real baseline. + +**Why this matters for the upgrade:** `pg_upgrade` does not preserve extended statistics, and these +expression statistics are in the same category. Skipping the post-upgrade `ANALYZE` puts production +straight back into the pre-`ANALYZE` state above. That step is load-bearing, not hygiene. + +### 2. Two of #4331's four indexes cannot be used by the application at all + +`idx_transactions_owner_effective_debit_date` and `idx_documents_owner_vat_report_date` are **never +chosen**. The `COALESCE(...)` predicate is demoted to a `Filter`, discarding 8,532 and 6,311 rows +per query. Note the chosen plan's cost is _identical_ for an eight-month and a one-month range +(1819.37 / 647.65 either way) — the predicate never becomes an index condition, so selectivity +cannot influence the choice. + +This is **not** a statistics problem (finding 1 fixed the estimates and the plan did not change) and +**not** a matching problem (the definitions and column types match exactly — all three columns are +`date`). + +**Cause: RLS.** Reproduced locally on 18.6 with an identical table, indexes and data, varying only +one thing: + +| Setup | Plan | +| ---------------------------------------------------------------------- | ------------------------------------------------------------------------------- | +| Superuser, `owner_id` qual written by hand | `Index Scan` on the **expression index**, COALESCE in `Index Cond` (cost 456) | +| Non-superuser, `owner_id` qual injected by an RLS policy under `FORCE` | `Index Scan` on the **owner_id index**, COALESCE demoted to `Filter` (cost 741) | + +Under RLS, Postgres will not evaluate a non-leakproof user qual before the security qual, so it +cannot be pushed into the index scan. `date >= const` is leakproof — which is exactly why #4331's +two _plain-column_ indexes (`idx_transactions_owner_event_date`, `idx_documents_owner_date`) work +fine. `CoalesceExpr` is treated as potentially leaky, so the whole predicate is demoted. + +**The upgrade does not fix this.** The reproduction was on 18.6. + +**Verified fix**, also reproduced locally: a `STORED` generated column makes the expression a plain +column, so the comparison is leakproof and indexable under RLS: + +```sql +ALTER TABLE … ADD COLUMN effective_debit_date date + GENERATED ALWAYS AS (COALESCE(debit_date_override, debit_date)) STORED; +CREATE INDEX … ON … (owner_id, effective_debit_date); +-- non-superuser under RLS then gets: +-- Index Cond: ((owner_id = ANY (...)) AND (effective_debit_date >= …) AND (… <= …)) +``` + +`STORED` is not optional and must be written explicitly: **from PG18 on, +`GENERATED ALWAYS AS (expr)` defaults to VIRTUAL**, and virtual generated columns cannot be indexed. +Provider queries would also need to reference the new column instead of the `COALESCE`, so this is a +migration plus server change — a separate PR, not upgrade work. Until then the two indexes are pure +write overhead. + +## What each section is guarding against + +| § | Guards against | +| ------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | Two separate things: whether the `pg_trgm` reindex is required (`datlocprovider` ≠ `'c'` ⇒ required — production is `'c'`, so it is not), **and** a glibc collation-version change, which would require reindexing every text index. See the section above. | +| 2 | `pg_upgrade` carrying an old extension version forward | +| 3 | An RLS gap: a table with `owner_id` and no policy is tenant data with no isolation | +| 4, 6 | Azure granting the first ADMIN-option user privileges over other roles post-upgrade | +| 5 | A view acquiring `security_invoker`, which would change how scope resolves | +| 7 | `get_current_business_scope()` being owned by a different role than its siblings | +| 8 | A policy silently changing shape | +| 9 | A `CONCURRENTLY` reindex that failed part-way, leaving an index the planner ignores | +| 10 | Expression indexes losing their statistics — `ANALYZE` is mandatory, not optional | +| 11, 12 | Data loss, and the storage headroom the upgrade needs | diff --git a/docs/operations/postgres-18-baselines/capture-plans.psql b/docs/operations/postgres-18-baselines/capture-plans.psql new file mode 100644 index 000000000..b11acc246 --- /dev/null +++ b/docs/operations/postgres-18-baselines/capture-plans.psql @@ -0,0 +1,200 @@ +-- Query plan baselines for the Postgres 16 -> 18 upgrade (lightweight variant). +-- +-- Named .psql, not .sql, on purpose: it contains psql meta-commands which +-- prettier-plugin-sql cannot parse, and `yarn prettier:check` runs over the whole repo. +-- +-- Captures EXPLAIN (ANALYZE, BUFFERS) for the queries where the PG18 risk actually +-- concentrates: +-- * the 4 scraper dedup queries -- B-tree skip-scan candidates on 18 +-- * the 4 date-range queries -- exercise the (owner_id, ) indexes added by #4331 +-- +-- Deliberately NOT captured: getChargesByFilters (charges.provider.ts:211-862). It is 650 +-- lines with ~40 parameters and cannot be transcribed faithfully by hand; capturing it needs +-- auto_explain driven through the UI. Accepted gap. +-- +-- MUST run as accounter_prod_user, NOT accounter_admin. accounter_admin has BYPASSRLS, so +-- plans captured as that role carry no owner_id predicate and are structurally different from +-- what production executes -- worthless as a baseline. +-- +-- Usage (note the two -v variables): +-- +-- printf 'accounter_prod_user password: '; read -rs PGPASSWORD; echo +-- docker exec -i -e PGPASSWORD="$PGPASSWORD" accounter-dev-db-1 \ +-- psql --host=$PROD_HOST \ +-- --username=accounter_prod_user --dbname=accounter_prod_db \ +-- -v owner=$OWNER_A \ +-- -v scope='{"$OWNER_A"}' \ +-- < docs/operations/postgres-18-baselines/capture-plans.psql \ +-- > docs/operations/postgres-18-baselines/16-plans-scope1.txt 2>&1 +-- unset PGPASSWORD +-- +-- Run it TWICE: once with a single-element scope, once with a multi-element scope +-- ('{"uuid-a","uuid-b"}'), writing 16-plans-scope1.txt and 16-plans-scopeN.txt. Scope +-- cardinality is invisible to the planner but changes which of the owner_id-leading composite +-- indexes get chosen, so both are needed. +-- +-- After the upgrade, re-run identically against the upgraded server -> 18-plans-*.txt, and diff. + +\set ON_ERROR_STOP off +\pset pager off +\timing off + +\echo '########################################################################' +\echo '# READ THE PROBE RESULT IN SECTION 2 BEFORE TRUSTING ANYTHING BELOW IT. #' +\echo '########################################################################' + +BEGIN; + +\echo '' +\echo '================ 1. session state ================' +-- The app sets these with set_config(..., true) == SET LOCAL, so this must be in a transaction. +-- app.current_business_id is MANDATORY: get_current_business_id() raises without it, and the +-- scope helper falls back to ARRAY[that] when the scope GUC is empty. +SET LOCAL app.current_business_id = :'owner'; +SET LOCAL app.current_business_scope = :'scope'; +SET LOCAL app.current_user_id = ''; +SET LOCAL app.auth_type = 'jwt'; +SET LOCAL statement_timeout = '120s'; +SET LOCAL application_name = 'accounter-server'; + +SELECT current_user, + accounter_schema.get_current_business_id() AS write_target, + accounter_schema.get_current_business_scope() AS read_scope, + array_length(accounter_schema.get_current_business_scope(), 1) AS scope_len; + +\echo '' +\echo '================ 2. PROBE: is RLS actually applied to me? ================' +\echo '-- EXPECTED: ERROR 42501, "query would be affected by row-level security policy".' +\echo '--' +\echo '-- The discriminator is whether that ERROR APPEARS AT ALL -- not whether a row comes' +\echo '-- back. A bypassing role against an empty table returns zero rows and no error, which' +\echo '-- looks deceptively similar to success. No error => RLS was NOT applied to you => every' +\echo '-- plan below is INVALID. Stop, reconnect as accounter_prod_user, and start again.' +SAVEPOINT rls_probe; +SET LOCAL row_security = off; +SELECT 1 AS should_not_reach_here FROM accounter_schema.charges LIMIT 1; +ROLLBACK TO SAVEPOINT rls_probe; + +\echo '' +\echo '================ 3. do the scraper tables have rows at all? ================' +\echo '-- Counts only, deliberately. An earlier version of this script printed sample rows,' +\echo '-- which wrote real bank account numbers, branch numbers and security identifiers into' +\echo '-- the capture file -- violating the redaction rule in README.md. Never print data here:' +\echo '-- the dedup queries below derive their own parameters from the tables.' +\echo '-- A zero count means that plan scans nothing; its SHAPE is still comparable, but its' +\echo '-- row estimates are meaningless.' +SELECT 'poalim_securities_transactions' AS tbl, count(*) FROM accounter_schema.poalim_securities_transactions +UNION ALL SELECT 'poalim_ils_account_transactions', count(*) FROM accounter_schema.poalim_ils_account_transactions +UNION ALL SELECT 'otsar_hahayal_foreign_account_transactions', count(*) FROM accounter_schema.otsar_hahayal_foreign_account_transactions +ORDER BY 1; + +\echo '================ 4. dedup query plans (PG18 skip-scan candidates) ================' + +\echo '---- 4a. foreign-securities.provider.ts:57-92 -- value_date supplied, trade_date skipped' +\echo '---- index: poalim_securities_transactions_dedup_uindex (owner_id, bank, branch, account, security, trade_date, value_date, ...)' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.poalim_securities_transactions +WHERE security = ANY (ARRAY(SELECT DISTINCT security + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND bank_number = ANY (ARRAY(SELECT DISTINCT bank_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND branch_number = ANY (ARRAY(SELECT DISTINCT branch_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND account_number = ANY (ARRAY(SELECT DISTINCT account_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND value_date = ANY (ARRAY(SELECT DISTINCT value_date + FROM accounter_schema.poalim_securities_transactions LIMIT 5)); + +\echo '---- 4b. poalim-scraper-ingestion.provider.ts:57-95 -- serial_number skipped' +\echo '---- index: poalim_ils_account_transactions_dedup_uindex (event_date, serial_number, account_number, branch_number)' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.poalim_ils_account_transactions +WHERE account_number = ANY (ARRAY(SELECT DISTINCT account_number + FROM accounter_schema.poalim_ils_account_transactions LIMIT 5)) + AND branch_number = ANY (ARRAY(SELECT DISTINCT branch_number + FROM accounter_schema.poalim_ils_account_transactions LIMIT 5)) + AND event_date = ANY (ARRAY(SELECT DISTINCT event_date + FROM accounter_schema.poalim_ils_account_transactions LIMIT 5)); + +\echo '---- 4c. poalim-scraper-ingestion.provider.ts:355-396 -- owner_id supplied only by RLS' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.poalim_securities_transactions +WHERE bank_number = ANY (ARRAY(SELECT DISTINCT bank_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND branch_number = ANY (ARRAY(SELECT DISTINCT branch_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND account_number = ANY (ARRAY(SELECT DISTINCT account_number + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND security = ANY (ARRAY(SELECT DISTINCT security + FROM accounter_schema.poalim_securities_transactions LIMIT 5)) + AND trade_date = ANY (ARRAY(SELECT DISTINCT trade_date + FROM accounter_schema.poalim_securities_transactions LIMIT 5)); + +\echo '---- 4d. otsar-hahayal-scraper-ingestion.provider.ts:137-159 -- value_date skipped, reference supplied' +\echo '---- index: otsar_hahayal_foreign_account_transactions_conflict_key (account, branch, date, value_date, reference, description)' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.otsar_hahayal_foreign_account_transactions +WHERE account = ANY (ARRAY(SELECT DISTINCT account + FROM accounter_schema.otsar_hahayal_foreign_account_transactions LIMIT 5)) + AND branch = ANY (ARRAY(SELECT DISTINCT branch + FROM accounter_schema.otsar_hahayal_foreign_account_transactions LIMIT 5)) + AND date = ANY (ARRAY(SELECT DISTINCT date + FROM accounter_schema.otsar_hahayal_foreign_account_transactions LIMIT 5)) + AND reference = ANY (ARRAY(SELECT DISTINCT reference + FROM accounter_schema.otsar_hahayal_foreign_account_transactions LIMIT 5)); + +\echo '' +\echo '================ 5. date-range plans -- the four indexes added by #4331 ================' +\echo '-- These are the reduced forms of the real provider queries: the ($gate = 0 OR ...) and' +\echo '-- ($x ::TEXT IS NULL OR ...) guards constant-fold away when the other filters are off,' +\echo '-- so this is what the planner effectively sees for a date-only filter.' + +\echo '---- 5a. idx_transactions_owner_event_date -- transactions.provider.ts:37-49' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.transactions +WHERE event_date >= '2026-01-01'::DATE + AND event_date <= '2026-08-31'::DATE +ORDER BY event_date DESC; + +\echo '---- 5b. idx_transactions_owner_effective_debit_date' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.transactions +WHERE COALESCE(debit_date_override, debit_date) >= '2026-01-01'::DATE + AND COALESCE(debit_date_override, debit_date) <= '2026-08-31'::DATE +ORDER BY event_date DESC; + +\echo '---- 5c. idx_documents_owner_date -- documents.provider.ts:237-277' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.documents +WHERE date >= '2026-01-01'::DATE + AND date <= '2026-08-31'::DATE +ORDER BY created_at DESC; + +\echo '---- 5d. idx_documents_owner_vat_report_date -- documents.provider.ts:226-235' +EXPLAIN (ANALYZE, BUFFERS) +SELECT * +FROM accounter_schema.documents +WHERE COALESCE(vat_report_date_override, date) >= '2026-01-01'::DATE + AND COALESCE(vat_report_date_override, date) <= '2026-08-31'::DATE +ORDER BY created_at DESC; + +\echo '' +\echo '================ 6. what to check in every plan above ================' +\echo '-- 1. An owner_id clause is PRESENT, e.g.' +\echo '-- Index Cond: (owner_id = ANY (...)) or Filter: (owner_id = ANY (...))' +\echo '-- No owner_id clause means RLS was bypassed and the capture is invalid.' +\echo '-- 2. Which index was chosen, and whether it was an Index Scan / Bitmap / Seq Scan.' +\echo '-- 3. Estimated vs actual rows -- a large divergence is what moves plans on 18.' + +ROLLBACK; + +\echo '' +\echo '================ end ================' +SELECT now() AS captured_at_utc, version(); diff --git a/docs/operations/postgres-18-baselines/pre-upgrade-catalog.psql b/docs/operations/postgres-18-baselines/pre-upgrade-catalog.psql new file mode 100644 index 000000000..5f522eb27 --- /dev/null +++ b/docs/operations/postgres-18-baselines/pre-upgrade-catalog.psql @@ -0,0 +1,159 @@ +-- Pre-upgrade catalog snapshot for the Postgres 16 -> 18 upgrade. +-- +-- Named .psql, not .sql, on purpose: it contains psql meta-commands (\pset, \echo) which +-- prettier-plugin-sql cannot parse, and `yarn prettier:check` runs over the whole repo. +-- +-- Captures the "before" state of every fact the post-upgrade checks have to re-assert. +-- Contains NO tenant data -- catalog metadata only -- so the output is safe to commit as-is. +-- +-- Run as a role that can read the catalogs; accounter_admin is fine. Example: +-- +-- printf 'accounter_admin password: '; read -rs PGPASSWORD; echo +-- docker exec -i -e PGPASSWORD="$PGPASSWORD" accounter-dev-db-1 \ +-- psql --host=$PROD_HOST \ +-- --username=accounter_admin --dbname=accounter_prod_db \ +-- < docs/operations/postgres-18-baselines/pre-upgrade-catalog.psql \ +-- > docs/operations/postgres-18-baselines/16-catalog-snapshot.txt 2>&1 +-- unset PGPASSWORD +-- +-- Re-run the identical script against the upgraded server and diff the two files. + +\pset pager off +\pset footer off +\timing off + +\echo '================ 1. server version and collation provider ================' +-- datlocprovider decides whether the pg_trgm / FTS reindex is REQUIRED after pg_upgrade: +-- 'c' = libc -> not required (still cheap insurance) +-- 'i' = ICU -> REQUIRED +-- 'b' = builtin -> REQUIRED +SELECT version(); +SELECT datname, datlocprovider, datcollate, datctype, datcollversion +FROM pg_database WHERE datname = current_database(); + +\echo '================ 2. extensions and versions ================' +-- pg_upgrade carries old extension versions forward; Azure auto-upgrades most. +SELECT e.extname, e.extversion, n.nspname AS schema +FROM pg_extension e JOIN pg_namespace n ON n.oid = e.extnamespace +ORDER BY e.extname; + +\echo '================ 3. RLS coverage summary ================' +SELECT count(*) AS tables, + count(*) FILTER (WHERE relrowsecurity) AS rls_enabled, + count(*) FILTER (WHERE relrowsecurity AND relforcerowsecurity) AS rls_forced +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind = 'r'; + +\echo '---- tables WITHOUT RLS. has_owner_id = true means tenant data with no policy ----' +SELECT c.relname, + pg_get_userbyid(c.relowner) AS owner, + EXISTS (SELECT 1 FROM pg_attribute a + WHERE a.attrelid = c.oid AND a.attname = 'owner_id' AND a.attnum > 0 + AND NOT a.attisdropped) AS has_owner_id +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind = 'r' AND NOT c.relrowsecurity +ORDER BY has_owner_id DESC, c.relname; + +\echo '---- tables WITH RLS enabled but NOT forced (owner escapes its own policy) ----' +SELECT c.relname, pg_get_userbyid(c.relowner) AS owner +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind = 'r' + AND c.relrowsecurity AND NOT c.relforcerowsecurity +ORDER BY c.relname; + +\echo '================ 4. table ownership distribution ================' +SELECT pg_get_userbyid(c.relowner) AS owner, c.relkind, count(*) +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind IN ('r','v','m') +GROUP BY 1, 2 ORDER BY 1, 2; + +\echo '================ 5. views: security_invoker must stay UNSET ================' +SELECT c.relname, pg_get_userbyid(c.relowner) AS owner, c.reloptions +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind = 'v' +ORDER BY c.relname; + +\echo '================ 6. roles and attributes ================' +SELECT rolname, rolsuper, rolbypassrls, rolcreatedb, rolcreaterole, rolcanlogin, rolinherit +FROM pg_roles +WHERE rolname IN ('accounter_admin','accounter_prod_user','accounter_staging_user','prod_group') + OR rolsuper OR rolbypassrls +ORDER BY rolname; + +\echo '---- role memberships (INHERIT TRUE is why FORCE RLS is load-bearing) ----' +SELECT r.rolname AS member, g.rolname AS granted_role, m.admin_option, m.inherit_option +FROM pg_auth_members m +JOIN pg_roles r ON r.oid = m.member +JOIN pg_roles g ON g.oid = m.roleid +WHERE r.rolname IN ('accounter_admin','accounter_prod_user','accounter_staging_user','prod_group') +ORDER BY r.rolname, g.rolname; + +\echo '================ 7. RLS helper functions: owner + SECURITY DEFINER ================' +-- get_current_business_scope() is owned by a DIFFERENT role than its siblings, so it executes +-- with different privileges. Azure grants the first ADMIN-option user privileges over other +-- roles after a major upgrade, so this is the row to re-check afterwards. +SELECT p.proname, + pg_get_userbyid(p.proowner) AS owner, + p.prosecdef AS security_definer, + p.provolatile, + p.proconfig +FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace +WHERE n.nspname = 'accounter_schema' AND p.proname LIKE 'get_current_%' +ORDER BY p.proname; + +\echo '================ 8. tenant_isolation policies ================' +SELECT count(*) AS policy_count, polcmd, polpermissive +FROM pg_policy GROUP BY polcmd, polpermissive ORDER BY polcmd, polpermissive; + +\echo '---- representative policy definitions ----' +SELECT tablename, policyname, permissive, cmd, qual, with_check +FROM pg_policies +WHERE schemaname = 'accounter_schema' + AND tablename IN ('charges','transactions','documents','tags','financial_entities') +ORDER BY tablename, policyname; + +\echo '================ 9. trigram indexes and validity ================' +-- All must be reindexed CONCURRENTLY after the upgrade if datlocprovider is not 'c'. +SELECT n.nspname AS schema, c.relname AS index_name, t.relname AS table_name, + i.indisvalid, i.indisready, pg_size_pretty(pg_relation_size(c.oid)) AS size +FROM pg_index i +JOIN pg_class c ON c.oid = i.indexrelid +JOIN pg_class t ON t.oid = i.indrelid +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' + AND EXISTS (SELECT 1 FROM unnest(i.indclass::oid[]) AS ic(oid) + JOIN pg_opclass oc ON oc.oid = ic.oid + WHERE oc.opcname = ANY (ARRAY['gin_trgm_ops','gist_trgm_ops'])) +ORDER BY c.relname; + +\echo '---- any INVALID index anywhere in the schema (planner silently ignores these) ----' +SELECT c.relname, i.indisvalid, i.indisready +FROM pg_index i +JOIN pg_class c ON c.oid = i.indexrelid +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND NOT (i.indisvalid AND i.indisready) +ORDER BY c.relname; + +\echo '================ 10. expression indexes (own statistics, need ANALYZE) ================' +SELECT c.relname AS index_name, t.relname AS table_name, + pg_get_indexdef(i.indexrelid) AS definition +FROM pg_index i +JOIN pg_class c ON c.oid = i.indexrelid +JOIN pg_class t ON t.oid = i.indrelid +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' + AND 0 = ANY (i.indkey::int[]) -- 0 in indkey marks an expression column +ORDER BY t.relname, c.relname; + +\echo '================ 11. row counts for the main tenant tables ================' +SELECT 'charges' AS t, count(*) FROM accounter_schema.charges +UNION ALL SELECT 'transactions', count(*) FROM accounter_schema.transactions +UNION ALL SELECT 'documents', count(*) FROM accounter_schema.documents +UNION ALL SELECT 'ledger_records', count(*) FROM accounter_schema.ledger_records +ORDER BY 1; + +\echo '================ 12. database size ================' +SELECT pg_size_pretty(pg_database_size(current_database())) AS db_size; + +\echo '================ end of snapshot ================' +SELECT now() AS captured_at_utc; diff --git a/docs/operations/postgres-18-baselines/rehearsal-command-sheet.md b/docs/operations/postgres-18-baselines/rehearsal-command-sheet.md new file mode 100644 index 000000000..2c8d3a8b2 --- /dev/null +++ b/docs/operations/postgres-18-baselines/rehearsal-command-sheet.md @@ -0,0 +1,345 @@ +# Rehearsal command sheet — Step 4.3 + +Every check to run against the PITR-restored server, in order, with the expected result for each. +Parameterised by `$RESTORED_HOST`. Paste blocks in sequence. + +Companion to `../postgres-18-upgrade.md`. + +## ⚠️ Ordering rule — read this before anything else + +**`ANALYZE` must run BEFORE the plan captures.** Not after, not "sometime during". + +`pg_upgrade` does not preserve expression-index statistics. We already hit this on production: +before `ANALYZE`, the two `COALESCE` indexes estimated **51 rows vs 1671 actual** and **38 vs +1162**. The 16 baselines in this directory were captured _after_ `ANALYZE`. Capture the 18 plans +first and you will diff a post-`ANALYZE` 16 file against a pre-`ANALYZE` 18 file, see estimates +swing by 30×, and conclude PostgreSQL 18 changed the planner. It did not — you measured your own +missing statistics. + +## ⚠️ Paste hazard + +Do **not** paste a multi-line block containing `read`. zsh feeds the next pasted line into it as the +password. Step 0b is on its own for exactly this reason — paste it, press Enter, then type. + +--- + +## 0a. Variables + +```sh +cd /path/to/accounter-fullstack # your local checkout +BASE=docs/operations/postgres-18-baselines +RESTORED_HOST=$RESTORED_HOST # <-- set to the real hostname +A=$OWNER_A # owner for scope1 +B=$OWNER_B # second business for scopeN +ADMIN_CONN="host=$RESTORED_HOST user=accounter_admin dbname=accounter_prod_db sslmode=require" +APP_CONN="host=$RESTORED_HOST user=accounter_prod_user dbname=accounter_prod_db sslmode=require" +``` + +PITR preserves roles and passwords, so both accounts work on the restored server unchanged. + +## 0b. Passwords — paste each line alone, then type + +```sh +printf 'accounter_admin password: ' +read -rs PGADMIN +echo +echo "captured ${#PGADMIN} chars" +``` + +```sh +printf 'accounter_prod_user password: ' +read -rs PGAPP +echo +echo "captured ${#PGAPP} chars" +``` + +Both must report a non-zero, plausible length. If either reads `0`, the paste ate it — redo. + +## 0c. Confirm the restored server before upgrading it + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" -c " + SELECT version(); + SELECT datname, datlocprovider, datcollate, datcollversion, + pg_database_collation_actual_version(oid) AS actual_version + FROM pg_database WHERE datname = current_database();" +``` + +**Expect:** PostgreSQL **16.x** (PITR restores the same major version — this is correct), +`datlocprovider = c`, `datcollversion = 2.38`, `actual_version = 2.38`. + +If it already reports 18, you are pointed at the wrong server. Stop. + +## 0d. Restart the restored server — REQUIRED before the upgrade + +A PITR-restored server comes up with `max_connections` staged pending restart, and Azure refuses a +major version upgrade while any parameter is pending. Observed 2026-09-01: the upgrade failed +`MajorVersionUpgradeFailedPrecheck` for exactly this reason. Production was clean, so this is a +restore artifact — expect it on every restored server. + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" \ + -c "SELECT name, setting, source, pending_restart FROM pg_settings WHERE pending_restart;" +``` + +Any row → portal → `$RESTORED_HOST` → **Overview** → **Restart**. Wait for `Ready`, then re-run the +query and confirm it returns **0 rows** before continuing. + +--- + +## 1. Upgrade the restored server — portal + +Portal → `$RESTORED_HOST` → **Overview** → **Upgrade** → version **18** → Action **Validate and +upgrade** → **Start**. + +Record: + +- **Start (UTC):** `16:48:34` (2026-09-01) +- **Duration:** **17m 54s** ← the production estimate +- **Minor version landed on:** **18.6** + +The first attempt at 19:37:05 IDT failed at precheck on the pending `max_connections` (step 0d) and +did no work, so it does not count toward the estimate. + +**Budget the window, not just this number:** ~18 min upgrade + `ANALYZE` + any collation reindex + +the deploy freeze either side. Plan 30–40 minutes of unavailability, not 18. + +Wait for status `Ready` before continuing. + +--- + +## 2. Collation version — the check that can only be done here + +Not rehearsable on the local container: `postgres:18-alpine` is musl and reports no collation +version at all. + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" -c " + SELECT datname, datcollate, datcollversion, + pg_database_collation_actual_version(oid) AS actual_version + FROM pg_database WHERE datname = current_database(); + SELECT collname, collversion, pg_collation_actual_version(oid) AS actual + FROM pg_collation + WHERE collversion IS NOT NULL AND collversion <> pg_collation_actual_version(oid);" +``` + +**Expect:** `datcollversion = 2.38 = actual_version`, and an **empty** second result. + +**If they differ** — the upgrade moved the server to a different glibc, and every index whose +ordering depends on collation is potentially mis-sorted: all btree indexes on `text`/`varchar`, +unique constraints on text, any string comparison. Remedy, **in this order**: + +```sql +REINDEX DATABASE accounter_prod_db; -- not CONCURRENTLY-capable at DB scope +ALTER DATABASE accounter_prod_db REFRESH COLLATION VERSION; -- ONLY after the reindex +``` + +Refreshing first clears the warning while leaving the indexes wrong. Also check **Monitoring → +Server logs** for `database "…" has a collation version mismatch`. + +--- + +## 3. ANALYZE — Azure-mandatory, and must precede the plan captures + +```sh +time docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" -c "ANALYZE;" +``` + +Seconds on 105 MB. Takes only `SHARE UPDATE EXCLUSIVE` — does not block reads or writes. + +**Record the duration** — this step is mandatory on production too, so it belongs in the window +budget. + +--- + +## 4. Catalog snapshot → diff against 16 + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" \ + < $BASE/pre-upgrade-catalog.psql > $BASE/18-catalog-snapshot.txt 2>&1 + +diff -u $BASE/16-catalog-snapshot.txt $BASE/18-catalog-snapshot.txt +``` + +**Expected differences only:** `version()`, extension versions, index sizes, row counts, +`captured_at_utc`. + +**Anything else is a finding** — in particular RLS coverage (must stay 101 tables / 60 enabled / 60 +forced), the tables-without-RLS list, `FORCE` flags, view `reloptions` (`security_invoker` must stay +unset), role attributes and memberships, function ownership, and `datcollversion`. + +`salaries` may have moved into the RLS-enabled set if PR #4340 has deployed — an intended change, +not a finding. + +## 5. Plan captures → diff against 16 + +```sh +run() { + docker exec -i -e PGPASSWORD="$PGAPP" accounter-dev-db-1 \ + psql "$APP_CONN" -v owner=$A -v scope="$1" \ + < $BASE/capture-plans.psql > "$2" 2>&1 + if grep -qiE 'FATAL|password authentication failed' "$2"; then + echo " ✗ $2 — connection failed, deleting" + rm -f "$2" + elif ! grep -q 'row-level security policy' "$2"; then + echo " ✗ $2 — RLS probe did NOT fire: capture INVALID" + else + echo " ✓ $2 — $(wc -l < "$2") lines, RLS probe fired" + fi +} + +run "{\"$A\"}" $BASE/18-plans-scope1.txt +run "{\"$A\",\"$B\"}" $BASE/18-plans-scopeN.txt + +diff -u $BASE/16-plans-scope1.txt $BASE/18-plans-scope1.txt +diff -u $BASE/16-plans-scopeN.txt $BASE/18-plans-scopeN.txt +``` + +Two `✓` lines required. The RLS probe must fire — a capture taken by a bypassing role carries no +`owner_id` predicate and is worthless. + +**Reading the diff:** + +| Observation | Meaning | +| ------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------ | +| Same index chosen, similar estimates | No plan change. The expected outcome. | +| A **dedup query** switches to a skip scan on 18 | The PG18 improvement landing. Good — record it. | +| `idx_transactions_owner_event_date` / `idx_documents_owner_date` **stop** being used | **Regression.** Investigate before the production window. | +| The two `COALESCE` queries still show `Filter` + `Rows Removed` | **Expected, not a regression** — issue #4341. RLS demotes the predicate on 18 too; verified locally on 18.6. | +| Estimates wildly off again | `ANALYZE` did not run, or ran after the capture. Redo in the right order. | + +Timings are not comparable across runs (cold vs warm cache). Compare **plan shape and estimates**. + +## 6. Extension version + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" \ + -c "SELECT extname, extversion FROM pg_extension ORDER BY extname;" +``` + +Baseline was `pg_trgm 1.6` in `public`. Azure auto-upgrades most extensions, so expect it current. +Run `ALTER EXTENSION pg_trgm UPDATE;` only if it is behind what 18 ships — idempotent either way. + +## 7. Trigram reindex — optional here, rehearse it anyway + +`datlocprovider = 'c'` (libc), so this is **not required**. Worth running once to prove the tooling +works and to time it, rather than discovering both under pressure. + +```sh +ALLOW_REMOTE_DB=1 POSTGRES_HOST="$RESTORED_HOST" POSTGRES_DB=accounter_prod_db \ + POSTGRES_USER=accounter_admin POSTGRES_PASSWORD="$PGADMIN" POSTGRES_SSL=1 \ + yarn db:reindex-trgm --dry-run + +ALLOW_REMOTE_DB=1 POSTGRES_HOST="$RESTORED_HOST" POSTGRES_DB=accounter_prod_db \ + POSTGRES_USER=accounter_admin POSTGRES_PASSWORD="$PGADMIN" POSTGRES_SSL=1 \ + yarn db:reindex-trgm --confirm +``` + +**Expect:** 7 indexes discovered, collation provider reported as libc, all rebuilt `CONCURRENTLY`, +closing with "all valid". The tool is uncommitted — run from **this working tree**. + +## 8. RLS invariants — the suite + +```sh +ALLOW_REMOTE_DB=1 POSTGRES_HOST="$RESTORED_HOST" POSTGRES_DB=accounter_prod_db \ + POSTGRES_USER=accounter_admin POSTGRES_PASSWORD="$PGADMIN" POSTGRES_SSL=1 \ + yarn vitest run --project unit packages/migrations/src/__tests__/rls-all-tables.test.ts +``` + +Needs `CREATEDB`, hence `accounter_admin` and not the app role. It creates +`accounter_migration_test_rls_`, migrates into it, and drops it. **Confirm the throwaway +database is gone:** + +```sh +docker exec -i -e PGPASSWORD="$PGADMIN" accounter-dev-db-1 psql "$ADMIN_CONN" \ + -c "SELECT datname FROM pg_database WHERE datname LIKE 'accounter_migration_test%';" +``` + +Empty result expected. + +**Azure-specific:** after a major upgrade the first ADMIN-option user gains privileges over other +roles. Step 4's catalog diff covers `pg_roles` and memberships — read that section deliberately +rather than skimming it. + +## 9. pgTyped drift + +```sh +ALLOW_REMOTE_DB=1 POSTGRES_HOST="$RESTORED_HOST" POSTGRES_DB=accounter_prod_db \ + POSTGRES_USER=accounter_admin POSTGRES_PASSWORD="$PGADMIN" POSTGRES_SSL=1 \ + yarn generate:sql + +find packages -path '*__generated__*' -name '*.types.ts' \ + -not -path '*/node_modules/*' -not -path '*/dist/*' | sort | xargs shasum | shasum +``` + +**Do not compare this hash to a baseline taken from a different schema.** pgTyped introspects the +live database, so the hash depends on the _schema_ as much as the version — and production's schema +has drifted from what the migrations build (see below). Comparing a production-schema hash to a +migration-built baseline conflates two variables and produces a false alarm. That is exactly what +happened on 2026-09-02. + +Reference values for this tree: + +| Source | Hash | +| ----------------------------------------- | ------------------------------------------ | +| Migration-built schema, PG16 **and** PG18 | `841666e6194a83d7e3fd2f29341ea585accc598b` | +| Production schema, PG16 **and** PG18 | `a5de15bc891f52ed824be15bbc4d67332443c02a` | + +**Version-independence is proven in both directions** — 16 and 18 agree on each schema. So a +mismatch here means the _schema_ differs, not that PG18 changed type inference. + +To test version drift properly, hold the schema constant: restore the production dump schema-only +into a throwaway PG16 (use the **18** client — a pg_dump 18 archive is format 1.16, which pg_restore +16 cannot read) and generate against both. + +**Then regenerate against local**, so the working tree is not left holding types derived from a +temporary server: + +```sh +POSTGRES_HOST=localhost POSTGRES_PORT=5432 POSTGRES_DB=accounter \ + POSTGRES_USER=postgres POSTGRES_PASSWORD=postgres POSTGRES_SSL=0 yarn generate:sql +``` + +## 10. The two things no catalog query covers + +- **A real free-text search from the UI**, pointed at the restored server. Exercises + `getChargesByFilters` and all seven trigram indexes — the one query shape with no captured + baseline. Use a **synthetic** search term, not a real customer name. +- **One full scrape-ingestion batch.** Exercises the four dedup queries and the `ON CONFLICT` paths. + +Neither is scriptable from here, and together they are the difference between "the catalogs look +right" and "the application works". + +--- + +## 11. Delete the rehearsal server + +Portal → `$RESTORED_HOST` → **Overview** → **Delete**. + +Leaving it running means an unmonitored full copy of production accounting data, billing at +production rates. Do this as soon as the record sheet is filled in. + +```sh +unset PGADMIN PGAPP +``` + +--- + +## Record sheet + +| Item | Value | +| ------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Upgrade duration | **17m 54s** (19:48:34 → ~20:06 IDT, 2026-09-01) | +| Minor version landed on | **18.6** — build string identical to 16.15 (gcc 13.2.0), i.e. same base image | +| `ANALYZE` duration | **6.3s** — negligible; safe to run inside the window | +| `datcollversion` after upgrade | **2.38, matches actual_version — no drift.** No broad reindex needed | +| Catalog diff — unexpected differences | **NONE.** All 7 security-relevant sections byte-identical (RLS coverage, ownership, views/`security_invoker`, roles, memberships, `get_current_*`, policies). Only version, index sizes, row counts, db size, timestamp changed | +| Plan diff — changes found | **Skip scan confirmed on 4a and 4c** — `dedup_uindex` now used with `value_date` in Index Cond after unconstrained `trade_date`; Rows Removed by Filter 262→0 and 261→0, cost 229.75→185. 4b/4d unchanged. Section 5 plans identical; the two #4331 COALESCE indexes still demoted to Filter on 18 (issue #4341 confirmed with production data) | +| Trigram reindex duration (7 indexes) | **Dry run only** — 7 discovered, libc reported, correct CONCURRENTLY statements. Not required (`datlocprovider = c`) so `--confirm` skipped. First real-world validation of the tool against Azure 18.6 | +| `rls-all-tables` result | **PASS against the rehearsal server.** Target confirmed via the guard error naming `accounter_admin@$RESTORED_HOST…`, and `dotenv` proven not to override inline env | +| pgTyped hash matched | **PASS — no PG18 drift.** Prod schema on 16 and 18 both give `a5de15bc…`; migration-built schema on both gives `841666e6…`. The initial mismatch was schema drift, not version | +| UI free-text search | **PASS** | +| Scrape-ingestion batch | **PASS** | +| Rehearsal server deleted | pending — do it now | + +Fill this in **before** deleting the server — several rows are unrecoverable afterwards. diff --git a/docs/operations/postgres-18-upgrade.md b/docs/operations/postgres-18-upgrade.md new file mode 100644 index 000000000..f875f9472 --- /dev/null +++ b/docs/operations/postgres-18-upgrade.md @@ -0,0 +1,588 @@ +# Postgres 16 → 18 Upgrade — Runbook + +Production runs **Azure Database for PostgreSQL — Flexible Server**. Azure performs the upgrade +itself with `pg_upgrade` behind an in-place major-version-upgrade button, and its docs cover the +mechanics well. This runbook is the part Azure's docs do not cover: the checks that are specific to +_this_ schema, and the two invariants that exist only in prose and would fail silently. + +The SQL in this repo needs **no changes** to run on 18 — the incompatibility-by-incompatibility +audit is in [`docs/postgresql-v18-migration.md`](../postgresql-v18-migration.md). Dev and CI are +already pinned to 18 (#4329), so the version skew currently runs in the safe direction: queries are +validated against a newer parser and planner than production. Do not let production overtake them. + +## ✅ EXECUTED — production upgraded 2026-09-02 + +Production is on **PostgreSQL 18.6**. This runbook is now a record as well as a procedure; the +sections below were followed as written except where noted. + +| | | +| ---------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Rehearsal (PITR-restored copy) | 17m 54s, all 11 checks passed | +| **Production upgrade** | 16.14 → **18.6** | +| `ANALYZE` after upgrade | **5.2s** — mandatory, ran immediately | +| `datcollversion` | **2.38 → 2.38, no drift.** Same base image (gcc 13.2.0 both). No text-index reindex needed | +| Trigram reindex | **Not required** (`datlocprovider = 'c'`), not run | +| Catalog diff (same server, before/after) | **All 7 security-relevant sections byte-identical.** Azure's ADMIN-option privilege change altered nothing — RLS coverage 101/60/60, ownership, `security_invoker` unset, role attributes, `prod_group → accounter_prod_user INHERIT TRUE`, `get_current_*` ownership, policies | +| Plan diff | **No regressions.** Skip scan now live on the two securities dedup queries: `dedup_uindex` chosen, `value_date` indexed past unconstrained `trade_date`, `Rows Removed by Filter` 262/261 → **0** | +| Trigram index sizes | **Roughly halved** by the upgrade (e.g. 3416 → 1352 kB); DB 105 → 99 MB despite two days more data. All indexes valid. Mechanism not established | +| UI free-text search, scrape batch | Both verified on production | +| Deploys | Frozen for the window, unfrozen after | + +**Three things the rehearsal caught that would have hit the window:** a pending-restart +`max_connections` failing the precheck (a PITR-restore artifact — production was clean), restored +servers arriving with **no firewall rules**, and the `ANALYZE`-before-plan-captures ordering trap. + +**Still open:** issue #4341 (two indexes unusable under RLS — unaffected by the upgrade, confirmed +on 18), and the production/migration schema drift documented in the baselines README. + +## The failure modes this guards against + +Ordered by how quietly they fail. + +1. **RLS silently stops isolating tenants.** The production authorization model is _not created by + any migration_. `prod_group` owns `tags` and `extended_tags`, `extended_charges` has a different + owner (`accounter_prod_user`), and `FORCE ROW LEVEL SECURITY` is load-bearing because a table + owner otherwise bypasses its own policies. Azure additionally changes role privileges across a + major upgrade: afterwards, the first user created on the server holding the ADMIN option has + administrative privileges over other roles. Nothing in dev or CI can warn you, because both + connect as `postgres` — a superuser with `BYPASSRLS` (see + `packages/server/src/__tests__/helpers/rls-role.ts`). A regression here is a cross-tenant data + leak that returns HTTP 200. +2. **Free-text search degrades or changes behaviour.** PG18 changed full-text search and `pg_trgm` + to use the cluster's default collation provider instead of always libc. The entire + charges/transactions/documents/counterparty search is `ILIKE '%…%'` served by seven GIN trigram + indexes — `charges.provider.ts:213` states the strategy outright ("Identify IDs via Trigram + indexes before doing any heavy math"). Wrong or stale indexes here mean wrong search results, not + just slow ones. +3. **Plans shift everywhere at once.** Every query carries an implicit + `owner_id = ANY (accounter_schema.get_current_business_scope())` from RLS + (`2026-05-25T10-00-00.rls-multi-business-scope.ts:108`), and the planner cannot see that array's + length. Any change in `ScalarArrayOpExpr` selectivity moves plans across the whole application + simultaneously rather than query by query. #4331 raised the stakes: its four new indexes all + _lead_ with `owner_id`. +4. **A deploy lands mid-upgrade.** The migration runner takes **no advisory lock** + (`packages/migrations/src/pg-migrator.ts`). If migrations run on deploy, a deploy that starts + while the database is unavailable fails partway through them. +5. **Regret after a successful upgrade.** There is no revert. See [Rollback](#rollback) — it is + weaker than it looks, and knowing that _before_ the window is the point. + +## Before the window + +### Step 0 — run Azure's Upgrade Validation Checks + +Do this first, and days early. Flexible Server has a first-class pre-flight check that runs Azure's +real compatibility rules against the actual server without touching it — no downtime, no restart, no +version change: + +```sh +# set RG and SERVER for your environment first +az postgres flexible-server upgrade-validate \ + --resource-group "$RG" --name "$SERVER" --version 18 +``` + +It is authoritative in a way this document cannot be, because the blocking rules vary by +source/target pair and change over time. **Treat a clean validation run, not this checklist, as the +go/no-go.** Requirements: server status `Ready`, no other operation in progress, connectivity to +every database on the server; it cannot run against a read replica. + +Our schema is expected to come back clean. Verified against Azure's blocker list on current `main`: + +| Azure blocker | This schema | +| ----------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | +| Blocked/utility extensions | `pg_trgm` only (`2026-03-23T12-00-00.index-search-strings.ts:7`) — on none of Azure's lists | +| `EVENT TRIGGER`s | None. (The 20 `AFTER` triggers are ordinary row/statement triggers and are unaffected.) | +| Views depending on `pg_stat_activity` | None — the only two references are a comment (`server/src/index.ts:40`) and a `pg_terminate_backend` call in a test | +| Objects depending on `pg_stat_statements` | None; no monitoring-catalog reads in app code | +| Large objects (`pg_largeobject`) | None | +| PostGIS / TimescaleDB | Not used | + +### Environment facts — confirmed on the portal 2026-09-01 + +Several generic prerequisites below do **not apply to this server**. Checked rather than assumed: + +| Fact | Value | Consequence | +| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Compute tier | **Burstable, 1 vCore** (B1ms), West Europe, availability zone 2 | See CPU credits below | +| Storage | 32 GB provisioned, **105 MB used (~0.3%)** | ≥10–20% free requirement trivially met — **no action** | +| Read replicas | **None.** The single row on the Replication blade is the primary itself (Role = `Primary`) | No delete/re-create cycle — **the item that could have added days is gone** | +| High availability | **Disabled**, status `Not enabled`. Portal: _"Enabling zone redundant high availability is not supported with the chosen compute tier"_ | No HA disable/re-enable cycle; **no spare-standby capacity requirement**; and the NSG ports 5432/6432 + outbound-Azure-Storage concern **does not apply** | +| CPU credits | **288 / 288**, flat at the cap for 18+ hours | Full burst bank (~288 vCore-minutes). Not a constraint; no need to match credit state between rehearsal and production | +| Entra authentication | Enabled | Verify after the upgrade alongside the RLS role checks — Azure's ADMIN-option change interacts with role membership | +| Collation | `datlocprovider = 'c'` (libc), `datcollate = en_US.utf8`, `datcollversion = 2.38` | Trigram reindex **not required**, only prudent. But see § 1b — a `datcollversion` change is a finding | + +**What having no HA costs you:** the SLA is 99.9% rather than 99.99%, and there is no standby to +fall back on. That does not change the upgrade itself — an in-place major version upgrade takes the +server down regardless of HA — but it means Azure's implicit pre-upgrade backup is the _only_ safety +net if the upgrade fails mid-flight. Budget accordingly; do not treat "HA is off" as making the +window lower-risk. + +**A correction on parallelism:** PG18's parallel GIN build (`enable_parallel_gin_build`, +`max_parallel_maintenance_workers`) is cited elsewhere in this document as making a reindex faster. +On **1 vCore** there is no parallelism to exploit, so that reassurance does not apply here. Moot in +practice, since the reindex is not required. + +### ⛔ Pending-restart parameters block the upgrade — check this first + +Hit for real on the rehearsal server, 2026-09-01: + +```json +{ + "code": "MajorVersionUpgradeFailedPrecheck", + "message": "The major version upgrade failed precheck. Upgrading with pg_settings pending restart + is not allowed: found setting with name max_connections and current value 100." +} +``` + +Azure refuses a major version upgrade while **any** server parameter is staged awaiting a restart. + +**Resolved 2026-09-01: this is an artifact of the PITR restore, not a property of production.** +Checked both servers with the query below — production returned **0 rows**; the restored server had +`max_connections = 100`, `source = configuration file`, `pending_restart = t`. So PITR provisioning +stages the parameter and never restarts, and a freshly restored server cannot be upgraded until it +is. Production would not have failed this precheck. + +Check it anyway before the window: a parameter can be staged at any time by anyone editing the +portal, and nothing in the application surfaces it. A validation run from days earlier is stale +evidence. + +```sql +SELECT name, setting, unit, source, pending_restart +FROM pg_settings +WHERE pending_restart; +``` + +An empty result means nothing is staged and the precheck will not trip on this. Any row is a +blocker. The portal equivalent is **Settings → Server parameters**, which flags staged values. + +**The fix is a restart** (portal → Overview → **Restart**), which applies the staged value and +clears the flag. Two consequences for the production window: + +- A restart is **downtime on top of the upgrade downtime**. If production has a pending parameter, + budget for it and do the restart _ahead of_ the window rather than discovering it at the precheck. +- Nothing in the app warns you a parameter is staged. It can sit pending indefinitely after someone + edits a parameter in the portal and does not restart — which is precisely how this becomes a + window-day surprise. + +**Also note for the rollback path:** a PITR-restored server may come up with parameters staged +pending restart, so a recovery restore needs a restart before it is consistent — on top of +recreating its firewall rules (see Rollback). + +### Prerequisites + +| Check | Why it matters | +| ------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| **≥10–20% free storage** | Azure calls this out explicitly: temporary log files and metadata operations grow disk usage mid-upgrade, and insufficient space causes upgrade failure or rollback. Growing storage on Flexible Server is one-way — check early. | +| **Delete read replicas** | In-place upgrade supports neither geo-replication nor read replicas. Every replica, including cascading ones, must be deleted before upgrading the primary and re-created after. This is the one item that cannot be done inside a short window. | +| **HA capacity + NSG rules** | If HA is enabled, Azure disables it, upgrades the primary, then re-enables it — which needs spare capacity for a new standby. Confirm NSG rules permit ports **5432 and 6432** within the VNet and outbound to Azure Storage for log archiving; otherwise HA silently fails to re-enable and you finish the window with no standby. | +| **Upgrade logs on** | Set `logfiles.download_enable = ON` and a sane `logfiles.retention_days`, then read `PG_Upgrade_Logs` during the run. This is the only real visibility into a stalled upgrade. | +| **Connection username format** | If this server was ever automigrated from Single Server, the `username@servername` login format stops working after an in-place major upgrade — only plain `username` is accepted. Our connection string is assembled from `POSTGRES_USER` (`packages/server/scripts/set-db-url.cjs`, `environment.ts:319`), so check that value across server, migrations, MCP server and scrapers. | +| **Minor version** | Nothing to choose: Azure automatically deploys the latest supported minor, so there is no 18.0 risk. **Record what you actually landed on.** For context: 18.1 fixed an OOM in parallel GIN index builds, 18.3 was an out-of-cycle release fixing regressions including a standby freeze, 18.5 was never released, 18.6 (2026-08-13) is current. | + +### Freeze Render deploys for the window + +**The architecture is split, which is why two vendors appear in this document: the backend is +deployed on Render, the Postgres database is on Azure.** Confirmed 2026-08-31. + +Production migrations **do** run inside the Render build command, against the Azure database. The +command as of 2026-08-31: + +```sh +yarn install --immutable && npx playwright install && yarn db:migrate && yarn server:build:prod +``` + +- **Freeze Render deploys for the whole window.** The migration runner takes **no advisory lock** + (`pg-migrator.ts`) and the database is unavailable for part of the upgrade. +- **The `&&` chaining is load-bearing — check it is still there before the window.** It means a + deploy that lands mid-upgrade _aborts_: `yarn db:migrate` fails, `yarn server:build:prod` never + runs, and the previously deployed server keeps serving. Until 2026-08-31 these steps were joined + by `;`, under which a failed migration did **not** stop the deploy — the new server shipped with + migrations unapplied, silently. If anyone reverts that, the failure mode goes back to being + invisible. +- Aborting is much better than shipping, but it is not free: migrations are applied one at a time + with no transaction spanning them (and the two most recent use `noTransaction` for + `CREATE INDEX CONCURRENTLY`), so an interrupted run can leave the schema **partially migrated** + while the old code serves. Freeze anyway; do not rely on `&&` as a substitute. +- The upgrade itself needs no application config or code deploy: an Azure in-place upgrade retains + the server name, so Render's existing connection settings keep working untouched. +- Re-enable deploys only once the post-upgrade checks below have passed. + +### Guard the root `.env` first + +Tracked in-repo at `packages/mcp-server/docs/todo.md:116-124`: the root `.env` is what codegen, +migrations, seeds and DB-backed tests all read, so anything reading it targets production by +default. This window makes that materially more dangerous, because you will run codegen and +migrations more than usual and against a restored server — and +`packages/server/scripts/set-db-url.cjs` builds `DATABASE_URL` from whatever `POSTGRES_*` happens to +be in `.env`. One stale value and a rehearsal step runs against production. + +Point the root `.env` at local and keep production credentials in a separate, explicitly named file +**before** the window. + +This is not hypothetical. It bites in ordinary use, twice observed: + +- `scripts/vitest-global-setup.ts` runs before **every** vitest project, `--project unit` included. + It connects via the same `test-db-config.ts` (which loads `.env` / `../../.env`) and executes + `seedCountries(client)` — a write. "I only ran unit tests" is not a reason to skip an override. +- `packages/migrations/src/__tests__/rls-all-tables.test.ts` opens the `postgres` database on + whatever host `.env` names and issues `CREATE DATABASE`. Against production this fails only + because `accounter_prod_user` lacks `CREATEDB` — luck, not a safeguard. Had the role been + stronger, the next step was `runPGMigrations` against that server. + +During the window, `.env` is also the thing most likely to be edited repeatedly (local → restored +server → local), so its contents change under you. Re-check it in the same breath as the command: + +```sh +grep '^POSTGRES' .env # confirm the target, every time +POSTGRES_HOST=localhost POSTGRES_DB=accounter POSTGRES_USER=postgres POSTGRES_SSL=0 yarn test +``` + +A `.env` you verified an hour ago is not evidence about the `.env` you have now. + +**There is now a guard for this** (`packages/migrations/src/local-db-guard.ts`). The test harness, +the vitest global setup, the RLS suite and both seed scripts **refuse** a non-local host unless +`ALLOW_REMOTE_DB=1` is set for that command. `migration:run` only **warns** — deliberately, because +production deploys may apply migrations during the build and that path is still unconfirmed (see +above); export `ENFORCE_LOCAL_DB=1` in your shell profile to make it strict for you. + +The guard inspects the configured host only. A port-forward or tunnel presenting a deployed database +as `localhost` passes it. It raises the floor; it is not a sandbox — so keep checking `.env` too. + +## Rehearsal + +This is the whole risk-reduction strategy; do not skip it. + +1. **PITR-restore production to a new server.** Azure restores to a new server, never in place. +2. **Upgrade that server** with the same in-place upgrade you intend to run on production. +3. **Run every check** in [Post-upgrade checks](#post-upgrade-checks--ours-not-azures) against it, + plus two things no catalog query covers: a real free-text search from the UI, and one full + scrape-ingestion batch. +4. **Record the duration.** This is the production estimate — there is no better source for it. +5. **Delete the restored server** once you are done, so it does not linger as a stale copy of + production data. + +## Post-upgrade checks — ours, not Azure's + +Run these on the rehearsal server first, then again on production after the real upgrade. + +### 1. Collation provider, then the trigram reindex + +PG18 changed FTS and `pg_trgm` to use the cluster's default collation provider instead of always +libc. Upstream: clusters defaulting to a _non-libc_ provider "could observe changes in behavior of +some full-text search functions, as well as the `pg_trgm` extension", and should reindex FTS and +`pg_trgm` indexes after `pg_upgrade`. Check which case you are in: + +```sql +SELECT datname, datlocprovider, datcollate, datctype +FROM pg_database WHERE datname = current_database(); +``` + +**Measured on production 2026-08-31: `datlocprovider = 'c'` (libc), `datcollate = en_US.utf8`.** So +the trigram reindex is **not required** for this cluster — it drops to cheap insurance. Re-check +after the upgrade anyway rather than trusting this line. + +#### 1b. The collation _version_ — a different and much broader risk + +Do not confuse this with the provider question above. Separately from PG18's `pg_trgm` change, +Postgres records the version of the collation library the database was built against. If the upgrade +moves the server to a base image with a different glibc, that version changes, and **every index +whose ordering depends on collation is potentially mis-sorted** — all btree indexes on +`text`/`varchar`, unique constraints on text columns, and anything comparing strings. That is a far +bigger job than seven trigram indexes. + +Production was on glibc **2.38** before the upgrade (`datcollversion`, captured in the baseline +snapshot). Check both the database default and any non-default collations: + +```sql +-- database default: stored version vs what the OS now provides +SELECT datname, datcollate, datcollversion, + pg_database_collation_actual_version(oid) AS actual_version +FROM pg_database WHERE datname = current_database(); + +-- any individual collation whose stored version no longer matches reality +SELECT collname, collversion, pg_collation_actual_version(oid) AS actual_version +FROM pg_collation +WHERE collversion IS NOT NULL AND collversion <> pg_collation_actual_version(oid); +``` + +`datcollversion = actual_version` and an empty second result ⇒ nothing to do. **A mismatch, or a +`WARNING: database "…" has a collation version mismatch` in the logs, means:** + +1. `REINDEX DATABASE accounter_prod_db;` — or at minimum every btree index on a text column. This is + not `CONCURRENTLY`-capable at database scope, so plan it as downtime or reindex table by table + with `REINDEX INDEX CONCURRENTLY`. +2. Then `ALTER DATABASE accounter_prod_db REFRESH COLLATION VERSION;` to clear the warning — **only + after** the reindex. Refreshing first hides the problem without fixing it. + +Two caveats: + +- **This cannot be rehearsed on the local dev container.** `postgres:18-alpine` uses musl, which + reports no collation version at all (`datcollversion` comes back empty), whereas production is + glibc. Verify it on the PITR-restored rehearsal server, which is a real Azure instance. +- The rehearsal is the _only_ early warning you get. If the restored server's `actual_version` + differs from 2.38 after being upgraded to 18, expect the same on production and budget the reindex + into the window. + +`datlocprovider = 'i'` (ICU) or `'b'` (builtin) → reindex is **required**. `'c'` (libc) → not +required. Either way there are only seven indexes and this is cheap insurance: + +```sql +REINDEX INDEX CONCURRENTLY accounter_schema.idx_charges_desc_trgm; -- charges.user_description +REINDEX INDEX CONCURRENTLY accounter_schema.idx_trans_src_trgm; -- transactions.source_description +REINDEX INDEX CONCURRENTLY accounter_schema.idx_trans_src_ref_trgm; -- transactions.source_reference +REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_desc_trgm; -- documents.description +REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_remarks_trgm; -- documents.remarks +REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_serial_trgm; -- documents.serial_number +REINDEX INDEX CONCURRENTLY accounter_schema.idx_financial_entities_name_trgm; -- financial_entities.name +``` + +**Use the tool rather than the list.** `yarn db:reindex-trgm` discovers every trigram index from +`pg_index`/`pg_opclass` at runtime, always emits `CONCURRENTLY`, reports the collation provider, +flags invalid indexes and `_ccnew` leftovers from a previously failed rebuild, times each index, and +re-reads the catalog afterwards to confirm validity: + +```sh +yarn db:reindex-trgm --dry-run # list what would be rebuilt, change nothing +yarn db:reindex-trgm --confirm # rebuild +``` + +It takes its target from `POSTGRES_*` like everything else, so set those to the upgraded server +explicitly. The SQL below is the same work by hand, kept for when you would rather not run a script +against production — but note the list is a snapshot and the tool is not. + +Two things to get right here: + +- **All seven, including the last** — the reason to prefer the tool. + `idx_financial_entities_name_trgm` was added recently (#4330, + `2026-08-31T11-00-00.index-financial-entity-names.ts`) for the six counterparty-name `ILIKE` + branches of the charges free-text filter. It is as exposed to the collation change as the original + six. An older copy of this list has only six entries. +- **`CONCURRENTLY` regardless of how they were built.** Only the seventh was originally created + concurrently; the first six came from a plain `CREATE INDEX IF NOT EXISTS` + (`2026-03-23T12-00-00.index-search-strings.ts`). A plain `REINDEX` takes `ACCESS EXCLUSIVE` and + would block writes to `charges`, `transactions` and `documents` for the duration. + +PG18 adds parallel GIN builds (`enable_parallel_gin_build`, `max_parallel_maintenance_workers`), so +the rebuild is faster than it would have been on 16. + +Afterwards, confirm nothing was left invalid — a `CONCURRENTLY` build that fails part-way leaves an +index the planner silently ignores: + +```sql +SELECT c.relname, i.indisvalid, i.indisready +FROM pg_index i +JOIN pg_class c ON c.oid = i.indexrelid +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND NOT (i.indisvalid AND i.indisready); +``` + +### 2. Verify the extension version + +```sql +SELECT extname, extversion FROM pg_extension; +``` + +Azure auto-upgrades most extensions during an in-place major upgrade, so unlike a bare `pg_upgrade` +this should already be current. Run `ALTER EXTENSION pg_trgm UPDATE;` only if the version is behind +what 18 ships — it is idempotent and harmless either way. + +### 3. `ANALYZE` — required, not optional + +Azure documents this as a required post-upgrade step: run `ANALYZE` in each database to refresh +`pg_statistic`, because missing or stale statistics cause bad plans and excess memory use. + +```sql +ANALYZE; -- or: ANALYZE VERBOSE; scoped per table if you want progress +``` + +PG18's `pg_upgrade` retains optimizer statistics, but _extended_ statistics are explicitly not +preserved. There are no `CREATE STATISTICS` objects here, but there are six **expression** indexes +carrying their own statistics, and two are new: + +- `COALESCE(full_purchase_date, full_purchase_date_outbound)` — scraper dedup + (`2026-05-04T12-00-00.…:8-38`) +- `idx_transactions_owner_effective_debit_date` on + `(owner_id, COALESCE(debit_date_override, debit_date))` +- `idx_documents_owner_vat_report_date` on `(owner_id, COALESCE(vat_report_date_override, date))` + +The latter two arrived with #4331 (`2026-08-31T10-00-00.add-tenant-scoped-date-indexes.ts`) and sit +directly in the path of the date-range filters that PR made sargable. Stale statistics on them would +regress exactly the query shapes it was written to fix. + +### 4. Re-assert the RLS invariants + +The load-bearing check. These four queries are the ones already verified against production +(`docs/coherent-owner-scoping-for-mcp/plan.md:51-108`); re-run all four and compare against the +expected values, do not eyeball them: + +```sql +-- 1. view owner → prod_group +SELECT viewowner FROM pg_views +WHERE schemaname = 'accounter_schema' AND viewname = 'extended_tags'; + +-- 2. owner cannot bypass RLS → rolsuper = f, rolbypassrls = f +SELECT rolname, rolsuper, rolbypassrls FROM pg_roles +WHERE rolname IN ('prod_group', 'accounter_prod_user'); + +-- 3. RLS ENABLED *and FORCED* on the base table → tags: t / t +-- (also charges: t/t, tax_categories: t/t) +SELECT c.relname, pg_get_userbyid(c.relowner) AS owner, + c.relrowsecurity, c.relforcerowsecurity +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relname IN ('tags', 'extended_tags'); + +-- 4. policy resolves through a session GUC, not current_user +SELECT policyname, qual FROM pg_policies +WHERE schemaname = 'accounter_schema' AND tablename = 'tags'; +-- expect tenant_isolation: +-- owner_id = ANY (accounter_schema.get_current_business_scope()) +``` + +Why each one, briefly — the reasoning was learned by running them, not by reading the schema: + +- **Step 3 is the one that would fail quietly.** `prod_group` _owns_ `tags`, and a table owner + bypasses its own RLS policies unless `FORCE ROW LEVEL SECURITY` is set. Had `relforcerowsecurity` + been `f`, the view would escape RLS with `rolsuper` and `rolbypassrls` both still `f` — i.e. steps + 2 and 4 green and the data leaking anyway. +- **`extended_charges` does not imply `extended_tags`.** Different owners (`accounter_prod_user` vs + `prod_group`), so "the charges path works, therefore this works" does not transfer. Check each + view's own owner. +- **No view sets `security_invoker`,** which is harmless _only_ because scope resolves through a + session GUC rather than `current_user`: the policy evaluates identically whichever role executes + the view. If that ever changes, this assumption goes with it. + +Then confirm no view acquired `security_invoker` and no table lost RLS: + +```sql +SELECT c.relname, pg_get_userbyid(c.relowner) AS owner, c.reloptions -- security_invoker must stay unset +FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'accounter_schema' AND c.relkind = 'v'; +``` + +Finally, run the existing suite — it builds a genuine non-superuser role and checks every table: + +```sh +# set RESTORED_HOST and ADMIN_USER for the PITR-restored server first +ALLOW_REMOTE_DB=1 \ + POSTGRES_HOST="$RESTORED_HOST" POSTGRES_DB=accounter_prod_db \ + POSTGRES_USER="$ADMIN_USER" POSTGRES_SSL=1 \ + yarn vitest run --project unit packages/migrations/src/__tests__/rls-all-tables.test.ts +``` + +Three things about that command: + +- **`ALLOW_REMOTE_DB=1` is required.** The suite refuses a non-local host without it + (`packages/migrations/src/local-db-guard.ts`), because it `CREATE DATABASE`s and runs every + migration into the result. This is the one intended non-local use of that opt-in — type it + deliberately, in the same command as the restored server's hostname. +- **Target it explicitly.** Bare, it uses the root `.env` — see the `.env` warning above. It must + point at the restored server, not at production and not at local. +- **The role needs `CREATEDB`.** The test creates a throwaway database + (`accounter_migration_test_rls_`), runs migrations into it, and drops it. + `accounter_prod_user` cannot do this — use the server admin role, and confirm the throwaway + database was dropped afterwards. + +**Azure-specific:** after a major upgrade, the first user created on the server with the ADMIN +option holds administrative privileges over other roles. Re-check the `prod_group` / +`accounter_prod_user` split against that, not just against step 2's output. + +### 5. Capture plan baselines + +Capture `EXPLAIN (ANALYZE, BUFFERS)` for these on 16 **before** the window and again after +(`BUFFERS` is on by default in 18): + +| Query | Why | +| -------------------------------------------------------------------------- | ------------------------------------------------------- | +| `getChargesByFilters` (`charges.provider.ts:211`) | The trigram-first search strategy, and the widest query | +| `extended_charges` consumer (`accountant-approval.provider.ts:12-20`) | Big view, sequential/bitmap-scan shaped | +| Creditcard LATERAL self-join (`creditcard-transactions.provider.ts:10-27`) | Join-shape sensitive | +| The four Part 3.4 dedup queries | Skip-scan candidates on the scraper hot path | +| One date-filtered charges query **and** one date-filtered documents query | Exercises #4331's new `(owner_id, )` indexes | + +Capture the last row's baselines on **current `main`**, not a pre-#4331 checkout: a baseline taken +against the old `::TEXT::DATE` predicates measures a query shape that no longer exists. + +### 6. Regenerate pgTyped types against 18 + +```sh +yarn generate:sql +``` + +Codegen introspects a live database, so any type-inference difference surfaces here first. Expect +zero diff — this was already proven before #4329 merged, by generating against 16.10 and 18.6 and +comparing hashes over every `__generated__/*.types.ts`. Mind the `.env` warning above: this step +connects to whatever `POSTGRES_*` points at, and pgTyped is **not** behind the guard (it only reads, +so it was left unguarded) — making it the one step here with no safety net. + +## If something looks wrong afterwards + +| Symptom | First check | +| ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | +| Search returns fewer/odd results | `datlocprovider` and whether the seven trigram reindexes actually completed — plus the `indisvalid` query in check 1 | +| Search is slow but correct | An invalid index the planner is ignoring (same query), or missing `ANALYZE` | +| A tenant sees another tenant's rows | Stop. Check 4, step 3 (`relforcerowsecurity`) first, then the Azure ADMIN-option role change | +| Broad slowdown across unrelated queries | `ANALYZE` not run, or `ScalarArrayOpExpr` selectivity shift — compare against the check 5 baselines | +| Date-filtered charge/document lists slow | Whether the four `(owner_id, )` indexes from #4331 survived and are being chosen | +| App cannot connect at all | The `username@servername` format (see prerequisites), then NSG rules on 5432/6432 | +| No standby after the window | HA failed to re-enable — NSG rules or insufficient capacity | +| Connections pile up `idle in transaction` | Unrelated to the upgrade: see [`db-connection-pool.md`](db-connection-pool.md) | + +A corrected balance is **not** a regression: #4326 fixed `getLedgerBalanceToDate`, which had been +dropping credit entity 2 from every balance it computed. Figures that changed there changed because +of that fix, not the upgrade. + +## Rollback + +Be precise about this, because it is weaker than it looks. + +- **Failure _during_ the upgrade is well covered.** After a successful precheck and immediately + before starting, Azure takes an implicit backup and can use it to restore the instance to its + previous version on error. +- **Regret _after_ a successful upgrade is not.** There is **no automated way to revert**. Recovery + is a PITR restore to a timestamp before the upgrade, which lands on a **new server with a new + name** — a connection-string change and a real cutover, not a rollback. + +> ### ⚠️ A PITR-restored server has NO firewall rules +> +> Observed 2026-09-01 while creating the rehearsal server: **Networking → Firewall rules came back +> empty**, even though the source server has rules. The restore does not carry them over. +> +> This is a hole in the rollback path above, not a rehearsal curiosity. If you ever PITR-restore +> production as a recovery, the restored server is **unreachable by everything** — Render, the +> scrapers, the MCP server, your laptop — until its firewall rules (and any private-endpoint or VNet +> configuration) are recreated by hand. Under outage pressure that is exactly the step that gets +> missed, and it presents as "the restore worked but nothing can connect". +> +> **Before the window:** screenshot or export the primary's firewall rules and networking +> configuration so they can be reapplied without having to reconstruct them from memory. +> +> ```sh +> az postgres flexible-server firewall-rule list -g "$RG" -n "$SERVER" -o table +> ``` + +Two consequences worth acting on before the window: confirm the backup retention window covers how +long a regression might plausibly go unnoticed, and take a manual `pg_dump` — the repo contains no +backup, restore or replication tooling at all (`package.json`'s `seed:reset-staging` is an `echo` +placeholder), so Azure's own backups are the only copy. + +## Sign-off checklist + +- [ ] `az postgres flexible-server upgrade-validate` clean +- [ ] ≥10–20% free storage; read replicas deleted; HA capacity and NSG rules confirmed +- [ ] `logfiles.download_enable = ON` +- [ ] `POSTGRES_USER` confirmed not in `username@servername` form +- [ ] Root `.env` pointed at local, prod credentials moved to a named file +- [ ] Where production migrations run: established +- [ ] Deploy freeze in effect +- [ ] Manual `pg_dump` taken; retention window confirmed +- [ ] Rehearsal done on a PITR-restored server; duration recorded as `______` +- [ ] Plan baselines captured on 16 +- [ ] **Post-upgrade:** `datcollversion` compared against the 2.38 baseline — a change means + reindexing every text index, then `ALTER DATABASE … REFRESH COLLATION VERSION` in that order +- [ ] **Post-upgrade:** minor version recorded; collation provider checked; seven trigram indexes + reindexed `CONCURRENTLY` and all valid; extension version verified; `ANALYZE` run +- [ ] **Post-upgrade:** four RLS invariant queries re-run and compared; `security_invoker` still + unset; `rls-all-tables` suite green; Azure ADMIN-option change reviewed +- [ ] **Post-upgrade:** `yarn generate:sql` produces no diff +- [ ] **Post-upgrade:** real free-text search from the UI; one full scrape-ingestion batch +- [ ] ~~Read replicas re-created; HA re-enabled with a standby present~~ — **N/A on this server**: + no replicas, HA unavailable on Burstable (confirmed 2026-09-01) diff --git a/docs/postgresql-v18-migration.md b/docs/postgresql-v18-migration.md new file mode 100644 index 000000000..b7152994f --- /dev/null +++ b/docs/postgresql-v18-migration.md @@ -0,0 +1,535 @@ +# Postgres 16 → 18 upgrade: code review, required adjustments, and follow-ups + +## Context + +Production Postgres is moving from v16 to v18. This plan answers three questions asked of the +codebase — what must change in code, what the upgrade unblocks, and what else matters — and then +scopes the work. + +**Headline: the SQL in this repo needs no changes to run on 18.** I checked every PG17 and PG18 +incompatibility from the upstream release notes against the schema and the server's queries. The +codebase avoids essentially all of them, for structural reasons rather than luck: + +| PG17/18 incompatibility | Status here | +| -------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| PG18: `GENERATED ALWAYS AS (…)` defaults to VIRTUAL instead of STORED | No generated columns exist, in server SQL or in any of the 205 migrations | +| PG17: restricted `search_path` for `VACUUM`/`ANALYZE`/`REINDEX`/`CREATE INDEX`/matviews | No materialized views; the only expression indexes use `COALESCE` (built-in). RLS helpers already declare `SET search_path = pg_catalog` (`2026-02-10T12-05-00.create-rls-helper-function.ts:12,36,52`) | +| PG18: `AFTER` triggers run as the role active when queued | 20 `AFTER` triggers, but none are `CONSTRAINT`/`DEFERRABLE`, so they fire at statement end under one unchanging role | +| PG18: `VACUUM`/`ANALYZE` now recurse into inheritance children | No partitioned or inherited tables | +| PG18: unlogged partitioned tables disallowed | No unlogged tables | +| PG18: `COPY FROM` CSV no longer treats `\.` as EOF | No `COPY FROM` | +| PG17: `pg_stat_statements` renames, `pg_stat_bgwriter` → `pg_stat_checkpointer`; PG18: `pg_stat_wal`/`pg_stat_io` column changes | No monitoring catalog reads in app code — `observability/pool-monitor.ts` reads the in-process `pg.Pool` only | +| PG17: `old_snapshot_threshold`, `db_user_namespace`, `adminpack` removed | None referenced | +| PG18: MD5 password auth deprecated | No `md5()`/pgcrypto in SQL; hashing is `node:crypto` | +| Driver stack (`pg` 8.23.0, `slonik` 49.10.9, `pg-promise` 12.7.1, pgTyped 2.4.x) | node-postgres supports PG18; wire protocol stays 3.0, so PG18's 256-bit cancel keys (protocol 3.2 only) don't apply. The `@slonik/pg-driver` patch only relaxes a multi-statement guard — no protocol code | + +## Status (2026-08-31) + +Everything in Part 1 and four of Part 4's findings are **merged into `main`**. What remains is the +production upgrade itself (Part 2) and the opt-in follow-ups (Part 3). + +| Item | PR | State | +| ----------------------------------------------------- | --------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | +| Part 1 — dev + CI pinned to Postgres 18 | [#4329](https://github.com/Urigo/accounter-fullstack/pull/4329) | ✅ merged | +| Part 4 — `getLedgerBalanceToDate` credit-entity-2 bug | [#4326](https://github.com/Urigo/accounter-fullstack/pull/4326) | ✅ merged | +| Part 4 — `$isUnmatched` correlation bug | [#4327](https://github.com/Urigo/accounter-fullstack/pull/4327) | ✅ merged | +| Part 4 — non-sargable date predicates | [#4331](https://github.com/Urigo/accounter-fullstack/pull/4331) | ✅ merged | +| Part 4 — `financial_entities.name` trigram index | [#4330](https://github.com/Urigo/accounter-fullstack/pull/4330) | ✅ merged | +| Part 2 — production upgrade runbook | — | ✅ runbook written ([`operations/postgres-18-upgrade.md`](operations/postgres-18-upgrade.md)); the upgrade itself not yet performed | +| Part 3 — 3.1 … 3.7 follow-ups | — | ⬜ not started | + +**Two of those merges change Part 2's checklist,** so read the amendments rather than an older copy: + +- #4330 added a **seventh** trigram index (`idx_financial_entities_name_trgm`), which must be in the + post-upgrade `REINDEX` set — see Part 2 check 1. +- #4331 added **two new expression indexes** over `COALESCE(...)` date columns, both leading with + `owner_id`. That strengthens the `ANALYZE` step (check 3) and makes the RLS + `ScalarArrayOpExpr`-selectivity question in check 5 more consequential, not less. + +Version skew is now resolved in the safe direction: dev and CI run 18 while production is still on +16, which is the harmless ordering. Do not let production overtake them. + +--- + +**The stack is split across two vendors: the backend is deployed on Render, and the Postgres +database is Azure Database for PostgreSQL — Flexible Server.** (An earlier draft of this plan +treated the whole thing as Render, which got the database wrong. A later draft over-corrected and +dismissed the Render references as staging-only — also wrong: Render is the production _application_ +host, and production migrations really do run in its build command against the Azure database, +confirmed 2026-08-31. Both vendors belong in this plan, for different layers. There is still no +`render.yaml` or infrastructure config in the repo, so neither layer is inferable from the code.) + +Azure removes the usual cutover risks too, but for its own reasons: an in-place major version +upgrade "retains the server name and other settings of the current server" and requires no changes +to application connection strings, so no application config or code deploy is needed for the upgrade +itself. PG18 is GA on Flexible Server with in-place upgrade support in all regions. + +So the actual work is small and splits three ways: two version pins plus a Docker-image layout break +(Part 1), a runbook for the things Azure's upgrade docs leave to us (Part 2), and a set of genuinely +useful follow-ups that only become available on 18 (Part 3). + +--- + +## Part 1 — Required code changes ✅ merged in #4329 + +Scope: 3 files. Do this **before or with** the production upgrade, never after — CI codegen and +tests validate queries against the pinned dev image, so leaving it on 16 means PRs are checked +against a planner and parser that production no longer runs. + +**Landed as [#4329](https://github.com/Urigo/accounter-fullstack/pull/4329).** Verified on 18.6 +before merge: all migrations applied cleanly, `yarn test` / `yarn test:integration` / +`yarn test:demo-seed` / `rls-all-tables` all green, and pgTyped output byte-identical to the same +codegen run against 16.10 (zero type drift). The subsections below are kept as the record of what +changed and why — in particular the `PGDATA` symlink trap, which is the thing to remember at the +next major bump. + +> **Developer action, if you haven't already:** `rm -rf docker/.accounter-dev/postgresql/db` then +> `yarn local:setup`. An 18 server will not start against a 16 data directory. + +### 1.1 `docker/docker-compose.dev.yml` — bump the image _and_ fix the data-dir mount + +This is the one change with a real trap. The official `postgres:18` images moved `PGDATA` to a +version-scoped path (`/var/lib/postgresql/18/docker`) and moved the declared `VOLUME` to +`/var/lib/postgresql`. `/var/lib/postgresql/data` is now a **symlink**, so the current config — +which both sets `PGDATA` to that path and bind-mounts over it — makes the container fail at startup. +A tag-only bump breaks every CI workflow that uses `.github/actions/setup` with `localDB`/`pgTyped`, +because that action brings this exact compose file up (`action.yml:68-75`) and then runs +`yarn db:init` and pgTyped against it. + +```yaml +db: + image: postgres:18-alpine # was postgres:16-alpine + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: accounter + # postgres:18+ scopes PGDATA by major version (/var/lib/postgresql/18/docker) and + # declares its VOLUME at /var/lib/postgresql. Leave PGDATA unset and mount the + # parent: /var/lib/postgresql/data is a symlink in these images, so mounting + # over it fails at startup. + volumes: + - ./.accounter-dev/postgresql/db:/var/lib/postgresql +``` + +Drop the `PGDATA` line entirely. The healthcheck (`pg_isready -d $$POSTGRES_DB -U $$POSTGRES_USER`) +is unaffected. + +**Dev-machine step:** the cluster now lands in `./.accounter-dev/postgresql/db/18/docker`, and an 18 +server refuses to start against a 16 data directory regardless. Each developer deletes +`./.accounter-dev/postgresql/db` and re-runs `yarn local:setup`. Dev-only data; nothing to preserve. +CI is unaffected — the path is fresh every run. + +### 1.2 `.github/workflows/server-tests.yml:17` — bump the service container + +`image: postgres:16-alpine` → `postgres:18-alpine`. Service containers mount no volume, so the tag +bump alone is sufficient here. + +### 1.3 `README.md` — refresh the local-DB instructions + +Add the "delete `./.accounter-dev/postgresql/db` when the major version changes" note near the +`yarn local:setup` step (step 4). While in there, fix the service name in the prerequisites block at +`README.md:142`: it says `docker compose -f docker/docker-compose.dev.yml up -d postgres`, but the +service is named `db` (`docker-compose.dev.yml:4`), so that command has never worked. + +--- + +## Part 2 — Upgrade runbook ✅ written + +**The runbook now lives at +[`docs/operations/postgres-18-upgrade.md`](operations/postgres-18-upgrade.md) — use that, not this +section, on upgrade day.** Written in the style of the existing +`docs/operations/db-connection-pool.md` (problem → diagnosis → steps), it is the operational +document: failure modes, pre-flight, rehearsal, the six post-upgrade checks, a symptom → first-check +table, the rollback reality, and a sign-off checklist. + +The rest of this section is the reasoning it was distilled from, kept for review. It exists because +Azure's upgrade docs cover the mechanics but say nothing about the parts that are ours: the trigram +reindex decision, statistics, and re-asserting the RLS invariants. + +Three things the runbook has that this section did not, all found while writing it: + +- The RLS re-assertion reuses the **four queries already verified against production** + (`docs/coherent-owner-scoping-for-mcp/plan.md:51-108`) together with their expected values, rather + than the three generic catalog queries sketched below. Step 3 (`relforcerowsecurity`) is the one + that fails quietly with every other check green — worth knowing that was learned by running them. +- The trigram `REINDEX` must be `CONCURRENTLY` for **all seven**: only + `idx_financial_entities_name_trgm` was originally built that way, so a plain `REINDEX` would take + `ACCESS EXCLUSIVE` on `charges`, `transactions` and `documents`. +- A post-reindex `indisvalid`/`indisready` check, since a `CONCURRENTLY` rebuild that fails part-way + leaves behind an index the planner silently ignores — the same trap #4330's migration guards + against at build time. + +**Step 0 — run Azure's Upgrade Validation Checks.** Do this first, and days early. Flexible Server +has a first-class pre-flight check (portal or `az postgres flexible-server upgrade-validate`) that +runs Azure's real compatibility rules against the actual server without touching it — no downtime, +no restart, no version change. It is authoritative in a way this document cannot be, because the +blocking rules vary by source/target pair and change over time. Requirements: server status +**Ready**, no other operation in progress, connectivity to every database on the server; it cannot +run against a read replica. Treat a clean validation run, not this checklist, as the go/no-go. + +**Prerequisites (Azure-specific)** + +- **≥10–20% free storage.** Azure calls this out explicitly: temporary log files and metadata + operations grow disk usage mid-upgrade, and insufficient space causes upgrade failure or rollback. + Check before booking the window; growing storage on Flexible Server is one-way. +- **Delete read replicas first.** In-place upgrade does not support geo-replication or read replicas + — every replica, including cascading ones, must be deleted before upgrading the primary and + re-created afterward. This is the one prerequisite that can't be done inside a short window. +- **Confirm the minor version you land on, don't choose it.** Azure automatically deploys the latest + supported minor as part of the upgrade, so there is no 18.0 risk and nothing to negotiate — but + record what you actually got. For context on why the minor matters: 18.1 fixed an OOM in parallel + GIN index builds, 18.3 was an out-of-cycle release fixing regressions including a standby freeze, + **18.5 was never released**, and 18.6 (2026-08-13) is current. +- **HA is dropped and re-added around the upgrade.** If HA is enabled, Azure disables it, upgrades + the primary, then re-enables it — which needs spare capacity to provision a new standby. Verify + the server's NSG rules permit traffic on ports **5432 and 6432** within the VNet and outbound to + Azure Storage for log archiving; if they don't, HA silently fails to re-enable and you finish the + window with no standby. +- **Enable upgrade logs before you start:** set `logfiles.download_enable = ON` and a sane + `logfiles.retention_days`, then read `PG_Upgrade_Logs` during the run. This is the only real + visibility into a stalled upgrade. +- **Check the connection username format.** If this server was ever automigrated from Single Server, + the `username@servername` login format stops working after an in-place major upgrade — only plain + `username` is accepted. Our connection string is assembled from `POSTGRES_USER` + (`packages/server/scripts/set-db-url.cjs`, `environment.ts:319`), so this is a one-line env check + across the server, migrations, MCP server and scrapers — but an easy way to take the app down + post-upgrade if missed. +- **Freeze `prod` deploys for the window,** and first **confirm where production migrations actually + run.** The claim that they run in the deploy build command is sourced to + `packages/server/docs/demo-staging-guide.md:552-558`, which documents _staging on Render_ — it + says nothing reliable about the Azure production deploy path, and nothing in the repo does. What + is verifiable and still matters: the migration runner takes **no advisory lock** + (`pg-migrator.ts`), so if migrations do run on deploy, a deploy landing mid-upgrade fails its + build partway through. Resolve this before the window rather than assuming either way. + +**No extension work needed for our schema.** Azure auto-upgrades most extensions during an in-place +upgrade, and `pg_trgm` — the only extension the schema creates +(`2026-03-23T12-00-00.index-search-strings.ts:7`) — is not on any of Azure's blocked lists. Checked +against Azure's blockers, the schema is also clean on: `EVENT TRIGGER`s (none — Azure's precheck +blocks them and requires dropping and recreating), views depending on `pg_stat_activity` (none; the +only two references are a comment in `server/src/index.ts:40` and a `pg_terminate_backend` call in +`rls-all-tables.test.ts:50`), objects depending on `pg_stat_statements` (none), large objects +(none), and PostGIS/TimescaleDB (not used). + +**Rehearsal (this is the whole risk-reduction strategy)** + +Azure's flow: **PITR-restore production to a new server**, upgrade that server, exercise it, record +the duration as the estimate for production, then delete it. Against the restored server, run the +checks below plus a real free-text search and one full scrape-ingestion batch. + +Be precise about what rollback means here, because it is weaker than it looks: + +- **Failure _during_ the upgrade** is well covered. Azure takes an implicit backup after a + successful precheck and immediately before starting, and can use it to restore the instance to its + previous version on error. +- **Regret _after_ a successful upgrade** is not. There is **no automated way to revert** — recovery + is a PITR restore to a timestamp before the upgrade, which lands on a **new server** with a new + name, and therefore is a connection-string change and a real cutover, not a rollback. Budget for + this being the actual worst case, and confirm the backup retention window covers how long it might + take someone to notice a regression. + +**Post-upgrade checks — ours, not Azure's** + +1. **Decide the trigram reindex.** PG18 changed full-text search and `pg_trgm` to use the cluster's + default collation provider instead of always libc. Upstream: clusters defaulting to a _non-libc_ + provider "could observe changes in behavior of some full-text search functions, as well as the + `pg_trgm` extension", and should reindex FTS and `pg_trgm` indexes after `pg_upgrade`. So check + first: + + ```sql + SELECT datname, datlocprovider, datcollate, datctype + FROM pg_database WHERE datname = current_database(); + ``` + + `datlocprovider = 'i'` (ICU) or `'b'` (builtin) → reindex is required. `'c'` (libc) → not + required. Either way there are only seven indexes and this is cheap insurance: + + ```sql + REINDEX INDEX CONCURRENTLY accounter_schema.idx_charges_desc_trgm; -- charges.user_description + REINDEX INDEX CONCURRENTLY accounter_schema.idx_trans_src_trgm; -- transactions.source_description + REINDEX INDEX CONCURRENTLY accounter_schema.idx_trans_src_ref_trgm; -- transactions.source_reference + REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_desc_trgm; -- documents.description + REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_remarks_trgm; -- documents.remarks + REINDEX INDEX CONCURRENTLY accounter_schema.idx_docs_serial_trgm; -- documents.serial_number + REINDEX INDEX CONCURRENTLY accounter_schema.idx_financial_entities_name_trgm; -- financial_entities.name (added #4330) + ``` + + The seventh index is new: #4330 added `idx_financial_entities_name_trgm` + (`2026-08-31T11-00-00.index-financial-entity-names.ts`) for the six counterparty-name `ILIKE` + branches in the charges free-text filter. It was the one hot search column the original trigram + work missed, and it is now equally exposed to the collation-provider change — do not work from a + six-index copy of this list. + + This is load-bearing, not hygiene: the entire charges/transactions/documents free-text search is + `ILIKE '%…%'` served by these GIN indexes, and `charges.provider.ts:213` states the strategy + outright ("Identify IDs via Trigram indexes before doing any heavy math"). PG18 also adds + parallel GIN builds (`enable_parallel_gin_build`, `max_parallel_maintenance_workers`), so the + rebuild is faster than it would have been on 16. Use `CONCURRENTLY` for all seven regardless of + how they were originally built: the first six were created with a plain + `CREATE INDEX IF NOT EXISTS` (`2026-03-23T12-00-00.index-search-strings.ts`), only + `idx_financial_entities_name_trgm` was built `CONCURRENTLY` (#4330). A plain `REINDEX` takes an + `ACCESS EXCLUSIVE` lock and would block writes to `charges`, `transactions` and `documents` for + the duration. + +2. **Verify `pg_trgm`'s extension version** — `SELECT extname, extversion FROM pg_extension;`. Azure + states it auto-upgrades most extensions during an in-place major upgrade, so unlike a bare + `pg_upgrade` this should already be current. Run `ALTER EXTENSION pg_trgm UPDATE;` only if the + version is behind what 18 ships; it is idempotent and harmless either way. + +3. **`ANALYZE` the schema — Azure documents this as a required post-upgrade step,** not an + optimization: run `ANALYZE` in each database to refresh `pg_statistic`, because missing or stale + statistics cause bad plans and excess memory use. PG18's `pg_upgrade` retains optimizer + statistics, but _extended_ statistics are explicitly not preserved. There are no + `CREATE STATISTICS` objects here, but there are now **six** expression indexes carrying their own + statistics, and #4331 added two of them: + + - scraper dedup: `COALESCE(full_purchase_date, full_purchase_date_outbound)` + (`2026-05-04T12-00-00.…:8-38`) + - `idx_transactions_owner_effective_debit_date` on + `(owner_id, COALESCE(debit_date_override, debit_date))` + - `idx_documents_owner_vat_report_date` on `(owner_id, COALESCE(vat_report_date_override, date))` + (both from `2026-08-31T10-00-00.add-tenant-scoped-date-indexes.ts`) + + The two new ones are directly in the path of the date-range filters that #4331 just made + sargable, so stale statistics on them would regress the query shapes that PR was written to fix. + A plain `ANALYZE` over `accounter_schema` is minutes and removes the question. + +4. **Re-assert the RLS invariants.** These are the ones that would hurt, and dev/CI cannot tell you + anything about them because dev and CI connect as `postgres` — a superuser with `BYPASSRLS` + (acknowledged in `packages/server/src/__tests__/helpers/rls-role.ts:6-9`). Production's model + exists only in prose: `prod_group` owns `tags` and `extended_tags`, `extended_charges` has a + different owner (`accounter_prod_user`), `FORCE ROW LEVEL SECURITY` is load-bearing because a + table owner otherwise bypasses its own RLS, and no view sets `security_invoker` + (`docs/coherent-owner-scoping-for-mcp/plan.md:51-108`, + `packages/mcp-server/docs/owner-scoping-review.md:81-86`). None of it is created or asserted by + any migration. So verify on the upgraded server: + + ```sql + SELECT rolname, rolsuper, rolbypassrls FROM pg_roles + WHERE rolname IN ('accounter_prod_user', 'prod_group'); + + SELECT relname, relrowsecurity, relforcerowsecurity, relowner::regrole + FROM pg_class WHERE relnamespace = 'accounter_schema'::regnamespace AND relrowsecurity; + + SELECT c.relname, c.relowner::regrole, c.reloptions -- security_invoker must stay unset + FROM pg_class c WHERE c.relnamespace = 'accounter_schema'::regnamespace AND c.relkind = 'v'; + ``` + + Then run the existing `packages/migrations/src/__tests__/rls-all-tables.test.ts` against the + restored server — it already builds a non-superuser role and checks every table. Note also that + Azure changes role privileges across a major upgrade: after the upgrade, the first user created + on the server with the ADMIN option holds administrative privileges over other roles. Re-check + the `prod_group` / `accounter_prod_user` ownership split against that. + +5. **Capture before/after plans** for the queries in Part 3.4. Every query in the app carries an + implicit `owner_id = ANY (accounter_schema.get_current_business_scope())` from RLS + (`2026-05-25T10-00-00.rls-multi-business-scope.ts:108`), and the planner can't see that array's + length. Any shift in `ScalarArrayOpExpr` selectivity moves plans across the whole application at + once rather than query by query, so a handful of baseline `EXPLAIN (ANALYZE, BUFFERS)` captures + is worth more here than in a typical app. (`BUFFERS` is on by default in 18.) + + **#4331 raised the stakes here.** Its four new indexes are all composite and all **lead with + `owner_id`** — the very column whose selectivity the planner has to guess through + `get_current_business_scope()`. Before that PR the date filters were non-sargable and no index + could serve them, so a misestimate cost little; now the intended plans depend on those indexes + being chosen. Include at least one date-filtered charges query and one date-filtered documents + query in the baseline set, and capture them on **current `main`**, not on a pre-#4331 checkout — + a baseline taken against the old text-cast predicates measures a query shape that no longer + exists. + +6. **Regenerate pgTyped types against 18** (`yarn generate:sql`) and confirm no type drift. Codegen + introspects a live database, so this is where any inference difference surfaces first. + +--- + +## Part 3 — What the upgrade unblocks (separate PRs, not upgrade-day work) + +Ordered by expected value. Each is independently shippable. + +### 3.1 `uuidv7()` for new-row PK defaults — the best of these + +Roughly 40 tables default their PK to `gen_random_uuid()` (v4, uniformly random) — including the +append-heavy ones: `charges`, `transactions`, `documents`, `ledger_records`, and every scraper raw +table. Random v4 PKs scatter every insert across the whole B-tree, causing page splits, poor cache +locality and extra WAL. PG18's `uuidv7()` is timestamp-ordered, so inserts land at the right-hand +edge of the index. + +The change is well-scoped because **id generation is almost entirely a DB-side `DEFAULT`** — a +migration altering the default is the whole change for those tables, and mixing v4 and v7 values in +one column is fine. Five app-side sites generate UUIDs and must be left alone or considered +separately: + +- `shared/helpers/deterministic-uuid.ts` (uuid v5) → `entity-ensure.provider.ts:48-72`. **Do not + touch**: the determinism _is_ the idempotency mechanism for `ON CONFLICT (id) DO NOTHING`. +- `auth/providers/invitations.provider.ts:161`, `email-ingestion-control.provider.ts:255-257` + (`jti`), `email-ingestion-ingest.provider.ts:311,449,484-485,719` — low volume, no urgency. + +Trade-off to state in the PR: `uuidv7()` embeds creation time, so ids stop being opaque about when a +row was made. For internal accounting ids that's typically fine, but it's a deliberate choice. + +### 3.2 Temporal constraints for `clients_contracts` + +`clients_contracts` stores `client_id`, `start_date`, `end_date` +(`2025-08-11T11-45-42.clients-contracts-table.ts:12-39`) with **no protection against two +overlapping contracts for the same client** — today that invariant is either enforced in application +code or not at all. PG18's `WITHOUT OVERLAPS` moves it into the database: + +```sql +ALTER TABLE accounter_schema.clients_contracts + ADD CONSTRAINT clients_contracts_no_overlap + UNIQUE (client_id, daterange(start_date, end_date, '[]') WITHOUT OVERLAPS); +``` + +Backfill caveat: this fails if overlapping rows already exist, so the migration needs a detection +query first and a decision about existing data. Worth checking production for violations before +committing to it. + +### 3.3 `NOT NULL NOT VALID` for future migrations + +205 migrations run during deploy, blocking (see the caveat in Part 2 about confirming exactly +where). On 16, adding `NOT NULL` to a large table requires a full blocking validation scan. PG18 +allows `NOT NULL NOT VALID` plus a later `VALIDATE CONSTRAINT`, splitting that into a fast metadata +change and a non-blocking validation. This is a convention to document rather than code to write — +the natural home is `packages/migrations/README.md`, alongside the existing guideline that +"migrations should not take too long to run". + +Same doc should note the PG18 generated-column default: from 18 on, `GENERATED ALWAYS AS (expr)` +without `STORED` means **VIRTUAL** (recomputed on read). There are no generated columns today, so +this is purely forward-looking — but it's the kind of default change that silently surprises whoever +writes the first one. + +### 3.4 Skip scan — real candidates, on the scraper hot path + +PG18's B-tree skip scan uses a multicolumn index when a _leading_ column is unconstrained but later +ones are. Four dedup queries have exactly that gap today and can only use a prefix of their index: + +| Query | Index | Gap | +| ----------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------- | +| `foreign-securities.provider.ts:79-92` | `poalim_securities_transactions_dedup_uindex (owner_id, bank_number, branch_number, account_number, security, trade_date, value_date, …)` | `trade_date` skipped, `value_date` supplied — the best candidate in the repo | +| `poalim-scraper-ingestion.provider.ts:92-94` | `poalim_ils_account_transactions_dedup_uindex (event_date, serial_number, account_number, branch_number)` | `serial_number` skipped | +| `otsar-hahayal-scraper-ingestion.provider.ts:155-158` | `otsar_hahayal_foreign_account_transactions_conflict_key (account, branch, date, value_date, reference, description)` | `value_date` skipped, `reference` supplied | +| `poalim-scraper-ingestion.provider.ts:391-395` | securities dedup index | prefix + `security` + `trade_date`, `owner_id` supplied only by RLS | + +These may simply get faster with no code change — which is the point. `EXPLAIN` them on the upgraded +clone before adding any index; skip scan may make a planned index unnecessary, and it may also make +some existing single-column indexes redundant against their composite siblings. Measure before +dropping anything. + +### 3.5 `RETURNING old.* / new.*` + +Useful at the 8 `ON CONFLICT … DO UPDATE` sites and for audit logging: capture before-and-after in +one statement instead of SELECT-then-UPDATE, which also closes the race between the two. Worth being +precise about the limit — this does **not** help the 55 `ON CONFLICT … DO NOTHING` sites, since no +row is processed there and `RETURNING` stays empty. + +### 3.6 Observability + +PG18 adds `pg_stat_all_tables.total_vacuum_time` / `total_analyze_time` (and autovacuum variants), +byte-level columns in `pg_stat_io`, per-index lookup counts in `EXPLAIN ANALYZE`, `log_connections` +granularity and a `%L` client-IP log prefix. The pool heartbeat in `observability/pool-monitor.ts` +is currently client-side only; a DB-side counterpart would pair well with the diagnosis queries +already in `docs/operations/db-connection-pool.md`. Check which of these are exposed as Azure +**server parameters** before planning on the logging ones — Flexible Server withholds a number of +GUCs, and `log_connections` granularity in particular is worth confirming rather than assuming. + +### 3.7 Async I/O — set expectations honestly + +PG18's AIO subsystem (`io_method`, `effective_io_concurrency` default raised to 16) speeds up +sequential and bitmap heap scans, which is the shape of the big free-text search and the +`extended_charges` view. But per `docs/all-charges-performance-boost/findings.md`, the measured +AllCharges baseline was **23.95s across ~290 sequential DB round trips** — a latency-bound, +app-level N+1 problem that AIO cannot touch. The app-side plan in that folder remains the +higher-leverage work. Note also that AIO has been the least settled part of 18 across minors, and +some reports tie it to connection-pool interactions; if Azure exposes `io_method` as a server +parameter, leave it at the default rather than tuning it as part of this upgrade. + +--- + +## Part 4 — Other insights + +### ✅ Resolved — the four findings that shipped + +All four were found while surveying the SQL, none was upgrade-related, and all four merged on +2026-08-31 ahead of the production upgrade. Recorded here because they change what Part 2 must do, +and because the two correctness bugs affected historical output. + +1. **`getLedgerBalanceToDate` dropped credit entity 2** — merged in + [#4326](https://github.com/Urigo/accounter-fullstack/pull/4326). The four-branch `UNION` had two + byte-identical branches (`credit_entity1, credit_local_amount1, invoice_date`, twice) while + `credit_entity2` / `credit_local_amount2` appeared nowhere, so credit-side entity-2 amounts were + silently missing from every balance it computed — a wrong number in an accounting application, + not a slow query. **Note for the upgrade window:** this changes balances that were previously + understated. Don't mistake a corrected balance for an upgrade regression when comparing + before/after output. + +2. **`$isUnmatched` lost its correlation** — merged in + [#4327](https://github.com/Urigo/accounter-fullstack/pull/4327). In + `NOT EXISTS (SELECT 1 FROM … transactions t WHERE t.charge_id = charge_id)` the unqualified + `charge_id` resolved to the _inner_ `t.charge_id`, collapsing the predicate to "no transaction + anywhere has a non-null charge_id" — the filter returned nothing as soon as any transaction was + matched. Now correlated against `documents.charge_id`. + +3. **Non-sargable date predicates** — merged in + [#4331](https://github.com/Urigo/accounter-fullstack/pull/4331). This was the item that outranked + every PG18 feature in Part 3, and it is done: the `::TEXT::DATE` round-trips are gone (verified — + no `::TEXT::DATE` remains anywhere in `packages/server/src` or `packages/mcp-server/src`), and + `2026-08-31T10-00-00.add-tenant-scoped-date-indexes.ts` backs the filters with four composite + `(owner_id, )` indexes, two of them over `COALESCE(...)` expressions. `COALESCE` over two + date columns is immutable and therefore a legal index expression, which the old `::TEXT::DATE` + form was not. See Part 2 checks 3 and 5 for the consequences. + +4. **`financial_entities.name` trigram index** — merged in + [#4330](https://github.com/Urigo/accounter-fullstack/pull/4330). + `idx_financial_entities_name_trgm` (GIN, `gin_trgm_ops`, built `CONCURRENTLY`) now serves the six + counterparty-name `ILIKE '%…%'` branches in the charges free-text filter that previously fell + back to a sequential scan against a plain btree. The btree is deliberately kept for equality, + prefix and ordering. **This is the seventh index in Part 2 check 1's `REINDEX` set.** + +### ⬜ Still open + +**The `.env`-points-at-production hazard is amplified by this work.** Already flagged in-repo at +`packages/mcp-server/docs/todo.md:116-124`: the root `.env` is what codegen, migrations, seeds and +DB-backed tests all read, so anything reading it "targets production by default". During an upgrade +you will be running codegen and migrations more than usual, often against PITR-restored servers. +Worth a guard before the upgrade window, not after. Concretely: Part 2 check 6 asks you to run +`yarn generate:sql` against the upgraded server, and `packages/server/scripts/set-db-url.cjs` builds +`DATABASE_URL` from whatever `POSTGRES_*` happens to be in `.env` — one stale value and a rehearsal +step runs against production instead. + +**No backup, restore or replication tooling exists in the repo** — zero references to `pg_dump`, +`pg_restore`, `pg_basebackup`, publications or replication slots, and `package.json:42`'s +`seed:reset-staging` is an `echo` placeholder. The cutover is entirely Azure-managed, and a PITR +restore to a new server is the only rollback once the upgrade succeeds. This is a sharper gap on +Azure than it would be on a platform that leaves the old version running: an in-place `pg_upgrade` +converts the server in place, so "discovered days later" means restoring to a new server with a new +hostname and cutting over, bounded by the backup retention window. Worth deciding the retention +setting and a manual `pg_dump` before the window, not after. + +**Version-skew direction matters — now resolved, keep it that way.** Because dev/CI and production +are pinned independently, Part 1 went first (#4329). Dev and CI are on 18 while production is still +on 16, which is the harmless direction: queries are validated against the newer parser and planner. +The dangerous inversion — production on 18 while CI validates against 16 — is now only reachable by +reverting #4329, so don't. + +--- + +## Verification + +1. **Part 1 locally:** `rm -rf .accounter-dev/postgresql/db && yarn local:setup` — confirms the 18 + container starts with the new mount, `db:init` applies all 205 migrations cleanly on 18, and + `yarn generate` produces no pgTyped diff. Then `yarn test` and `yarn test:integration`. +2. **Part 1 in CI:** push the branch and confirm `server-tests.yml` (service container) and any + workflow using `.github/actions/setup` with `localDB`/`pgTyped` both go green. The setup action + is the one that would fail on the PGDATA symlink, so a green run there is the real signal. +3. **RLS on 18:** `yarn workspace @accounter-helper/migrations test` to exercise + `rls-all-tables.test.ts`, which creates a non-superuser role and checks every table's policy. +4. **On the upgraded PITR-restored server:** work Part 2's post-upgrade checklist — collation + provider, trgm reindex decision, `ALTER EXTENSION`, `ANALYZE`, the three RLS catalog queries, the + `rls-all-tables` suite, a real free-text search from the UI, and one full scrape-ingestion batch. + Record the upgrade duration as the production estimate. +5. **Plan baselines:** `EXPLAIN (ANALYZE, BUFFERS)` on `getChargesByFilters` + (`charges.provider.ts:211`), the `extended_charges` consumer + (`accountant-approval.provider.ts:12-20`), the creditcard LATERAL self-join + (`creditcard-transactions.provider.ts:10-27`), and the four Part 3.4 dedup queries — captured on + 16 and again on the 18 clone. +6. **Standard gates:** `yarn lint`, `yarn prettier:check`, `yarn generate` before committing. diff --git a/package.json b/package.json index 995cf7056..57adeea59 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "build:tools": "yarn workspaces foreach --all --parallel --include @accounter/etana-scraper --include @accounter/etherscan-scraper --include @accounter/green-invoice-graphql --include @accounter/hashavshevet-mesh --include @accounter/israeli-vat-scraper --include @accounter/kraken-scraper --include @accounter/payper-mesh --include @accounter/pcn874-generator --include @accounter/shaam-uniform-format-generator --include @accounter/shaam6111-generator run build", "changeset:publish": "node scripts/changeset-publish.mjs", "db:migrate": "yarn migration:run", + "db:reindex-trgm": "tsx scripts/reindex-trigram-indexes.ts", "encrypt-password": "node scripts/encrypt-password.mjs", "generate": "concurrently -c blue,green -n GraphQL,SQL \"yarn generate:graphql\" \"yarn generate:sql\"", "generate:all": "yarn generate", diff --git a/scripts/__tests__/reindex-trigram-indexes.test.ts b/scripts/__tests__/reindex-trigram-indexes.test.ts new file mode 100644 index 000000000..d7c55cdea --- /dev/null +++ b/scripts/__tests__/reindex-trigram-indexes.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, it, vi } from 'vitest'; +import { + buildReindexStatement, + discoverTrigramIndexes, + formatBytes, + formatDuration, + isConcurrentLeftover, + parseArgs, + quoteIdentifier, +} from '../reindex-trigram-indexes.js'; + +describe('buildReindexStatement', () => { + it('always emits CONCURRENTLY', () => { + // The whole point of the tool: six of the seven trigram indexes were originally created + // non-concurrently, and a plain REINDEX would take ACCESS EXCLUSIVE on charges, + // transactions and documents. + expect(buildReindexStatement('accounter_schema', 'idx_charges_desc_trgm')).toBe( + 'REINDEX INDEX CONCURRENTLY "accounter_schema"."idx_charges_desc_trgm"', + ); + }); + + it('quotes both schema and index name', () => { + const statement = buildReindexStatement('accounter_schema', 'idx_docs_desc_trgm'); + expect(statement).toContain('"accounter_schema"."idx_docs_desc_trgm"'); + }); + + it('never emits a bare REINDEX', () => { + const statement = buildReindexStatement('s', 'i'); + expect(statement).not.toMatch(/REINDEX INDEX "s"/); + }); +}); + +describe('quoteIdentifier', () => { + it('wraps in double quotes', () => { + expect(quoteIdentifier('simple')).toBe('"simple"'); + }); + + it('escapes embedded double quotes by doubling them', () => { + expect(quoteIdentifier('we"ird')).toBe('"we""ird"'); + }); + + it('neutralises an identifier that tries to close the quote and inject', () => { + // Names come from the catalog, not user input, but REINDEX takes no bind parameters so + // the statement is string-built and must be quoted properly regardless. + expect(quoteIdentifier('x"; DROP TABLE charges; --')).toBe( + '"x""; DROP TABLE charges; --"', + ); + }); +}); + +describe('parseArgs', () => { + it('defaults to the accounter schema, no action', () => { + const options = parseArgs([]); + expect(options.schema).toBe('accounter_schema'); + expect(options.dryRun).toBe(false); + expect(options.confirm).toBe(false); + }); + + it('defaults statement_timeout to 0 (disabled)', () => { + // A GIN rebuild can far exceed the app's 120s default; being killed part-way leaves an + // invalid index behind. + expect(parseArgs([]).statementTimeoutMs).toBe(0); + }); + + it('reads --dry-run and --confirm', () => { + expect(parseArgs(['--dry-run']).dryRun).toBe(true); + expect(parseArgs(['--confirm']).confirm).toBe(true); + }); + + it('accepts an explicit schema and timeout', () => { + const options = parseArgs(['--schema', 'other_schema', '--statement-timeout-ms', '600000']); + expect(options.schema).toBe('other_schema'); + expect(options.statementTimeoutMs).toBe(600_000); + }); + + it('falls back to 0 for a non-numeric timeout rather than NaN', () => { + expect(parseArgs(['--statement-timeout-ms', 'soon']).statementTimeoutMs).toBe(0); + }); +}); + +describe('isConcurrentLeftover', () => { + it.each(['idx_charges_desc_trgm_ccnew', 'idx_x_ccnew1', 'idx_x_ccnew12'])( + 'detects %j as a failed-rebuild leftover', + name => { + expect(isConcurrentLeftover(name)).toBe(true); + }, + ); + + it.each(['idx_charges_desc_trgm', 'idx_ccnew_something', 'idx_financial_entities_name_trgm'])( + 'does not flag %j', + name => { + expect(isConcurrentLeftover(name)).toBe(false); + }, + ); +}); + +describe('discoverTrigramIndexes', () => { + it('finds indexes by opclass rather than by a hardcoded name list', async () => { + // The stale-list failure mode is exactly what this tool exists to prevent: the set grew + // from six to seven and any copied list silently skipped the new one. + const query = vi.fn().mockResolvedValue({ + rows: [ + { + schema: 'accounter_schema', + index_name: 'idx_charges_desc_trgm', + table_name: 'charges', + is_valid: true, + is_ready: true, + bytes: '32768', + }, + ], + }); + + const indexes = await discoverTrigramIndexes({ query } as never, 'accounter_schema'); + + expect(indexes).toEqual([ + { + schema: 'accounter_schema', + indexName: 'idx_charges_desc_trgm', + tableName: 'charges', + isValid: true, + isReady: true, + bytes: 32768, + }, + ]); + + const [sql, params] = query.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('pg_opclass'); + expect(sql).not.toContain('idx_charges_desc_trgm'); + expect(params[0]).toBe('accounter_schema'); + expect(params[1]).toEqual(['gin_trgm_ops', 'gist_trgm_ops']); + }); + + it('coerces bigint byte counts arriving as strings', async () => { + const query = vi.fn().mockResolvedValue({ + rows: [ + { + schema: 's', + index_name: 'i', + table_name: 't', + is_valid: false, + is_ready: false, + bytes: '9007199254740993', + }, + ], + }); + const [index] = await discoverTrigramIndexes({ query } as never, 's'); + expect(typeof index?.bytes).toBe('number'); + expect(index?.isValid).toBe(false); + }); +}); + +describe('formatBytes', () => { + it.each([ + [0, '0 B'], + [512, '512 B'], + [1024, '1.0 KiB'], + [32768, '32.0 KiB'], + [1024 * 1024 * 5, '5.0 MiB'], + [1024 ** 3 * 2, '2.0 GiB'], + ])('formats %i as %s', (bytes, expected) => { + expect(formatBytes(bytes)).toBe(expected); + }); +}); + +describe('formatDuration', () => { + it.each([ + [16, '16ms'], + [1500, '1.5s'], + [65_000, '1m 5s'], + [3_600_000, '60m 0s'], + ])('formats %ims as %s', (ms, expected) => { + expect(formatDuration(ms)).toBe(expected); + }); +}); diff --git a/scripts/reindex-trigram-indexes.ts b/scripts/reindex-trigram-indexes.ts new file mode 100644 index 000000000..d0284029b --- /dev/null +++ b/scripts/reindex-trigram-indexes.ts @@ -0,0 +1,350 @@ +/* eslint-disable no-console -- this is an operator-facing CLI; its stdout is the product. */ + +/** + * Rebuild every GIN trigram index in the schema, always `CONCURRENTLY`. + * + * Why this exists + * --------------- + * Postgres 18 changed full-text search and `pg_trgm` to use the cluster's default collation + * provider instead of always libc. A cluster whose provider is ICU (`datlocprovider = 'i'`) + * or builtin (`'b'`) must reindex its FTS and `pg_trgm` indexes after `pg_upgrade`; on libc + * (`'c'`) it is not required but is cheap insurance. The entire charges / transactions / + * documents / counterparty free-text search is `ILIKE '%…%'` served by these indexes, so + * getting it wrong means wrong search results, not merely slow ones. + * + * Two traps this tool exists to remove: + * + * 1. **`CONCURRENTLY` is not optional.** Only `idx_financial_entities_name_trgm` was + * originally built concurrently; the rest came from a plain + * `CREATE INDEX IF NOT EXISTS` in `2026-03-23T12-00-00.index-search-strings.ts`. A plain + * `REINDEX` takes `ACCESS EXCLUSIVE` and would block all writes to `charges`, + * `transactions` and `documents` for the duration. + * 2. **A hardcoded list goes stale.** The set grew from six to seven when + * `idx_financial_entities_name_trgm` was added, and a runbook copied before that change + * silently skips it. So this discovers the indexes from `pg_index`/`pg_opclass` at + * runtime — whatever trigram indexes exist are the ones rebuilt. + * + * Usage + * ----- + * yarn db:reindex-trgm --dry-run # list what would be rebuilt, touch nothing + * yarn db:reindex-trgm --confirm # actually rebuild + * + * The target comes from `POSTGRES_*`, and running this against a deployed database is the + * whole point (upgrade day, or a PITR-restored rehearsal server) — so it prints the target + * and refuses to act without `--confirm`. + */ +import { config } from 'dotenv'; +import pg from 'pg'; + +config(); + +/** Opclasses that mark an index as a trigram index. */ +const TRIGRAM_OPCLASSES = ['gin_trgm_ops', 'gist_trgm_ops']; + +export type TrigramIndex = { + schema: string; + indexName: string; + tableName: string; + isValid: boolean; + isReady: boolean; + bytes: number; +}; + +export type ParsedArgs = { + schema: string; + dryRun: boolean; + confirm: boolean; + statementTimeoutMs: number; +}; + +/** + * Quote an identifier for interpolation. Index and schema names come from the catalog rather + * than user input, but they still reach a string-built statement — `REINDEX` takes no bind + * parameters — so quote them properly rather than trusting the source. + */ +export function quoteIdentifier(identifier: string): string { + return `"${identifier.replace(/"/g, '""')}"`; +} + +export function buildReindexStatement(schema: string, indexName: string): string { + return `REINDEX INDEX CONCURRENTLY ${quoteIdentifier(schema)}.${quoteIdentifier(indexName)}`; +} + +export function parseArgs(argv: readonly string[]): ParsedArgs { + const args = [...argv]; + const getValue = (flag: string): string | undefined => { + const index = args.indexOf(flag); + if (index === -1) return undefined; + return args[index + 1]; + }; + + const timeoutRaw = getValue('--statement-timeout-ms'); + const parsedTimeout = timeoutRaw === undefined ? Number.NaN : Number(timeoutRaw); + + return { + schema: getValue('--schema') ?? process.env.POSTGRES_SCHEMA ?? 'accounter_schema', + dryRun: args.includes('--dry-run'), + confirm: args.includes('--confirm'), + // 0 disables the per-statement timeout. A GIN rebuild on a large table can far exceed + // the app's 120s default, and being killed part-way leaves an invalid index behind. + statementTimeoutMs: Number.isFinite(parsedTimeout) ? parsedTimeout : 0, + }; +} + +export function formatBytes(bytes: number): string { + if (bytes < 1024) return `${bytes} B`; + const units = ['KiB', 'MiB', 'GiB', 'TiB']; + let value = bytes / 1024; + let unitIndex = 0; + while (value >= 1024 && unitIndex < units.length - 1) { + value /= 1024; + unitIndex += 1; + } + return `${value.toFixed(1)} ${units[unitIndex]}`; +} + +export function formatDuration(ms: number): string { + if (ms < 1000) return `${Math.round(ms)}ms`; + const seconds = ms / 1000; + if (seconds < 60) return `${seconds.toFixed(1)}s`; + const minutes = Math.floor(seconds / 60); + return `${minutes}m ${Math.round(seconds % 60)}s`; +} + +/** + * Leftovers from a failed `REINDEX INDEX CONCURRENTLY`: Postgres keeps the new index under a + * `_ccnew` suffix and marks it invalid. The planner ignores them but they consume space and + * are updated on every write, so they must be cleaned up by hand. + */ +export function isConcurrentLeftover(indexName: string): boolean { + return /_ccnew\d*$/.test(indexName); +} + +const DISCOVER_TRIGRAM_INDEXES = ` + SELECT n.nspname AS schema, + c.relname AS index_name, + t.relname AS table_name, + i.indisvalid AS is_valid, + i.indisready AS is_ready, + pg_relation_size(c.oid) AS bytes + FROM pg_index i + JOIN pg_class c ON c.oid = i.indexrelid + JOIN pg_class t ON t.oid = i.indrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + JOIN pg_am am ON am.oid = c.relam + WHERE n.nspname = $1 + AND EXISTS ( + SELECT 1 + FROM unnest(i.indclass::oid[]) AS ic(oid) + JOIN pg_opclass oc ON oc.oid = ic.oid + WHERE oc.opcname = ANY ($2::text[]) + ) + ORDER BY c.relname +`; + +export async function discoverTrigramIndexes( + client: Pick, + schema: string, +): Promise { + const result = await client.query(DISCOVER_TRIGRAM_INDEXES, [schema, TRIGRAM_OPCLASSES]); + return result.rows.map( + (row: { + schema: string; + index_name: string; + table_name: string; + is_valid: boolean; + is_ready: boolean; + bytes: string | number; + }) => ({ + schema: row.schema, + indexName: row.index_name, + tableName: row.table_name, + isValid: row.is_valid, + isReady: row.is_ready, + bytes: Number(row.bytes), + }), + ); +} + +function describeTarget(): string { + const user = process.env.POSTGRES_USER ?? ''; + const host = process.env.POSTGRES_HOST ?? ''; + const port = process.env.POSTGRES_PORT ?? '5432'; + const db = process.env.POSTGRES_DB ?? ''; + return `${user}@${host}:${port}/${db}`; +} + +async function main(): Promise { + const options = parseArgs(process.argv.slice(2)); + + const client = new pg.Client({ + user: process.env.POSTGRES_USER, + password: process.env.POSTGRES_PASSWORD, + host: process.env.POSTGRES_HOST, + port: parseInt(process.env.POSTGRES_PORT || '5432', 10), + database: process.env.POSTGRES_DB, + ssl: process.env.POSTGRES_SSL === '1' ? { rejectUnauthorized: false } : false, + }); + + await client.connect(); + + try { + console.log(`Target: ${describeTarget()}`); + console.log(`Schema: ${options.schema}`); + + // The collation provider decides whether this rebuild is *required* or merely prudent, + // so report it either way -- it is the first question the runbook asks. + const provider = await client.query<{ + datname: string; + datlocprovider: string; + datcollate: string; + }>( + `SELECT datname, datlocprovider, datcollate + FROM pg_database WHERE datname = current_database()`, + ); + const locProvider = provider.rows[0]?.datlocprovider; + const providerLabel = + locProvider === 'c' + ? 'libc — reindex not strictly required after pg_upgrade, but cheap insurance' + : locProvider === 'i' + ? 'ICU — reindex IS REQUIRED after a pg_upgrade to 18' + : locProvider === 'b' + ? 'builtin — reindex IS REQUIRED after a pg_upgrade to 18' + : `unknown (${String(locProvider)})`; + console.log(`Collation provider: ${providerLabel}`); + + const version = await client.query<{ server_version: string }>('SHOW server_version'); + console.log(`Server: Postgres ${version.rows[0]?.server_version ?? 'unknown'}`); + console.log(''); + + const indexes = await discoverTrigramIndexes(client, options.schema); + + if (indexes.length === 0) { + console.log( + `No trigram indexes found in "${options.schema}". Nothing to do.\n` + + '(If that is unexpected, check the schema name and that pg_trgm is installed.)', + ); + return; + } + + const leftovers = indexes.filter(index => isConcurrentLeftover(index.indexName)); + const invalid = indexes.filter( + index => !isConcurrentLeftover(index.indexName) && (!index.isValid || !index.isReady), + ); + const targets = indexes.filter(index => !isConcurrentLeftover(index.indexName)); + + console.log(`Found ${targets.length} trigram index(es):`); + for (const index of targets) { + const flags = index.isValid && index.isReady ? '' : ' ⚠️ INVALID'; + console.log( + ` ${index.indexName.padEnd(38)} ${index.tableName.padEnd(20)} ${formatBytes(index.bytes).padStart(10)}${flags}`, + ); + } + console.log(''); + + if (leftovers.length > 0) { + console.warn('⚠️ Leftovers from a previously failed REINDEX CONCURRENTLY:'); + for (const index of leftovers) { + console.warn(` ${index.schema}.${index.indexName}`); + } + console.warn( + ' These are ignored by the planner but still maintained on every write.\n' + + ' Drop them before rebuilding:\n' + + leftovers + .map( + index => + ` DROP INDEX CONCURRENTLY IF EXISTS ${quoteIdentifier(index.schema)}.${quoteIdentifier(index.indexName)};`, + ) + .join('\n'), + ); + console.warn(''); + } + + if (invalid.length > 0) { + console.warn( + `⚠️ ${invalid.length} index(es) are invalid and are being rebuilt — the planner is ignoring them right now.\n`, + ); + } + + if (options.dryRun || !options.confirm) { + console.log( + options.dryRun + ? 'Dry run — nothing was changed. Statements that would run:' + : 'No --confirm flag given, so nothing was changed. Statements that would run:', + ); + for (const index of targets) { + console.log(` ${buildReindexStatement(index.schema, index.indexName)};`); + } + console.log(''); + console.log(`Re-run with --confirm to rebuild against ${describeTarget()}`); + return; + } + + // REINDEX ... CONCURRENTLY cannot run inside a transaction block, so each statement is + // issued on its own with autocommit. No BEGIN anywhere in this function. + await client.query(`SET statement_timeout = ${Number(options.statementTimeoutMs)}`); + console.log( + options.statementTimeoutMs === 0 + ? 'statement_timeout disabled for this session (a large GIN rebuild can take a while).' + : `statement_timeout = ${options.statementTimeoutMs}ms`, + ); + console.log(''); + + const started = Date.now(); + const failures: Array<{ index: TrigramIndex; error: unknown }> = []; + + for (const [position, index] of targets.entries()) { + const label = `[${position + 1}/${targets.length}] ${index.indexName}`; + const indexStarted = Date.now(); + process.stdout.write(`${label} … `); + try { + await client.query(buildReindexStatement(index.schema, index.indexName)); + console.log(`done in ${formatDuration(Date.now() - indexStarted)}`); + } catch (error) { + console.log('FAILED'); + console.error(` ${error instanceof Error ? error.message : String(error)}`); + failures.push({ index, error }); + } + } + + console.log(''); + console.log(`Total: ${formatDuration(Date.now() - started)}`); + + // Re-read the catalog rather than trusting the absence of errors: a rebuild that was + // cancelled or interrupted can leave the index invalid without raising here. + const after = await discoverTrigramIndexes(client, options.schema); + const stillInvalid = after.filter(index => !index.isValid || !index.isReady); + + if (stillInvalid.length > 0) { + console.error(''); + console.error('❌ Indexes still invalid after the run:'); + for (const index of stillInvalid) { + console.error(` ${index.schema}.${index.indexName}`); + } + console.error( + ' An invalid index is ignored by the planner — free-text search will fall back\n' + + ' to sequential scans until this is resolved.', + ); + process.exitCode = 1; + return; + } + + if (failures.length > 0) { + console.error(`❌ ${failures.length} statement(s) failed; see above.`); + process.exitCode = 1; + return; + } + + console.log('✅ All trigram indexes rebuilt and valid.'); + } finally { + await client.end(); + } +} + +// Only run when executed directly, so the helpers above stay unit-testable. +if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) { + main().catch(error => { + console.error(error); + process.exit(1); + }); +}