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

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.

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:

- end user should always care about postmaster binaries availability;
- even so, appropriate postgres executable may be absent in the ENV/PATH;
- locations of pg_rewind and postgres may be arbitrary depending on the 
distribution, which may be custom as well.

I cannot propose a reliable way of detecting path to postgres executable 
without directly asking users to provide it via PATH, command line 
option, etc. If someone can suggest anything, then it would be possible 
to make patch simpler in some way, but I always wanted to keep pg_rewind 
standalone and as simple as possible for end users.

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.

> 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.

Yes, sure, please find attached new version of the patch set consisting 
of two separated patches. First is for making guc-file.l common between 
frontend/backend and second one is for adding new options into pg_rewind.

> +            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.

OK, I have slightly refactored and simplified this part. All checks of 
the recovered file have been moved into RestoreArchivedWAL. Hope it 
looks better now.

> 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.



Regards

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company