Re: fixing pg_ctl with relative paths

On January 23, 2013 9:13 AM Josh Kupershmidt wrote:
>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.
>
>[1]
http://www.postgresql.org/message-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C
9v60YbmMmZUQ(at)mail(dot)gmail(dot)com
>[2]
http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6Gts
RVm-LtfWfW=g(at)mail(dot)gmail(dot)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

Please find the review of the patch.

Basic stuff:
------------
- Patch applies OK
- Compiles cleanly with no warnings
- Regression tests pass.

What it does:
-------------
The restart functionality of pg_ctl has problems with relative paths. This
patch removes the
problems arising during restart. This patch strips the data directory which
is present in the
postmaster options and keep the rest of the options already provided incase
if user not provided
any options during restart.

Code Review:
------------
+if (orig_post_opts) {
+ post_opts = strip_datadirs(orig_post_opts);
+}

No need of "{}" as the only one statement block is present in the if block.

+ free(tmp);

The above statement can be moved inside the if (*(trailing_quote + 1) !=
'\0') {
where it's exact usage is present.

Testing:
--------
I tested this feature with different postmaster options and database folder
names, found no problem.

The database folder with quotes present in it is any way having problems
with pg_ctl.
I feel the strip_datadirs() function header explanation is providing good
understanding.
Overall the patch is good. It makes the pg_ctl restart functionality works
well.

Regards,
Hari babu