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(dot)lumir(at)gmail(dot)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