Skip to content

🐛(docs) run migration 0027 without superuser role#2284

Merged
lunika merged 2 commits into
mainfrom
postgres/migration
May 13, 2026
Merged

🐛(docs) run migration 0027 without superuser role#2284
lunika merged 2 commits into
mainfrom
postgres/migration

Conversation

@lunika
Copy link
Copy Markdown
Member

@lunika lunika commented May 11, 2026

Purpose

In development environment, we want to use the application with a
postgres user not having superuser role. The migration 0027 needs
superuser role to be executed, so we want to be able to test migrations
and other scenarios with a normal user. In order to create this user,
the compose service must be deleted first and then recreated with
DB_USER and DB_PASSWORD environment variable values different with
POSTGRES_USER and POSTGRES_PASSWORD.

Once with user without superuser role, we modified the migration 0027 to make it runnable with a user without superuser role.

Proposal

  • 🔧(postgres) create a user without superuser role
  • 🐛(docs) run migration 0027 without superuser role

Fix #1895

@lunika lunika self-assigned this May 11, 2026
@lunika
Copy link
Copy Markdown
Member Author

lunika commented May 11, 2026

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Review Change Stack

Walkthrough

This pull request replaces a PostgreSQL C-language function with a SQL-based equivalent to enable deployments on managed databases without superuser privileges. The migration changes f_unaccent(text) to directly call PostgreSQL's built-in unaccent() function instead of delegating through a regdictionary-based helper. Docker initialization is configured to mount a scripts directory, and a new Bash entrypoint script creates a non-superuser application database user by reading environment variables and executing SQL with error handling. Configuration comments document the new setup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is partially related but uses vague terminology ('docs') in parentheses and doesn't clearly convey the main technical change. Consider using a more descriptive title like 'Refactor migration 0027 to work without superuser privileges' to better reflect the technical change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the purpose, problem, and proposed changes related to making migration 0027 runnable without superuser role.
Linked Issues check ✅ Passed The PR successfully addresses issue #1895 by modifying migration 0027 to remove C-language function dependencies and adding setup for non-superuser Postgres users in development.
Out of Scope Changes check ✅ Passed All changes directly support the objectives: Docker Compose volume mount for init scripts, user creation script, migration configuration, and migration 0027 refactoring are all in scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch postgres/migration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docker/files/docker-entrypoint-initdb.d/01_create_app_user.sh`:
- Around line 15-16: The SQL here interpolates DB_USER, DB_PASSWORD and
POSTGRES_DB directly into the CREATE USER and ALTER DATABASE statements; replace
the direct bash interpolation with psql variable substitution (use the psql -v
mechanism and reference variables inside SQL with :'VAR' or :VAR as appropriate)
so the script passes DB_USER, DB_PASSWORD and POSTGRES_DB into the SQL safely
rather than expanding them in the shell; update the CREATE USER and ALTER
DATABASE statements to use those psql variables (and keep proper
quoting/escaping) instead of ${DB_USER}/${DB_PASSWORD}/${POSTGRES_DB}.
- Around line 9-12: The script assumes environment variables POSTGRES_USER,
POSTGRES_DB, DB_USER and DB_PASSWORD are present; add explicit validation at the
top that each required var is set and non-empty (check POSTGRES_USER and
POSTGRES_DB always, and if DB_USER != POSTGRES_USER then require DB_USER and
DB_PASSWORD), and on missing values print a clear error naming the missing
variable(s) and exit with a non-zero status so psql isn't invoked with unclear
errors.
- Around line 14-17: Replace the non-idempotent CREATE USER line with an
idempotent CREATE ROLE ... LOGIN so the script won't fail if the role already
exists: use CREATE ROLE "${DB_USER}" WITH LOGIN PASSWORD '${DB_PASSWORD}'; keep
the existing ALTER DATABASE "${POSTGRES_DB}" OWNER TO "${DB_USER}"; (use the
same DB_USER, DB_PASSWORD and POSTGRES_DB variables) so the role is created as a
login role and the database ownership is still transferred without error on
repeated runs.

In `@env.d/development/postgresql`:
- Line 14: Fix the typo in the comment "# The database must be deleted in roder
to be recreated." by changing "roder" to "order" so it reads "# The database
must be deleted in order to be recreated."; locate and update that exact comment
in the postgresql environment config (the comment near the database recreate
instruction).

In `@src/backend/core/migrations/0027_auto_20251120_0956.py`:
- Around line 16-17: The SQL function in migration 0027_auto_20251120_0956.py is
using an invalid `return` line for a LANGUAGE sql function; update the function
definition to include the required AS clause and dollar-quoted body and end with
a semicolon (e.g., change the fragment after `LANGUAGE sql IMMUTABLE PARALLEL
SAFE STRICT` to use `AS $$ RETURN unaccent($1) $$;` or `AS $$ SELECT
unaccent($1) $$;`). Ensure you replace the lone `return unaccent($1);` line with
the proper AS $$ ... $$; block so the migration executes under PostgreSQL.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ba18b8fd-829e-464b-b2c1-da82a31b6931

📥 Commits

Reviewing files that changed from the base of the PR and between fa3beca and 4f5cb43.

📒 Files selected for processing (4)
  • compose.yml
  • docker/files/docker-entrypoint-initdb.d/01_create_app_user.sh
  • env.d/development/postgresql
  • src/backend/core/migrations/0027_auto_20251120_0956.py

Comment thread docker/files/docker-entrypoint-initdb.d/01_create_app_user.sh
Comment thread docker/files/docker-entrypoint-initdb.d/01_create_app_user.sh
Comment thread docker/files/docker-entrypoint-initdb.d/01_create_app_user.sh
Comment thread env.d/development/postgresql Outdated
Comment thread src/backend/core/migrations/0027_auto_20251120_0956.py
@lunika lunika force-pushed the postgres/migration branch 3 times, most recently from e61c67a to d4d5967 Compare May 13, 2026 06:17
lunika added 2 commits May 13, 2026 08:18
In development environment we wanto tu use the application with a
postgres user not having superuser role. The migration 0027 needs
superuser role to be executed, so we want to be able to test migrations
and other scenarios with a normal user. In order to create this user,
the compose service must be deleted first and then recreated with
DB_USER and DB_PASSWORD environment variable values different with
POSTGRES_USER and POSTGRES_PASSWORD
The migration 00227 must be run with a postgres superuser, most af
managed postgresql database can not be run using this kind of user.
Ti fix this, we use postgresql unnacent function instead of accessing C
function.
@lunika lunika force-pushed the postgres/migration branch from d4d5967 to abd03d1 Compare May 13, 2026 06:18
@lunika lunika merged commit abd03d1 into main May 13, 2026
42 of 43 checks passed
@lunika lunika deleted the postgres/migration branch May 13, 2026 06:56
@AntoLC AntoLC mentioned this pull request Jun 3, 2026
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.

Migration 27 prevents deployments on managed databases

1 participant