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

Hi, Alexey!

19 окт. 2018 г., в 22:49, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):
I expect, that it will be a good idea to allow pg_rewind to look for a restore_command

+1
Normally you do not expect huge progress on failed master. But you still can get a lot of WAL if you have network partition and scheduled tasks like pg_repack.
I'm not actually aware what kind of problems led to these but we were considering some automation to fetch WALs for failed master to improve rewinded\resetuped ratio.

I prepared a proof of concept patch (please, find attached), which does exactly what I described above. I played with it a little and it seems to be working, tests were accordingly updated to verify this archive retrieval functionality too.

Patch is relatively simple excepting the one part: if we want to parse recovery.conf (with all possible includes, etc.) and get restore_command, then we should use guc-file.l parser, which is heavily linked to backend, e.g. in error reporting part. So I copied it and made frontend-safe version guc-file-fe.l. Personally, I don't think it's a good idea, but nothing else came to mind. It is also possible to leave the only one option -- passing restore_command as command line argument.

I think it is better to load restore_command from recovery.conf.

I didn't actually try patch yet, but the idea seems interesting. Will you add it to the commitfest?

Best regards, Andrey Borodin.

Hi Andrey,

Thank you for your reply.

I think it is better to load restore_command from recovery.conf.

Yes, it seems to be the most native way. That's why I needed this
rewritten (mostly copy-pasted) frontend-safe version of parser (guc-file.l).

I didn't actually try patch yet, but the idea seems interesting. Will
you add it to the commitfest?

I am willing to add it to the November commitfest, but I have some
concerns regarding frontend version of GUC parser. Probably, it is
possible to refactor guc-file.l to use it on both front- and backend.
However, it requires usage of IFDEF and mocking up ereport for frontend,
which is a bit ugly.

--
Alexey Kondratov

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

On 2018-Oct-22, Alexey Kondratov wrote:

I didn't actually try patch yet, but the idea seems interesting. Will
you add it to the commitfest?

I am willing to add it to the November commitfest, but I have some concerns
regarding frontend version of GUC parser. Probably, it is possible to
refactor guc-file.l to use it on both front- and backend. However, it
requires usage of IFDEF and mocking up ereport for frontend, which is a bit
ugly.

Hmm, I remember we had a project to have a new postmaster option that
would report the value of some GUC option, so instead of parsing the
file in the frontend, you'd invoke the backend to do the parsing. But I
don't know what became of that ... I don't see it in the postmaster
--help output.

Of course, recovery.conf options are not GUCs either ... that's another
pending patch.

We do have some backend-mock for frontends, e.g. in pg_waldump; plus
palloc is already implemented in libpgcommon. I don't know if what you
need to compile the lexer is a project much bigger than finishing the
other two patches I mention.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 22.10.2018 20:19, Alvaro Herrera wrote:

I didn't actually try patch yet, but the idea seems interesting. Will
you add it to the commitfest?

I am willing to add it to the November commitfest, but I have some concerns
regarding frontend version of GUC parser. Probably, it is possible to
refactor guc-file.l to use it on both front- and backend. However, it
requires usage of IFDEF and mocking up ereport for frontend, which is a bit
ugly.

Hmm, I remember we had a project to have a new postmaster option that
would report the value of some GUC option, so instead of parsing the
file in the frontend, you'd invoke the backend to do the parsing. But I
don't know what became of that ...

Brief searching in the mailing list doesn't return something relevant,
but the project seems to be pretty straightforward at first sight.

Of course, recovery.conf options are not GUCs either ... that's another
pending patch.

We do have some backend-mock for frontends, e.g. in pg_waldump; plus
palloc is already implemented in libpgcommon. I don't know if what you
need to compile the lexer is a project much bigger than finishing the
other two patches I mention.

This thing, in opposite, is a long-living, there are several threads
starting from the 2011th. I have found Michael's, Simon's, Fujii's
patches and Greg Smith's proposal (see, e.g. [1, 2]). If I get it right,
the main point is that if we turn all options in the recovery.conf into
GUCs, then it becomes possible to set them inside postgresql.conf and
get rid of recovery.conf. However, it ruins backward compatibility and
brings some other issues noted by Heikki
/messages/by-id/5152F778.2070205@vmware.com, while
keeping both options is excess and ambiguous.

