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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, vladimirlesk(at)yandex-team(dot)ru, dsarafan(at)yandex-team(dot)ru
Subject: Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line
Date: 2019-02-18 13:26:55
Message-ID: 8540fcbc-fb91-35be-1025-0b7343b2c19a@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v3-0002-Options-to-use-restore_command-with-pg_rewind.patch text/x-patch 21.2 KB
v3-0001-Make-guc-file.l-common-for-frontend-and-backend.patch text/x-patch 46.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Surafel Temesgen 2019-02-18 13:33:53 Re: pg_dump multi VALUES INSERT
Previous Message Michael Banck 2019-02-18 13:22:42 Re: Progress reporting for pg_verify_checksums