Skip to content

auto-detect locale from old cluster during upgrade#52

Open
pkhartsk wants to merge 3 commits intodevexp-db:masterfrom
pkhartsk:master
Open

auto-detect locale from old cluster during upgrade#52
pkhartsk wants to merge 3 commits intodevexp-db:masterfrom
pkhartsk:master

Conversation

@pkhartsk
Copy link

including tests

Automatically detect and preserve locale settings (lc_collate, lc_ctype)
from the old PostgreSQL cluster during upgrade operations. This fixes
upgrade failures when system locale has changed between initialization
and upgrade.

The implementation queries locale from the old cluster's template1
database, temporarily starting the server if needed. Detected locale
values are automatically passed to initdb via PGSETUP_INITDB_OPTIONS.

Fixes: Bug 1152556
@pkhartsk pkhartsk force-pushed the master branch 7 times, most recently from 268116e to 5c19c23 Compare February 23, 2026 15:54
Replaced check for running postgres with failure in `detect_old_locale`,
since the script would fail later anyway, so there's no reason to have
extra code that does nothing.
Fixed `psql` being run from the old pg bin directory.
Wrote a test for the script.
@pkhartsk pkhartsk changed the title [DRAFT] auto-detect locale from old cluster during upgrade auto-detect locale from old cluster during upgrade Feb 23, 2026
@pkhartsk pkhartsk marked this pull request as ready for review February 23, 2026 16:48
@fila43
Copy link
Contributor

fila43 commented Mar 2, 2026

I looked at the changes in detect_old_cluster_locale() and I think there are few issues with how the running server check is done now.

Using old_data_in_use() inside detect_old_cluster_locale() is problematic:

  1. Duplicate error output — old_data_in_use() prints an error message when pidfile exists. But it gets called twice during upgrade, first from detect_old_cluster_locale() and then again from upgrade() itself (around line 413). So the user sees the same error printed twice.

  2. Weaker detection than original PR — old_data_in_use() only does test -f "$pidfile", it doesn't verify if the process is actually running. The original patch (PR Auto-detect locale from old cluster during upgrade #51) did kill -0 "$pid" which properly handled stale pidfiles after a crash. Now a leftover pidfile blocks both locale detection and the whole upgrade even though nothing is running.

  3. Global vs local variable mismatch — detect_old_cluster_locale() takes old_datadir as parameter but old_data_in_use() reads the global $pgdataold. They happen to be the same value right now but it's fragile.

I think better approach would be to move the running-server check to the beginning of upgrade() with immediate exit, before detect_old_cluster_locale() gets called.

Then detect_old_cluster_locale() doesn't need the old_data_in_use check at all — it can just assume server is not running and do its job (start temp server, query, stop). This way:

  • user gets one clear error message instead of two
  • detect_old_cluster_locale() stays simple without side effects
  • no dependency on global variable inside the function

One more thing — psql path
Using bare psql without a path depends on it being in PATH. I'd suggest using "$PGENGINE/psql" instead — PGENGINE points to @bindir@ (usually /usr/bin) where the client is actually installed. The old $old_bindir/psql from PR #51 was wrong (psql is not shipped there in newer packaging), but having an explicit path is still safer than relying on PATH.

Updated 'old_data_in_use()' to check if postgres is running, but exit either way even if there is just an orphan pidfile.
This only runs once at the beginning of 'upgrade()' to fail fast.
Use new psql's full path instead of realying on $PATH.
@fila43
Copy link
Contributor

fila43 commented Mar 5, 2026

LGTM lest's wait for test results

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants