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

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2

Hi hackers,

Currently Postgres has options for continuous WAL files archiving, which is quite often used along with master-replica setup. OK, then the worst is happened and it's time to get your old master back and synchronize it with new master (ex-replica) with pg_rewind. However, required WAL files may be already archived and pg_rewind will fail. You can copy these files manually, but it is difficult to calculate, which ones you need. Anyway, it complicates building failover system with automatic failure recovery.

I expect, that it will be a good idea to allow pg_rewind to look for a restore_command in the target data directory recovery.conf or pass it is as a command line argument. Then pg_rewind can use it to get missing WAL files from the archive. I had a few talks with DBAs and came to conclusion, that this is a highly requested feature.

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.

What do you think?


--

Alexey Kondratov

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

Russian Postgres Company


pg_rewind-restore_command-v1.0.patch (39K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andrey Borodin-2
Hi, Alexey!

19 окт. 2018 г., в 22:49, Alexey Kondratov <[hidden email]> написал(а):

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.
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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




Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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
https://www.postgresql.org/message-id/5152F778.2070205@..., 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]
https://www.postgresql.org/message-id/flat/CAHGQGwHi%3D4GV6neLRXF7rexTBkjhcAEqF9_xq%2BtRvFv2bVd59w%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/flat/CA%2BU5nMKyuDxr0%3D5PSen1DZJndauNdz8BuSREau%3DScN-7DZ9acA%40mail.gmail.com

--
Alexey Kondratov

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


Reply | Threaded
Open this post in threaded view
|

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

Andrey Borodin-2
Hi, Alexey!

> 25 окт. 2018 г., в 17:37, Alexey Kondratov <[hidden email]> написал(а):

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

Best regards, Andrey Borodin
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
In reply to this post by Alvaro Herrera-9
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
In reply to this post by Andrey Borodin-2
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


Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
In reply to this post by Michael Paquier-2
> 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


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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] %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


pg_rewind-restore_command-v1.1.patch (39K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On Wed, Nov 7, 2018 at 10:58 AM Alexey Kondratov <[hidden email]> 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?

Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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], 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


Regards

--
Alexey Kondratov

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


0001-pg_rewind-options-to-use-restore_command-v1.2.patch (41K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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


0001-pg_rewind-options-to-use-restore_command-v2.0.patch (66K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andrey Borodin-2
Hi!

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

> 30 окт. 2018 г., в 8:01, Michael Paquier <[hidden email]> написал(а):
>
> 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 <[hidden email]> написал(а):
>
> 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.
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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

0001-pg_rewind-options-to-use-restore_command-v2.1.patch (91K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
On 21.01.2019 23:50, [hidden email] 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


0001-pg_rewind-options-to-use-restore_command-v2.2.patch (69K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andrey Borodin-2
Hi! Thanks for the next version!

> 8 февр. 2019 г., в 18:30, Alexey Kondratov <[hidden email]> написал(а):
>
> On 21.01.2019 23:50, [hidden email] 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.
Reply | Threaded
Open this post in threaded view
|

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

Alexey Kondratov-2
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] 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
In reply to this post by Alexey Kondratov-2
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
?

12