fixing pg_ctl with relative paths

From: Josh Kupershmidt <schmiddy(at)gmail(dot)com>
To: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: fixing pg_ctl with relative paths
Date: 2013-01-23 03:42:54
Message-ID: CAK3UJRFK8-izAU1SMpNZr5Muc+6CRWBk0_7ByJnwUoapj3esFQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

There have been some complaints[1][2] in the past about pg_ctl not
playing nice with relative path specifications for the datadir. Here's
a concise illustration:

$ mkdir /tmp/mydata/ && initdb /tmp/mydata/
$ cd /tmp/
$ pg_ctl -D ./mydata/ start
$ cd /
$ pg_ctl -D /tmp/mydata/ restart

IMO it's pretty hard to defend the behavior of the last step, where
pg_ctl knows exactly which datadir the user has specified, and
succeeds in stopping the server but not starting it.

Digging into this gripe, a related problem I noticed is that `pg_ctl
... restart` doesn't always preserve the "-D $DATADIR" argument as the
following comment suggests it should[4]:

* We could pass PGDATA just in an environment
* variable but we do -D too for clearer postmaster
* 'ps' display

Specifically, if one passes in additional -o options, e.g.

$ pg_ctl -D /tmp/mydata/ -o "-N 10" restart

after which postmaster.opts will be missing the "-D ..." argument
which is otherwise recorded, and the `ps` output is similarly
abridged.

Anyway, Tom suggested[3] two possible ways of fixing the original
gripe, and I went with his latter suggestion,

| for pg_ctl restart to override that
| option with its freshly-derived idea of where the data directory is

mainly so we don't need to worry about the impact of changing the
appearance of postmaster.opts, plus it seems like this code fits
better in pg_ctl.c rather than the postmaster (where the
postmaster.opts file is actually written). The fix seems to be pretty
simple, namely stripping post_opts of the old "-D ... " portion and
having the new specification, if any, be used instead. I believe the
attached patch should fix these two infelicities.

Full disclosure: the strip_datadirs() function makes no attempt to
properly handle pathnames containing quotes. There seems to be some,
uh, confusion in the existing code about how these should be handled
already. For instance,

$ mkdir "/tmp/here's a \" quote"
$ initdb "/tmp/here's a \" quote"

How to successfully start, restart, and stop the server with pg_ctl is
left as an exercise for the reader. So I tried to avoid that can of
worms with this patch, though I'm happy to robustify strip_datadirs()
if we'd like to properly support such pathnames, and there's consensus
on how to standardize the escaping.

Josh

[1] http://www.postgresql.org/message-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C9v60YbmMmZUQ@mail.gmail.com
[2] http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g@mail.gmail.com
[3] http://www.postgresql.org/message-id/27233.1350234453@sss.pgh.pa.us
[4] Note, ps output and postmaster.opts will not include the datadir
specification if you rely solely on the PGDATA environment variable
for pg_ctl

Attachment Content-Type Size
pgctl_paths.v01.diff application/octet-stream 3.6 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2013-01-23 03:52:03 Re: Back-branch update releases coming in a couple weeks
Previous Message Amit Kapila 2013-01-23 03:40:42 Re: Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]