Skip to content

more sane way of starting and stopping postgres server using pg_ctl #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 21, 2015
Merged

more sane way of starting and stopping postgres server using pg_ctl #80

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d49499c
more sane way of starting and stopping postgres server using pg_ctl a…
renzok Aug 4, 2015
0ffbeb7
fixed missing quotes and removed curly braces according to discussion
renzok Aug 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 8 additions & 22 deletions docker-entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ set_listen_addresses() {
}

if [ "$1" = 'postgres' ]; then
mkdir -p "$PGDATA"
chown -R postgres "$PGDATA"
mkdir -p "${PGDATA}"
chown -R postgres "${PGDATA}"
Copy link
Member

Choose a reason for hiding this comment

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

The curly braces are unnecessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, habit of mine and my editor highlights them the better, will take that back


chmod g+s /run/postgresql
chown -R postgres /run/postgresql
Expand Down Expand Up @@ -44,21 +44,11 @@ if [ "$1" = 'postgres' ]; then

{ echo; echo "host all all 0.0.0.0/0 $authMethod"; } >> "$PGDATA/pg_hba.conf"

set_listen_addresses '' # we're going to start up postgres, but it's not ready for use yet (this is initialization), so don't listen to the outside world yet

gosu postgres "$@" &
pid="$!"
for i in {30..0}; do
if echo 'SELECT 1' | psql --username postgres &> /dev/null; then
break
fi
echo 'PostgreSQL init process in progress...'
sleep 1
done
if [ "$i" = 0 ]; then
echo >&2 'PostgreSQL init process failed'
exit 1
fi
# internal start of server in order to allow set-up using psql-client
gosu postgres pg_ctl -D ${PGDATA} \
Copy link
Member

Choose a reason for hiding this comment

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

Curly braces unnecessary, but does need quotes, "$PGDATA".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will change

Copy link

Choose a reason for hiding this comment

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

Just FYI; from Google's Shell Style Guide:

prefer "${var}" over "$var"

Copy link
Member

Choose a reason for hiding this comment

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

"but see details" --> where are the details? 😞

-o "-c listen_addresses=''" \
-w start # does not listen on TCP/IP and wait
# until start finished
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace here needs to be made consistently tabs, too. 👍


Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep this or a similar test to make sure it is ready to run some SQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is not necessary because as far as I understand pg_ctl will only exit 0 if connection is possible. Moreover, later we do psql anyway i.e. if connection not possible the next psql will exit > 0 anyway.

: ${POSTGRES_USER:=postgres}
: ${POSTGRES_DB:=$POSTGRES_USER}
Expand Down Expand Up @@ -91,11 +81,7 @@ if [ "$1" = 'postgres' ]; then
echo
done

if ! kill -s TERM "$pid" || ! wait "$pid"; then
echo >&2 'PostgreSQL init process failed'
exit 1
fi

gosu postgres pg_ctl -D ${PGDATA} -m fast -w stop
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as line 48.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also pg_ctl will exit non-zero. Indeed, I forgot to catch this e.g. by

gosu postgres pg_ctl -D ${PGDATA} -m fast -w stop || {echo >&2 'PostgreSQL init process failed'; exit 1}

Copy link
Member

Choose a reason for hiding this comment

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

IMO if pg_ctl has a decent error message when it fails, we don't need to do anything except let set -e do its thing.

set_listen_addresses '*'
Copy link
Member

Choose a reason for hiding this comment

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

Since this function is now only used once, we can just put the seds here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, I would keep the function. however, an alternative is to supply the listen_address as option to pg_ctl. That would even render seds obsolote.


echo
Expand Down