POSTGRES_XLOG_DIR to specify transaction log dir #224

DanielDent · 2016-11-20T07:36:34Z

Adds support for the POSTGRES_XLOG_DIR environment variable, which specifies where the postgres transaction log is stored.

For some use cases, being able to place the transaction log on a different volume is useful.

(Existing support for providing flags via $POSTGRES_INITDB_ARGS is inadequate because of the need to create and chown/chmod the directory prior to running initdb)

DanielDent · 2016-11-25T21:34:42Z

I just rebased on your latest changes, tests still pass. Please let me know if there is anything I can do to help get this merged.

tianon · 2016-11-28T23:33:04Z

When it's used, isn't this directory something the user ought to be persisting as well?

DanielDent · 2016-11-28T23:41:08Z

Definitely. I didn't add another VOLUME because VOLUMEs can't be undefined and not everyone will want multiple VOLUMEs for their data.

Additionally, users don't necessarily need another VOLUME to use this.

A user could define PGDATA to /var/lib/postgresql/data/pg_data, and POSTGRES_XLOG_DIR to /var/lib/postgresql/data/pg_xlog, and both would be persisted within that volume.

But it also gives them the option to attach different volumes to these mount points, or to volumes specified at runtime. The transaction log and the data files have different performance requirements/optimizations, and it's useful to be able to put them on mountpoints which take advantage of the required characteristics.

tianon · 2016-11-28T23:47:57Z

So in the case of the xlogdir being a subdirectory of $PGDATA, permissions are already covered by the existing code, right?

I'm kind of inclined to put the permissions responsibility for this in the hands of the user, since it's already possible to do so today without explicit support for this functionality. To put it another way, if we accept this knob for this directory/flag, which initdb or postgres flag is going to come next?

DanielDent · 2016-11-29T00:29:10Z

The reason I created this fork/pull request is that I didn't find a way to have this work without modifying the upstream image.

I initially tried having xlogdir as a subdirectory of $PGDATA. Postgres refuses to allow initdb to occur unless the data directory is empty. This creates problems if you are trying to mount a different volume onto that directory, because then the xlogdir directory exists and it's not an empty directory.

I certainly understand the general concern about not wanting to cover every permutation of every use case. That's where things like POSTGRES_INITDB_ARGS allow users to customize the image for themselves at runtime.

Once users start having to manually fiddle with permissions just to launch a container, docker ceases to be something which removes administrative burden in the way it ought to, and I don't see a way around it in this case.

DanielDent · 2017-02-28T01:03:34Z

@tianon I just rebased again.

I understand the desire to not have complicated support for every potential permutation of how Postgres is used, but I'd also point out that creating directories/setting permissions automatically so postgres can start without intervention does seem to be something that is seen as worth doing already - it's just that the current code doesn't work if you are using a custom transaction log directory.

If my commit can be merged as-is, yay! Or if you have alternative suggestions for how I could support this use case, or a more generalized merge-worthy approach I could implement which would cover my use case, I would be pleased to work towards having something which is merge-worthy.

tianon · 2017-02-28T19:31:19Z

docker-entrypoint.sh

+        chmod 700 "$POSTGRES_XLOG_DIR"
+        chown -R postgres "$POSTGRES_XLOG_DIR"
+        POSTGRES_INITDB_ARGS="$POSTGRES_INITDB_ARGS --xlogdir $POSTGRES_XLOG_DIR"
+    fi


Thanks for your patience and good-nature, @DanielDent 😞 ❤️

I'm not 100% sold, but I concede that we probably need something for this. Here are my thoughts on the implementation:

I'd like a blank space above the comment (so that the /var/run/postgresql lines are separated from these)

I'd rather name the variable POSTGRES_INITDB_XLOGDIR so it makes it more clear what it interacts with (and maps directly to the option it enables)

the POSTGRES_INITDB_ARGS="$POSTGRES_INITDB_ARGS ..." line should start with export,
ie export POSTGRES_INITDB_ARGS="$POSTGRES_INITDB_ARGS ..."

the four-space indent you've got should be tabs (matching the rest of the file) 👍

Happy to take over for any of this if you'd prefer, but want to give you first dibs (and a chance to converse if what I've proposed doesn't make sense or isn't accurate 😄) ❤️

DanielDent · 2017-02-28T21:31:32Z

Great. I've incorporated your proposed changes.

Additionally, I reworked things slightly to support secrets (file_env), which did not exist when I originally made these changes. I can revert that if need be - I suspect the desired interface is 'all environment variables can also be provided as file secrets' but I haven't read a style guide on the subject.

tianon

Good call on including file_env! 👍

Just a couple minor comments and I think we're good to go!

tianon · 2017-02-28T21:48:27Z

docker-entrypoint.sh

@@ -47,7 +57,6 @@ if [ "$1" = 'postgres' ]; then

 	# look specifically for PG_VERSION, as it is expected in the DB dir
 	if [ ! -s "$PGDATA/PG_VERSION" ]; then
-		file_env 'POSTGRES_INITDB_ARGS'


This one needs to stay here in case we didn't hit the [ "$(id -u)" = '0' ] block for the above if (it should be harmless to duplicate it -- the function takes care of making sure multi-invocation is safe).

tianon · 2017-02-28T21:48:45Z

docker-entrypoint.sh

