Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

Hi,

On 2019-02-18 16:26:55 +0300, Alexey Kondratov wrote:
> Hi Andres,
> 
> Thank you for your feedback.
> 
> On 16.02.2019 6:41, Andres Freund wrote:
> > It sounds like a seriously bad idea to use a different parser for
> > pg_rewind.  Why don't you just use postgres for it? As in
> > /path/to/postgres -D /path/to/datadir/ -C shared_buffers
> > ?
> 
> 
> Initially, when I started working on this patch, recovery options were not a
> part of GUCs, so it was not possible. Now, recovery.conf is a part of
> postgresql.conf and postgres -C only reads config files, initializes GUCs,
> prints required parameter and shuts down. Thus, it seems like an acceptable
> solution for me. Though I am still a little bit afraid to start up a server,
> which is meant to be shut down during rewind process, even for such a short
> period of time.

-C doesn't really start the server.


> The only thing I am concerned most about is that pg_rewind always has been a
> standalone utility, so you were able to simply rewind two separated data
> directories one relatively another without any need for other postgres
> binaries. If we rely on postgres -C this would be tricky in some cases:

We don't generally support that. I don't see why this is something we
ought to invest significant effort into. It's not like you meaningfully
can use pg_rewind on a server without postgres installed.


> Anyway, currently I do not use a different parser for pg_rewind. A few
> versions back I have made guc-file.l common for frontend/backend. So
> technically speaking it is the same parser as postmaster use, only small
> number of sophisticated error reporting is wrapped with IFDEF.

Meh, there's significant parsing related logic, including dealing with
multiple values, that you've excluded. And you've copied substantial
amounts of code to make your approach possible.


> > Isn't this entirely broken? restore_command could be set in a different
> > file no?
> 
> Maybe I got it wrong, but I do not think so. Since recovery options are now
> a part of GUCs, restore_command may be only set inside postgresql.conf or
> any files/subdirs which are included there to take an effect, isn't it?
> Parser will walk postgresql.conf with all includes recursively and should
> eventually find it, if it was set.

Note that e.g. .auto.conf is loaded explicitly:
	/*
	 * Parse the PG_AUTOCONF_FILENAME file, if present, after the main file to
	 * replace any parameters set by ALTER SYSTEM command.  Because this file
	 * is in the data directory, we can't read it until the DataDir has been
	 * set.
	 */
	if (DataDir)
	{
		if (!ParseConfigFile(PG_AUTOCONF_FILENAME, false,
							 NULL, 0, 0, elevel,
							 &head, &tail))
		{
			/* Syntax error(s) detected in the file, so bail out */
			error = true;
			ConfFileWithError = PG_AUTOCONF_FILENAME;
			goto bail_out;
		}
	}

- Andres