Skip to content

Conversation

Re4zOon
Copy link

@Re4zOon Re4zOon commented Jul 23, 2025

Due to the function being called from create_database, the IFS is already set to , If we then update IFS_ORG to IFS, we lose the original IFS globally.
Found this while trying to add max_connection variable to our postgres as command.

This is a dirty fix, should be improved properly, as its not reusable really (sorry, its midnight here, took two hours to find the reason :D).

Due to the function being called from create_database, the IFS is already set to ,
If we then update IFS_ORG to IFS, we lose the original IFS globally.

This is a dirty fix, should be improved properly, as its not reusable really (sorry, its midnight here, took two hours to find the reason :D).
@Re4zOon
Copy link
Author

Re4zOon commented Jul 23, 2025

FYI in debug logs its visible:

postgresql-1  | + [[ -n gitlabhq_production ]]
postgresql-1  | + case $REPLICATION_MODE in
postgresql-1  | + IFS_ORG='          <-------------- Saving in create_database (line 337)
postgresql-1  | '
postgresql-1  | + IFS=,
postgresql-1  | + for database in ${DB_NAME}
postgresql-1  | + echo 'Creating database: gitlabhq_production...'
postgresql-1  | Creating database: gitlabhq_production...
postgresql-1  | ++ psql -U postgres -Atc 'SELECT 1 FROM pg_catalog.pg_database WHERE datname = '\''gitlabhq_production'\'''
postgresql-1  | ‣ Granting access to gitlab user...
postgresql-1  | + [[ -z 1 ]]
postgresql-1  | + load_extensions gitlabhq_production      <-------------- Going into extension load function (line 345)
postgresql-1  | + local database=gitlabhq_production
postgresql-1  | + [[ '' == true ]]
postgresql-1  | + IFS_ORG=,              <-------------- Overriding IFS_ORG with the current (already modifies) IFS (line 321)
postgresql-1  | + IFS=,
postgresql-1  | + IFS=,

@kkimurak
Copy link
Owner

Thank you for fix, I'll check and reflect this in this week's release, scheduled for about 10 hours from now.

@@ -318,13 +318,13 @@ load_extensions() {
psql -U ${PG_USER} -d ${database} -c "CREATE EXTENSION IF NOT EXISTS unaccent;" >/dev/null 2>&1
fi

IFS_ORG=$IFS
IFS_ORG2=$IFS
Copy link
Owner

@kkimurak kkimurak Jul 24, 2025

Choose a reason for hiding this comment

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

How about making IFS_ORG function-local? local is not standardized in POSIX, but since bash is specified in the shebang, I think we should not need to worry about shells that cannot use local, or shells that can use it but have differences in behavior.

Suggested change
IFS_ORG2=$IFS
local IFS_ORG=$IFS

This should be also applied to IFS_ORG in create_database().

I made sure that this change properly restore original IFS at the end of create_database(). Trace:

$ docker run --rm -e "DB_NAME=testdb,testdb2" -e "DB_USER=db_user" -e "DB_USER_IS_DB_OWNER=true" -e "DB_EXTENSION=btree_gist,pg_trgm" -e "DB_PASS=password" -e "DEBUG=true" kkimurak/sameersbn-postgresql:latest 
(---- omitted ----)
+ create_database
+ [[ -n testdb,testdb2 ]]
+ case $REPLICATION_MODE in
+ local 'IFS_ORG=
' <-- Here IFS_ORG contains newline
+ IFS=,
+ for database in ${DB_NAME}
+ echo 'Creating database: testdb...'
Creating database: testdb...
++ psql -U postgres -Atc 'SELECT 1 FROM pg_catalog.pg_database WHERE datname = '\''testdb'\'''
+ [[ -z '' ]]
+ psql -U postgres -c 'CREATE DATABASE "testdb" WITH TEMPLATE = "template1";'
+ load_extensions testdb
+ local database=testdb
+ [[ '' == true ]]
+ local IFS_ORG=,
+ IFS=,
(---- omitted ----)
‣ Setting db_user as an owner of database testdb2...
+ IFS='
+ IFS='
' <-- IFS restored to contain newline at the end of create_database()
+ create_replication_user
(---- omitted ----)
Starting PostgreSQL 16...

Copy link
Author

Choose a reason for hiding this comment

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

Yep, this makes more sense :D
As said, it was already midnight and you can imagine it wasn't 10 minutes to debug this error from scratch.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks again for taking time. Also, I've included this in the pull request sameersbn#175 I submitted to the upstream project (sameersbn@420ce73), so please let me know if you have any issues.

@kkimurak
Copy link
Owner

I'll change it to use local for now, merge it, and include it in this week's release. Please let us know if you have any issues.
Thank you for your contribution!

@kkimurak kkimurak merged commit 6e50101 into kkimurak:develop Jul 24, 2025
kkimurak added a commit that referenced this pull request Jul 31, 2025
See pull request #11

* Fix IFS parsing (Re4zOon)
* remove empty line (Re4zOon)
* Make temporary variable `IFS_ORG` function-local (kkimurak)

Co-authored-by: Re4zOon <[email protected]>
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