Re: Bug with pg_ctl -w/wait and config-only directories

Robert Haas wrote:
> On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > OK, here is a patch that adds a -C option to the postmaster so any
> > config variable can be dumped, even while the server is running (there
> > is no security check because we don't have a user name at this point),
> > e.g.:
> >
> > ? ? ? ?postgres -D /pg_upgrade/tmp -C data_directory
> > ? ? ? ?/u/pg/data
> 
> It seems both ugly and unnecessary to declare dump_config_variable as
> char[MAXPGPATH].  MAXPGPATH doesn't seem like the right length for a
> buffer intended to hold a GUC name, but in fact I don't think you need
> a buffer at all.  I think you can just declare it as char * and say
> dump_config_variable = optarg. getopt() doesn't overwrite the input
> argument vector, does it?

Well, as I remember, it writes a null byte at the end of the argument
and then passes the pointer to the start --- when it advances to the
next argument, it removes the null, so the pointer might still be valid,
but does not have proper termination (or maybe that's what strtok()
does).  However, I can find no documentation that mentions this
restriction, so perhaps it is old and no longer valid.

If you look in our code you will see tons of cases of:

	user = strdup(optarg);
	pg_data = xstrdup(optarg);
	my_opts->dbname = mystrdup(optarg);

However, I see other cases where we just assign optarg and optarg is a
string, e.g. pg_dump:

	username = optarg;

Doing a Google search shows both types of coding in random code pieces.

Are the optarg string duplication calls unnecessary and can be removed?
Either that, or we need to add some.

> Also, I think you should remove this comment:
> 
> + 		/* This allows anyone to read super-user config values. */
> 
> It allows anyone to read super-user config values *who can already
> read postgresql.conf*.  Duh.

Oh, very good point --- I had not realized that.  Comment updated.

> > It also modifies pg_ctl to use this feature. ?It works fine for pg_ctl
> > -w start/stop with a config-only directory, so this is certainly in the
> > right direction. ?You can also use pg_ctl -o '--data_directory=/abc' and
> > it will be understood:
> >
> > ? ? ? ?pg_ctl -o '--data_directory=/u/pg/data' -D tmp start
> >
> > If you used --data_directory to start the server, you will need to use
> > --data_directory to stop it, which seems reasonable.
> 
> Yep.  I think that when adjust_data_dir() changes pg_data it probably
> needs to call canonicalize_path() on the new value.

Done.

> > Patch attached. ?This was much simpler than I thought. ?:-)
> 
> Yes, this looks pretty simple.

What really saved my bacon was the fact that the -o parameters are
passed into the backend and processed by the backend, rather than pg_ctl
having to read through that mess and parse it.  Doing that logic was
going to be very hard and unlikely to be back-patch-able.

Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +