Re: [Patch] pg_rewind: options to use restore_command fromrecovery.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
thetarget 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