Thus, though everyone agreed that recovery.conf options should be turned
into GUCs, there is still no consensus in details. I don't think that I
know Postgres architecture enough to start this discussion again, but
thank you for pointing me in this direction, it was quite interesting
from the historical perspective.

I will check guc-file.l again, maybe it is not so painful to make it
frontend-safe too.

[1]: /messages/by-id/CAHGQGwHi=4GV6neLRXF7rexTBkjhcAEqF9_xq+tRvFv2bVd59w@mail.gmail.com
/messages/by-id/CAHGQGwHi=4GV6neLRXF7rexTBkjhcAEqF9_xq+tRvFv2bVd59w@mail.gmail.com

[2]: /messages/by-id/CA+U5nMKyuDxr0=5PSen1DZJndauNdz8BuSREau=ScN-7DZ9acA@mail.gmail.com
/messages/by-id/CA+U5nMKyuDxr0=5PSen1DZJndauNdz8BuSREau=ScN-7DZ9acA@mail.gmail.com

--
Alexey Kondratov

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

Hi, Alexey!

25 окт. 2018 г., в 17:37, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):

Will you add this patch to CF?
I'm going to review it.

Best regards, Andrey Borodin

On Mon, Oct 22, 2018 at 02:19:07PM -0300, Alvaro Herrera wrote:

Hmm, I remember we had a project to have a new postmaster option that
would report the value of some GUC option, so instead of parsing the
file in the frontend, you'd invoke the backend to do the parsing. But I
don't know what became of that ... I don't see it in the postmaster
--help output.

I did not recall this one.

Of course, recovery.conf options are not GUCs either ... that's another
pending patch.

But I do recall this one. Still isn't it q bit different? Because in
the case of pg_rewind let's remember that the origin cluster can be
offline or online, and that the target has to be offline. I am also not
sure that we would want to use the same command restore_command with
pg_rewind and an instance in recovery.

Something that we could think about is directly to provide a command to
pg_rewind via command line. Another possibility would be to have a
separate tool which scans a data folder and fetches by itself a range of
WAL segments wanted. I have implemented something like that for an
internal solution, able to handle as well timeline jumps across
segments with a server-side and a client-side implementation (that was
to allow a passive to catch up using a set of WAL segments, if the
active does not have a replication slot, and segments were fetched
through a Postgres instance which has a local archive).
--
Michael

Hi Andrey,

Will you add this patch to CF?
I'm going to review it.

Best regards, Andrey Borodin

Here it is https://commitfest.postgresql.org/20/1849/

--
Alexey Kondratov

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

Something that we could think about is directly to provide a command to
pg_rewind via command line.

In my patch I added this option too. One can pass restore_command via -R
option, e.g.:

pg_rewind -P --target-pgdata=/path/to/master/pg_data
--source-pgdata=/path/to/standby/pg_data -R 'cp /path/to/wals_archive/%f %p'

Another possibility would be to have a
separate tool which scans a data folder and fetches by itself a range of
WAL segments wanted.

Currently in the patch, with dry-run option (-n) pg_rewind only fetches
missing WALs to be able to build file map, while doesn't touch any data
files. So I guess it behaves exactly as you described and we do not need
a separate tool.

--
Alexey Kondratov

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

On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:

Currently in the patch, with dry-run option (-n) pg_rewind only fetches
missing WALs to be able to build file map, while doesn't touch any data
files. So I guess it behaves exactly as you described and we do not need a
separate tool.

Makes sense perhaps. Fetching only WAL segments which are needed for
the file map is critical, as you don't want to spend bandwidth for
nothing. Now, I look at your patch, and I can see things to complain
about, at least three at short glance:
- The TAP test added will fail on Windows.
- Simply copy-pasting RestoreArchivedWAL() from the backend code to
pg_rewind is not an acceptable option. You don't care about %r either
in this case.
- Reusing the GUC parser is something I would avoid as well. Not worth
the complexity.

