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

Hi,

On 2019-02-08 18:30:18 +0300, Alexey Kondratov wrote:
> From 99c6d94f37a797400d41545a271ff111b92e9361 Mon Sep 17 00:00:00 2001
> From: Alexey Kondratov <alex.lumir@gmail.com>
> Date: Fri, 21 Dec 2018 14:00:30 +0300
> Subject: [PATCH] pg_rewind: options to use restore_command from
>  postgresql.conf or command line.
> 
> ---
>  doc/src/sgml/ref/pg_rewind.sgml               |  30 +-
>  src/backend/Makefile                          |   4 +-
>  src/backend/commands/extension.c              |   1 +
>  src/backend/utils/misc/.gitignore             |   1 -
>  src/backend/utils/misc/Makefile               |   8 -
>  src/backend/utils/misc/guc.c                  | 434 +++++++++++++--
>  src/bin/pg_rewind/Makefile                    |   2 +-
>  src/bin/pg_rewind/parsexlog.c                 | 166 +++++-
>  src/bin/pg_rewind/pg_rewind.c                 | 100 +++-
>  src/bin/pg_rewind/pg_rewind.h                 |  10 +-
>  src/bin/pg_rewind/t/001_basic.pl              |   4 +-
>  src/bin/pg_rewind/t/002_databases.pl          |   4 +-
>  src/bin/pg_rewind/t/003_extrafiles.pl         |   4 +-
>  src/bin/pg_rewind/t/RewindTest.pm             |  93 +++-
>  src/common/.gitignore                         |   1 +
>  src/common/Makefile                           |   9 +-
>  src/{backend/utils/misc => common}/guc-file.l | 518 ++++--------------
>  src/include/common/guc-file.h                 |  50 ++
>  src/include/utils/guc.h                       |  39 +-
>  src/tools/msvc/Mkvcbuild.pm                   |   7 +-
>  src/tools/msvc/clean.bat                      |   2 +-
>  21 files changed, 973 insertions(+), 514 deletions(-)
>  delete mode 100644 src/backend/utils/misc/.gitignore
>  rename src/{backend/utils/misc => common}/guc-file.l (60%)
>  create mode 100644 src/include/common/guc-file.h

As noted in a message of a few minutes ago, I'm very doubtful that the
approach of using guc-file.l like that is a good idea. But if we go for
that, that part of the patch *NEEDS* to be split into a separate
commit/patch. It's too hard to see functional changes otherwise.


> @@ -291,9 +299,46 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
>  
>  		if (xlogreadfd < 0)
>  		{
> -			printf(_("could not open file \"%s\": %s\n"), xlogfpath,
> -				   strerror(errno));
> -			return -1;
> +			bool  restore_ok;
> +
> +			/*
> +			 * If we have no restore_command to execute, then exit.
> +			 */
> +			if (private->restoreCommand == NULL)
> +			{
> +				printf(_("could not open file \"%s\": %s\n"), xlogfpath,
> +					   strerror(errno));
> +				return -1;
> +			}
> +
> +			/*
> +			 * Since we have restore_command to execute, then try to retreive
> +			 * missing WAL file from the archive.
> +			 */
> +			restore_ok = RestoreArchivedWAL(private->datadir,
> +											xlogfname,
> +											WalSegSz,
> +											private->restoreCommand);
> +
> +			if (restore_ok)
> +			{
> +				xlogreadfd = open(xlogfpath, O_RDONLY | PG_BINARY, 0);
> +
> +				if (xlogreadfd < 0)
> +				{
> +					printf(_("could not open restored from archive file \"%s\": %s\n"), xlogfpath,
> +							strerror(errno));
> +					return -1;
> +				}
> +				else
> +					pg_log(PG_DEBUG, "using restored from archive version of file \"%s\"\n", xlogfpath);
> +			}
> +			else
> +			{
> +				printf(_("could not restore file \"%s\" from archive: %s\n"), xlogfname,
> +					   strerror(errno));
> +				return -1;
> +			}
>  		}
>  	}

I suggest moving this to a separate function.




> +	if (restore_command != NULL)
> +	{
> +		if (restore_wals)
> +		{
> +			fprintf(stderr, _("%s: conflicting options: both -r and -R are specified\n"),
> +				progname);
> +			fprintf(stderr, _("You must run %s with either -r/--use-postgresql-conf or -R/--restore-command.\n"),
> +				progname);
> +			exit(1);
> +		}
> +
> +		pg_log(PG_DEBUG, "using command line restore_command=\'%s\'.\n", restore_command);
> +	}
> +	else if (restore_wals)
> +	{
> +		FILE	*conf_file;
> +
> +		/*
> +		 * Look for configuration file in the target data directory and
> +		 * try to get restore_command from there.
> +		 */
> +		snprintf(recfile_fullpath, sizeof(recfile_fullpath), "%s/%s", datadir_target, RESTORE_COMMAND_FILE);
> +		conf_file = fopen(recfile_fullpath, "r");
> +
> +		if (conf_file == NULL)
> +		{
> +			fprintf(stderr, _("%s: option -r/--use-postgresql-conf is specified, but postgreslq.conf is absent in the target directory\n"),
> +					progname);
> +			fprintf(stderr, _("You have to add postgresql.conf or pass restore_command with -R/--restore-command option.\n"));
> +			exit(1);
> +		}
> +		else
> +		{
> +			ConfigVariable *item,
> +						   *head = NULL,
> +						   *tail = NULL;
> +			bool			config_is_parsed;
> +
> +			/*
> +			 * We pass a fullpath to the configuration file as calling_file here, since
> +			 * parser will use its parent directory as base for all further includes
> +			 * if any exist.
> +			 */
> +			config_is_parsed = ParseConfigFile(RESTORE_COMMAND_FILE, true,
> +											   recfile_fullpath, 0, 0,
> +											   PG_WARNING, &head, &tail);
> +			fclose(conf_file);
> +
> +			if (config_is_parsed)
> +			{
> +				for (item = head; item; item = item->next)
> +				{
> +					if (strcmp(item->name, "restore_command") == 0)
> +					{
> +						if (restore_command != NULL)
> +						{
> +							pfree(restore_command);
> +							restore_command = NULL;
> +						}
> +						restore_command = pstrdup(item->value);
> +						pg_log(PG_DEBUG, "using restore_command=\'%s\' from %s.\n", restore_command, RESTORE_COMMAND_FILE);
> +					}
> +				}
> +
> +				if (restore_command == NULL)
> +					pg_fatal("could not find restore_command in %s file %s\n", RESTORE_COMMAND_FILE, recfile_fullpath);
> +			}
> +			else
> +				pg_fatal("could not parse %s file %s\n", RESTORE_COMMAND_FILE, recfile_fullpath);
> +
> +			FreeConfigVariables(head);
> +		}
> +	}
> +

Isn't this entirely broken? restore_command could be set in a different
file no?


Greetings,

Andres Freund