@@ -37,6 +37,16 @@ if [ "$1" = 'postgres' ] && [ "$(id -u)" = '0' ]; then
 	chown -R postgres /var/run/postgresql
 	chmod g+s /var/run/postgresql

+    # Create the transaction log directory before initdb is run (below) so the directory is owned by the correct user


This line appears to still be space-indented. 😄

Adds support for the POSTGRES_INITDB_XLOGDIR environment variable, which specifies where the postgres transaction log is stored. For some use cases, being able to place the transaction log on a different volume is useful. Existing support for providing flags via $POSTGRES_INITDB_ARGS is inadequate because of the need to create and chown/chmod the directory prior to running initdb.

DanielDent · 2017-02-28T22:10:00Z

Thanks. Re-pushed.

DanielDent · 2017-03-19T07:21:44Z

@tianon ping

… to where it'll be invoked regardless of our "root" status

tianon · 2017-03-20T17:09:51Z

Sorry for the delay @DanielDent; I thought of another edge case (users who use --user rather than our auto-step-down code), so I updated the PR to include that. 👍

tianon · 2017-03-20T17:10:09Z

Thanks for your patience!

- `memcached`: 1.4.36 - `postgres`: add `POSTGRES_INITDB_XLOGDIR` variable for `--xlogdir` during `initdb` (docker-library/postgres#224) - `pypy`: 5.7.0

docker-library/postgres#224 and docker-library/official-images#2762 added the POSTGRES_XLOG_DIR variable. This adds documentation for that variable.

docker-library/postgres#224 and docker-library/official-images#2762 added the POSTGRES_INITDB_XLOGDIR variable. This adds documentation for that variable.

DanielDent force-pushed the master branch from 6d48d68 to cc78dca Compare November 25, 2016 21:22

DanielDent force-pushed the master branch from cc78dca to 617b1d7 Compare February 28, 2017 00:50

DanielDent force-pushed the master branch from 617b1d7 to 34c02de Compare February 28, 2017 06:25

tianon reviewed Feb 28, 2017

View reviewed changes

DanielDent force-pushed the master branch 2 times, most recently from 513274c to 9d0b63f Compare February 28, 2017 21:26

DanielDent force-pushed the master branch from 9d0b63f to a6ba86a Compare February 28, 2017 21:33

tianon requested changes Feb 28, 2017

View reviewed changes

DanielDent force-pushed the master branch from a6ba86a to 89530f6 Compare February 28, 2017 22:06

yosifkit approved these changes Mar 1, 2017

View reviewed changes

Update order to match other blocks and move "--xlogdir" handling down…

913bc48

… to where it'll be invoked regardless of our "root" status

tianon force-pushed the master branch from e182f47 to 913bc48 Compare March 20, 2017 16:54

tianon approved these changes Mar 20, 2017

View reviewed changes

tianon merged commit 123aedc into docker-library:master Mar 20, 2017

tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 20, 2017

Update docker-library images

f14bb8b

- `memcached`: 1.4.36 - `postgres`: add `POSTGRES_INITDB_XLOGDIR` variable for `--xlogdir` during `initdb` (docker-library/postgres#224) - `pypy`: 5.7.0

tianon mentioned this pull request Mar 20, 2017

Update docker-library images docker-library/official-images#2762

Merged

DanielDent mentioned this pull request Apr 6, 2017

Document postgres POSTGRES_INITDB_XLOGDIR variable docker-library/docs#872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

POSTGRES_XLOG_DIR to specify transaction log dir #224

POSTGRES_XLOG_DIR to specify transaction log dir #224

Uh oh!

DanielDent commented Nov 20, 2016

Uh oh!

DanielDent commented Nov 25, 2016

Uh oh!

tianon commented Nov 28, 2016

Uh oh!

DanielDent commented Nov 28, 2016

Uh oh!

tianon commented Nov 28, 2016

Uh oh!

DanielDent commented Nov 29, 2016 •

edited

Loading

Uh oh!

DanielDent commented Feb 28, 2017

Uh oh!

tianon Feb 28, 2017

Uh oh!

DanielDent commented Feb 28, 2017 •

edited

Loading

Uh oh!

tianon left a comment

Uh oh!

tianon Feb 28, 2017

Uh oh!

tianon Feb 28, 2017

Uh oh!

DanielDent commented Feb 28, 2017

Uh oh!

DanielDent commented Mar 19, 2017

Uh oh!

tianon commented Mar 20, 2017

Uh oh!

tianon commented Mar 20, 2017

Uh oh!

Uh oh!

POSTGRES_XLOG_DIR to specify transaction log dir #224

POSTGRES_XLOG_DIR to specify transaction log dir #224

Uh oh!

Conversation

DanielDent commented Nov 20, 2016

Uh oh!

DanielDent commented Nov 25, 2016

Uh oh!

tianon commented Nov 28, 2016

Uh oh!

DanielDent commented Nov 28, 2016

Uh oh!

tianon commented Nov 28, 2016

Uh oh!

DanielDent commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielDent commented Feb 28, 2017

Uh oh!

tianon Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

DanielDent commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

tianon Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

DanielDent commented Feb 28, 2017

Uh oh!

DanielDent commented Mar 19, 2017

Uh oh!

tianon commented Mar 20, 2017

Uh oh!

tianon commented Mar 20, 2017

Uh oh!

Uh oh!

DanielDent commented Nov 29, 2016 •

edited

Loading

DanielDent commented Feb 28, 2017 •

edited

Loading