Another thing I am wondering is: do we actually need something complex?
What we want to know is what data is necessary to build the file map, so
we could also add an option to pg_rewind which checks what segments are
necessary and lets the user know about them? This also avoids the
security-related problems of manipulating a command at option-level.
This kind of options makes folks willing to use more sensitive data on
command line, which is not always a good idea...
--
Michael

On 30.10.2018 06:01, Michael Paquier wrote:

On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:

Currently in the patch, with dry-run option (-n) pg_rewind only fetches
missing WALs to be able to build file map, while doesn't touch any data
files. So I guess it behaves exactly as you described and we do not need a
separate tool.

Makes sense perhaps. Fetching only WAL segments which are needed for
the file map is critical, as you don't want to spend bandwidth for
nothing. Now, I look at your patch, and I can see things to complain
about, at least three at short glance:
- The TAP test added will fail on Windows.

Thank you for this. Build on Windows has been broken as well. I fixed it
in the new version of patch, please find attached.

- Simply copy-pasting RestoreArchivedWAL() from the backend code to
pg_rewind is not an acceptable option. You don't care about %r either
in this case.

According to the docs [1]https://www.postgresql.org/docs/11/archive-recovery-settings.html %r is a valid alias and may be used in
restore_command too, so if we take restore_command from recovery.conf it
might be there. If we just drop it, then restore_command may stop
working. Though I do not know real life examples of restore_command with
%r, we should treat it in expected way (as backend does), of course if
we want an option to take it from recovery.conf.

- Reusing the GUC parser is something I would avoid as well. Not worth
the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend safe.

[1]: https://www.postgresql.org/docs/11/archive-recovery-settings.html

--
Alexey Kondratov

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

On Wed, Nov 7, 2018 at 10:58 AM Alexey Kondratov <a.kondratov@postgrespro.ru> wrote:

On 30.10.2018 06:01, Michael Paquier wrote:

On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:

Currently in the patch, with dry-run option (-n) pg_rewind only fetches
missing WALs to be able to build file map, while doesn't touch any data
files. So I guess it behaves exactly as you described and we do not need a
separate tool.

Makes sense perhaps. Fetching only WAL segments which are needed for
the file map is critical, as you don't want to spend bandwidth for
nothing. Now, I look at your patch, and I can see things to complain
about, at least three at short glance:
- The TAP test added will fail on Windows.

Thank you for this. Build on Windows has been broken as well. I fixed it
in the new version of patch, please find attached.

Just to confirm, patch still can be applied without conflicts, and pass all the
tests. Also I like the original motivation for the feature, sounds pretty
useful. For now I'm moving it to the next CF.

- Simply copy-pasting RestoreArchivedWAL() from the backend code to
pg_rewind is not an acceptable option. You don't care about %r either
in this case.

According to the docs [1] %r is a valid alias and may be used in
restore_command too, so if we take restore_command from recovery.conf it
might be there. If we just drop it, then restore_command may stop
working. Though I do not know real life examples of restore_command with
%r, we should treat it in expected way (as backend does), of course if
we want an option to take it from recovery.conf.

- Reusing the GUC parser is something I would avoid as well. Not worth
the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend safe.

Any success with that?

Hi Dmitry,

On 30.11.2018 19:04, Dmitry Dolgov wrote:

Just to confirm, patch still can be applied without conflicts, and pass all the
tests. Also I like the original motivation for the feature, sounds pretty
useful. For now I'm moving it to the next CF.

Thanks, although I have slightly updated patch to handle recent merge of
the recovery.conf into GUCs and postgresq.conf [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=2dedf4d9a899b36d1a8ed29be5efbd1b31a8fe85, new patch is attached.

- Reusing the GUC parser is something I would avoid as well. Not worth
the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend safe.

Any success with that?

I looked into it and found that currently guc-file.c is built as part of
guc.c, so it seems to be even more complicated to unbound guc-file.c
from backend. Thus, I have some plan of how to proceed with patch:

1) Add guc-file.h and build guc-file.c separately from guc.c

2) Put guc-file.l / guc-file.h into common/*

3) Isolate all backend specific calls in guc-file.l with #ifdef FRONTEND

Though I am not sure that this work is worth doing against extra
redundancy added by simply adding frontend-safe copy of guc-file.l
lexer. If someone has any thoughts I would be glad to receive comments.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=2dedf4d9a899b36d1a8ed29be5efbd1b31a8fe85
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=2dedf4d9a899b36d1a8ed29be5efbd1b31a8fe85

Regards

--
Alexey Kondratov

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

Greetings,

- Reusing the GUC parser is something I would avoid as well.  Not
worth
the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend
safe.

Any success with that?

I looked into it and found that currently guc-file.c is built as part
of guc.c, so it seems to be even more complicated to unbound
guc-file.c from backend. Thus, I have some plan of how to proceed with
patch:

1) Add guc-file.h and build guc-file.c separately from guc.c

2) Put guc-file.l / guc-file.h into common/*

3) Isolate all backend specific calls in guc-file.l with #ifdef FRONTEND

Though I am not sure that this work is worth doing against extra
redundancy added by simply adding frontend-safe copy of guc-file.l
lexer. If someone has any thoughts I would be glad to receive comments.

I have finally worked it out. Now there is a common version of
guc-file.l and guc-file.c is built separately from guc.c. I had to use a
limited number of #ifndef FRONTEND, mostly to replace erreport calls.
Also, ProcessConfigFile and ProcessConfigFileInternal have been moved
inside guc.c explicitly as being a backend specific. So for me this
solution looks much more concise and neat.

Please, find the new version of patch attached. Tap tests have been
updated as well in order to handle both command line and postgresql.conf
specified restore_command.

Regards

--
Alexey Kondratov

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

Hi!

Thanks for working on this feature, I believe it solves actual problem of HA systems.

30 окт. 2018 г., в 8:01, Michael Paquier <michael@paquier.xyz> написал(а):

Another thing I am wondering is: do we actually need something complex?
What we want to know is what data is necessary to build the file map, so
we could also add an option to pg_rewind which checks what segments are
necessary and lets the user know about them?

From my point of view fetching WALs automatically is much better option for automation.

This also avoids the
security-related problems of manipulating a command at option-level.
This kind of options makes folks willing to use more sensitive data on
command line, which is not always a good idea...

I do not see any new security problems here.. I'd be happy if anyone pointed me out where I can learn about them.

26 дек. 2018 г., в 19:11, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):

Please, find the new version of patch attached.

The refactoring of guc-file looks sane, but I'm not an expert in frontend\backend modularity.

Here are some my notes:
1. RestoreArchivedWAL() shares a lot of code with RestoreArchivedFile(). Is it possible\viable to refactor and extract common part?
2. IMV pg_rewind with %r restore_command should fail. %r is designed to clean archive from WALs, nothing should be deleted in case of fetching WALs for rewind. Last restartpoint has no meaning during rewind. Or does it? If so, let's comment about it.
3. RestoreArchivedFile() checks for signals, is it done by pg_rewind elsewhere?
4. No documentation is updated
5. -R takes precedence over -r without notes. Shouldn't we complain? Or may be we should take one from config, iif nothing found use -R?

Thanks!

Best regards, Andrey Borodin.

Hi Andrey,

Thank you for the review! I have updated the patch according to your
comments and remarks. Please, find new version attached.

On 2019-01-07 12:12, Andrey Borodin wrote:

Here are some my notes:
1. RestoreArchivedWAL() shares a lot of code with
RestoreArchivedFile(). Is it possible\viable to refactor and extract
common part?

I am not sure, but I guess that differences in errors reporting and
points 2-3 are enough to leave new RestoreArchivedWAL() apart. There
are not many common parts left.

2. IMV pg_rewind with %r restore_command should fail. %r is designed
to clean archive from WALs, nothing should be deleted in case of
fetching WALs for rewind. Last restartpoint has no meaning during
rewind. Or does it? If so, let's comment about it.

Yes, during rewind we need the last common checkpoint, not restart
point.
Taking into account that Michael pointed me to this place too and I
cannot
propose a real-life example of restore_command with %r to use in
pg_rewind,
I decided to add an exception in such a case. So now pg_rewind will fail
with error.

3. RestoreArchivedFile() checks for signals, is it done by pg_rewind
elsewhere?

There is a comment part inside RestoreArchivedFile():

* However, if the failure was due to any sort of signal, it's best to
* punt and abort recovery. (If we "return false" here, upper levels
will
* assume that recovery is complete and start up the database!)

In other words, there is a possibility to start up the database assuming
that recovery went well, while actually it was terminated by signal. It
happens since failure is expected during the recovery, so some kind of
ambiguity occurs, which we trying to solve checking for termination
signals.

In contrast, we are looking in the archive for each missing WAL file
during
pg_rewind and if we failed to restore it by any means rewind will fail
indifferent of was it due to the termination signal or file is actually
missing in the archive. Thus, there no ambiguity occurs and we do not
need
to check for signals here.

That is how I understand it. Probably someone can explain why I am
wrong.

4. No documentation is updated

I have updated docs in order to reflect the new functionality as well.

5. -R takes precedence over -r without notes. Shouldn't we complain?
Or may be we should take one from config, iif nothing found use -R?

I do not think it is worth of additional complexity and we could expect,
that end-users know, what they want to use–either -r or -R–so I added
an error message due to the conflicting options.

Regards

--
Alexey Kondratov

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

On 21.01.2019 23:50, a.kondratov@postgrespro.ru wrote:

Thank you for the review! I have updated the patch according to your
comments and remarks. Please, find new version attached.

During the self-reviewing of the code and tests, I discovered some
problems with build on Windows. New version of the patch is attached and
it fixes this issue as well as includes some minor code revisions.

Regards

--
Alexey Kondratov

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

Hi! Thanks for the next version!

8 февр. 2019 г., в 18:30, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):

On 21.01.2019 23:50, a.kondratov@postgrespro.ru wrote:

Thank you for the review! I have updated the patch according to your
comments and remarks. Please, find new version attached.

During the self-reviewing of the code and tests, I discovered some problems with build on Windows. New version of the patch is attached and it fixes this issue as well as includes some minor code revisions.

I've made one more pass through code and found no important problems.

The patch moves code including these lines
* XXX this is an unmaintainable crock, because we have to know how to set
* (or at least what to call to set) every variable that could potentially
* have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. However, there's no
* time to redesign it for 9.1.
But I think it's not the point of this patch to refactor that code.

Here's a typo in postgreslq.conf
+ fprintf(stderr, _("%s: option -r/--use-postgresql-conf is specified, but postgreslq.conf is absent in the target directory\n"),

I'm still not sure guc-file refactoring you made is architecturally correct, I do not feel that my expertise is enough to judge, but everything works.

Besides this, I think you can switch patch to "Ready for committer".

check-world is passing on macbook, docs are here, feature is implemented and tested.

Best regards, Andrey Borodin.

Hi!

On 09.02.2019 14:31, Andrey Borodin wrote:

Here's a typo in postgreslq.conf
+ fprintf(stderr, _("%s: option -r/--use-postgresql-conf is specified, but postgreslq.conf is absent in the target directory\n"),

Fixed, thanks. I do not attach new version of the patch for just one
typo, maybe there will be some more remarks from others.

Besides this, I think you can switch patch to "Ready for committer".

check-world is passing on macbook, docs are here, feature is implemented and tested.

OK, cfbot [1]http://cfbot.cputube.org/alexey-kondratov.html does not complain about anything on Linux and Windows as
well, so I am setting it to "Ready for committer" for the next commitfest.

[1]: http://cfbot.cputube.org/alexey-kondratov.html

Regards

--
Alexey Kondratov

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

Hi,

On 2018-11-07 12:58:11 +0300, Alexey Kondratov wrote:

On 30.10.2018 06:01, Michael Paquier wrote:

- Reusing the GUC parser is something I would avoid as well. Not worth
the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend safe.

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
?