Обсуждение: [Patch] pg_rewind: options to use restore_command from recovery.confor command line

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

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 
https://www.postgresql.org/message-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] 
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

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] %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], 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

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
commonpart? 
2. IMV pg_rewind with %r restore_command should fail. %r is designed to clean archive from WALs, nothing should be
deletedin case of fetching WALs for rewind. Last restartpoint has no meaning during rewind. Or does it? If so, let's
commentabout 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
nothingfound 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
patchis 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
targetdirectory\n"), 

I'm still not sure guc-file refactoring you made is architecturally correct, I do not feel that my expertise is enough
tojudge, 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
thetarget 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

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
?

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

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

Hi,

On 2019-02-18 16:26:55 +0300, Alexey Kondratov wrote:
> 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.

-C doesn't really start the server.


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

We don't generally support that. I don't see why this is something we
ought to invest significant effort into. It's not like you meaningfully
can use pg_rewind on a server without postgres installed.


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

Meh, there's significant parsing related logic, including dealing with
multiple values, that you've excluded. And you've copied substantial
amounts of code to make your approach possible.


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

Note that e.g. .auto.conf is loaded explicitly:
    /*
     * Parse the PG_AUTOCONF_FILENAME file, if present, after the main file to
     * replace any parameters set by ALTER SYSTEM command.  Because this file
     * is in the data directory, we can't read it until the DataDir has been
     * set.
     */
    if (DataDir)
    {
        if (!ParseConfigFile(PG_AUTOCONF_FILENAME, false,
                             NULL, 0, 0, elevel,
                             &head, &tail))
        {
            /* Syntax error(s) detected in the file, so bail out */
            error = true;
            ConfFileWithError = PG_AUTOCONF_FILENAME;
            goto bail_out;
        }
    }

- Andres

On 2019-Feb-18, Andres Freund wrote:

> Hi,
> 
> On 2019-02-18 16:26:55 +0300, Alexey Kondratov wrote:
> > 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
> > > ?

Eh, this is what I suggested in this thread four months ago, though I
didn't remember at the time that aaa6e1def292 had already introduced -C
in 2011.  It's definitely the way to go ... all this messing about with
the parser is insane.

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

On 18.02.2019 19:49, Alvaro Herrera wrote:
>
>>> 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
>>>> ?
> Eh, this is what I suggested in this thread four months ago, though I
> didn't remember at the time that aaa6e1def292 had already introduced -C
> in 2011.  It's definitely the way to go ... all this messing about with
> the parser is insane.
>

Yes, but four months ago recovery options were not a part of GUCs.

OK, if you and Andres are surely negative about solution with parser, 
then I will work out this one with postgres -C and come back till the 
next commitfest. I found that something similar is already used in 
pg_ctl and there is a mechanism for finding valid executables in exec.c. 
So it does not seem to be a big deal at the first sight.

Thanks for replies!


Regards

-- 
Alexey Kondratov

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

Hi,

>
> I will work out this one with postgres -C and come back till the next 
> commitfest. I found that something similar is already used in pg_ctl 
> and there is a mechanism for finding valid executables in exec.c. So 
> it does not seem to be a big deal at the first sight.
>

I have reworked the patch, please find new version attached. It is 3 
times as smaller than the previous one and now touches pg_rewind's code 
only. Tests are also slightly refactored in order to remove duplicated 
code. Execution of postgres -C is used for restore_command retrieval (if 
-r is passed) as being suggested. Otherwise everything works as before.

Andres, Alvaro does it make sense now?



Regards

-- 
Alexey Kondratov

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

Hi!


> 20 февр. 2019 г., в 17:06, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):
>
>>
>> I will work out this one with postgres -C and come back till the next commitfest. I found that something similar is
alreadyused in pg_ctl and there is a mechanism for finding valid executables in exec.c. So it does not seem to be a big
dealat the first sight. 
>>
>
> I have reworked the patch, please find new version attached. It is 3 times as smaller than the previous one and now
touchespg_rewind's code only. Tests are also slightly refactored in order to remove duplicated code. Execution of
postgres-C is used for restore_command retrieval (if -r is passed) as being suggested. Otherwise everything works as
before.
>

> <v4-0001-pg_rewind-options-to-use-restore_command.patch>

The new patch is much smaller (less than 400 lines) and works as advertised.
There's a typo "retreive" there.

These lines look a little suspicious:
        char  postgres_exec_path[MAXPGPATH],
                  postgres_cmd[MAXPGPATH],
                  cmd_output[MAX_RESTORE_COMMAND];
Is it supposed to be any difference between MAXPGPATH and MAX_RESTORE_COMMAND?

Besides this, patch looks fine to me.

Best regards, Andrey Borodin.

On 3/6/19 5:38 PM, Andrey Borodin wrote:
>> 20 февр. 2019 г., в 17:06, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):
> 
> The new patch is much smaller (less than 400 lines) and works as advertised.
> There's a typo "retreive" there.
> 
> These lines look a little suspicious:
>         char  postgres_exec_path[MAXPGPATH],
>                   postgres_cmd[MAXPGPATH],
>                   cmd_output[MAX_RESTORE_COMMAND];
> Is it supposed to be any difference between MAXPGPATH and MAX_RESTORE_COMMAND?
> 
> Besides this, patch looks fine to me.

This patch appears to need attention from the author so I have marked it 
Waiting on Author.

Regards,
-- 
-David
david@pgmasters.net

On 07.03.2019 10:26, David Steele wrote:
> On 3/6/19 5:38 PM, Andrey Borodin wrote:
>>
>> The new patch is much smaller (less than 400 lines) and works as 
>> advertised.
>> There's a typo "retreive" there.

Ough, corrected this in three different places. Not my word, definitely. 
Thanks!

>>
>> These lines look a little suspicious:
>>         char  postgres_exec_path[MAXPGPATH],
>>                   postgres_cmd[MAXPGPATH],
>>                   cmd_output[MAX_RESTORE_COMMAND];
>> Is it supposed to be any difference between MAXPGPATH and 
>> MAX_RESTORE_COMMAND?
>>

Yes, it was supposed to be, but after your message I have double checked 
everything and figured out that we use MAXPGPATH for final 
restore_command build (with all aliases replaced). Thus, there is no 
need in a separated constant. I have replaced it with MAXPGPATH.

>
> This patch appears to need attention from the author so I have marked 
> it Waiting on Author.
>

I hope I have addressed all issues in the new patch version which is 
attached. Also, I have added more detailed explanation of new 
functionality into the multi-line commit-message.


Regards,

-- 
Alexey Kondratov

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

Hi!

> 7 марта 2019 г., в 20:27, Alexey Kondratov <a.kondratov@postgrespro.ru> написал(а):
>
> <v5-0001-pg_rewind-options-to-use-restore_command.patch>


I'm a bit confused by by console output routines. E.g. in pg_rewind's main() you call pg_fatal()s, and printf(), and
pg_log()with various levels. Shouldn't we use all the pg_* functions? 
But most of this printing usages were there before your patch.

I'm marking the patch as RFC, since I have no more notices and patch really looks good.

Best regards, Andrey Borodin.

On Wed, Mar 20, 2019 at 01:55:51PM +0800, Andrey Borodin wrote:
> I'm a bit confused by by console output routines. E.g. in
> pg_rewind's main() you call pg_fatal()s, and printf(), and pg_log()
> with various levels. Shouldn't we use all the pg_* functions?

pg_fatal() would exit immediately, and sometimes the error code paths
want to have multiple lines, which is why printf() gets used.

> But most of this printing usages were there before your patch.
>
> I'm marking the patch as RFC, since I have no more notices and patch
> really looks good.

That does not look fully baked yet, at least in my opinion.

+ * This is a simplified and adapted to frontend version of
+ * RestoreArchivedFile function from transam/xlogarchive.c
+ */
+static int
+RestoreArchivedWAL(const char *path, const char *xlogfname,
I don't think that we should have duplicates for that, so I would
recommend refactoring the code so as a unique code path is taken by
both, especially since the user can fetch the command from
postgresql.conf.

Why two options?  Wouldn't actually be enough use-postgresql-conf to
do the job?  Note that "postgres" should always be installed if
pg_rewind is present because it is a backend-side utility, so while I
don't like adding a dependency to other binaries in one binary, having
an option to pass out a command directly via the command line of
pg_rewind stresses me more.

Don't we need to worry about signals interrupting the restore command?
It seems to me that some refactoring from the stuff in xlogarchive.c
would be in order.
--
Michael

On 26.03.2019 11:19, Michael Paquier wrote:
> + * This is a simplified and adapted to frontend version of
> + * RestoreArchivedFile function from transam/xlogarchive.c
> + */
> +static int
> +RestoreArchivedWAL(const char *path, const char *xlogfname,
> I don't think that we should have duplicates for that, so I would
> recommend refactoring the code so as a unique code path is taken by
> both, especially since the user can fetch the command from
> postgresql.conf.

This comment is here since the beginning of my work on this patch and 
now it is rather misleading.

Even if we does not take into account obvious differences like error 
reporting, different log levels based on many conditions, cleanup 
options, check for standby mode; restore_command execution at backend 
recovery and during pg_rewind has a very important difference. If it 
fails at backend, then as stated in the comment 'Remember, we 
rollforward UNTIL the restore fails so failure here is just part of the 
process' -- it is OK. In opposite, in pg_rewind if we failed to recover 
some required WAL segment, then it definitely means the end of the 
entire process, since we will fail at finding last common checkpoint or 
extracting page map.

The only part we can share is constructing restore_command with aliases 
replacement. However, even in this place the logic is slightly 
different, since we do not need %r alias for pg_rewind. The only use 
case of %r in restore_command I know is pg_standby, which seems to be as 
not a case for pg_rewind. I have tried to move this part to the common, 
but it becomes full of conditions and less concise.

Please, correct me if I am wrong, but it seems that there are enough 
differences to keep this function separated, isn't it?

> Why two options?  Wouldn't actually be enough use-postgresql-conf to
> do the job?  Note that "postgres" should always be installed if
> pg_rewind is present because it is a backend-side utility, so while I
> don't like adding a dependency to other binaries in one binary, having
> an option to pass out a command directly via the command line of
> pg_rewind stresses me more.

I am not familiar enough with DBA scenarios, where -R option may be 
useful, but I was asked a few times for that. I can only speculate that 
for example someone may want to run freshly rewinded cluster as master, 
not replica, so its config may differ from replica's one, where 
restore_command is surely intended to be. Thus, it is easier to leave 
master's config at the place and just specify restore_command as command 
line argument.

> Don't we need to worry about signals interrupting the restore command?
> It seems to me that some refactoring from the stuff in xlogarchive.c
> would be in order.

Thank you for pointing me to this place again. Previously, I thought 
that we should not care about it, since if restore_command was not 
successful due to any reason, then rewind failed, so we will stop and 
exit at upper levels. However, if it was due to a signal, then some of 
next messages may be misleading, if e.g. user manually interrupted it 
for some reason. So that, I added a similar check here as well.

Updated version of patch is attached.


-- 
Alexey Kondratov

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

On Thu, Mar 28, 2019 at 6:49 AM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
> Updated version of patch is attached.

Hi Alexey,

This no longer applies.  Since the Commitfest is starting now, could
you please rebase it?

Thanks,

-- 
Thomas Munro
https://enterprisedb.com

Hi Thomas,

On 01.07.2019 15:02, Thomas Munro wrote:

> Hi Alexey,
>
> This no longer applies.  Since the Commitfest is starting now, could
> you please rebase it?

Thank you for a reminder. Rebased version of the patch is attached. I've 
also modified my logging code in order to obey new unified logging 
system for command-line programs commited by Peter (cc8d415117).


Regards

-- 
Alexey Kondratov

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

On 7/1/19 5:20 PM, Alexey Kondratov wrote:
> Hi Thomas,
>
> On 01.07.2019 15:02, Thomas Munro wrote:
>
>> Hi Alexey,
>>
>> This no longer applies.  Since the Commitfest is starting now, could
>> you please rebase it?
>
> Thank you for a reminder. Rebased version of the patch is attached. 
> I've also modified my logging code in order to obey new unified 
> logging system for command-line programs commited by Peter (cc8d415117).
>
>
> Regards
>
Hi Alexey,

I would like to suggest a couple of changes to docs and comments, please 
see the attachment.
The "...or fetched on startup" part also seems wrong here, but it's not 
a part of your patch, so I'm going to ask about it on psql-docs separately.

It might also be useful to reword the following error messages:
- "using restored from archive version of file \"%s\""
- "could not open restored from archive file \"%s\"
We could probably say something like "could not open file \"%s\" 
restored from WAL archive" instead.

On a more general note, I wonder if everyone is happy with the 
--using-postgresql-conf option name, or we should continue searching for 
a narrower term. Unfortunately, I don't have any better suggestions 
right now, but I believe it should be clear that its purpose is to fetch 
missing WAL files for target. What do you think?

-- 
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 26.07.2019 20:43, Liudmila Mantrova wrote:
>
> I would like to suggest a couple of changes to docs and comments, 
> please see the attachment.
> The "...or fetched on startup" part also seems wrong here, but it's 
> not a part of your patch, so I'm going to ask about it on psql-docs 
> separately.

Agreed, thank you a lot! Yes, "...or fetched on startup" looks a bit 
confusing for me, since the whole paragraph is about target server 
before running pg_rewind, but this statement is more about target server 
started first time after running pg_rewind, which is discussed in the 
next paragraph.

>
> It might also be useful to reword the following error messages:
> - "using restored from archive version of file \"%s\""
> - "could not open restored from archive file \"%s\"
> We could probably say something like "could not open file \"%s\" 
> restored from WAL archive" instead.

I have reworded these and some similar messages, thanks. New patch with 
changed messages is attached.

>
> On a more general note, I wonder if everyone is happy with the 
> --using-postgresql-conf option name, or we should continue searching 
> for a narrower term. Unfortunately, I don't have any better 
> suggestions right now, but I believe it should be clear that its 
> purpose is to fetch missing WAL files for target. What do you think?
>

I don't like it either, but this one was my best guess then. Maybe 
--restore-target-wal instead of --using-postgresql-conf will be better? 
And --target-restore-command instead of --restore-command if we want to 
specify that this is restore_command for target server?


Regards

-- 
Alexey Kondratov

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

On 01.08.2019 19:53, Alexey Kondratov wrote:
> On 26.07.2019 20:43, Liudmila Mantrova wrote:
>> On a more general note, I wonder if everyone is happy with the 
>> --using-postgresql-conf option name, or we should continue searching 
>> for a narrower term. Unfortunately, I don't have any better 
>> suggestions right now, but I believe it should be clear that its 
>> purpose is to fetch missing WAL files for target. What do you think?
>>
>
> I don't like it either, but this one was my best guess then. Maybe 
> --restore-target-wal instead of --using-postgresql-conf will be 
> better? And --target-restore-command instead of --restore-command if 
> we want to specify that this is restore_command for target server?
>

As Alvaro correctly pointed in the nearby thread [1], we've got an 
interference regarding -R command line argument. I agree that it's a 
good idea to reserve -R for recovery configuration write to be 
consistent with pg_basebackup, so I've updated my patch to use another 
letters:

1. -c/--restore-target-wal --- to use restore_command from postgresql.conf
2. -C/--target-restore-command --- to pass restore_command as a command 
line argument

Updated and rebased patch is attached. However, now I'm wondering, do we 
actually need 1. as a separated option and not being enabled by default? 
I cannot imagine a situation, when restore_command is set in the 
postgresql.conf and someone prefer pg_rewind to fail instead of fetching 
missed WALs automatically, but maybe there are some cases?


[1] 
https://www.postgresql.org/message-id/20190925174812.GA4916%40alvherre.pgsql


-- 
Alexey Kondratov

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

On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote:
> As Alvaro correctly pointed in the nearby thread [1], we've got an
> interference regarding -R command line argument. I agree that it's a good
> idea to reserve -R for recovery configuration write to be consistent with
> pg_basebackup, so I've updated my patch to use another letters:

The patch has rotten and does not apply anymore.  Could you please
send a rebased version?  I have moved the patch to next CF, waiting on
author for now.
--
Michael

On 01.12.2019 5:57, Michael Paquier wrote:
> On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote:
>> As Alvaro correctly pointed in the nearby thread [1], we've got an
>> interference regarding -R command line argument. I agree that it's a good
>> idea to reserve -R for recovery configuration write to be consistent with
>> pg_basebackup, so I've updated my patch to use another letters:
> The patch has rotten and does not apply anymore.  Could you please
> send a rebased version?  I have moved the patch to next CF, waiting on
> author for now.

Rebased and updated patch is attached.

There was a problem with testing new restore_command options altogether 
with recent ensureCleanShutdown. My test simply moves all WAL from 
pg_wal and generates restore_command for a new options testing, but this 
prevents startup recovery required by ensureCleanShutdown. To test both 
options in the same we have to leave some recent WAL segments in the 
pg_wal and be sure that they are enough for startup recovery, but not 
enough for successful pg_rewind run. I have manually figured out that 
required amount of inserted records (and generated WAL) to achieve this. 
However, I think that this approach is not good for test, since tests 
may be modified in the future (amount of writes to DB changed) or even 
volume of WAL written by Postgres will change. It will lead to falsely 
always failed or passed tests.

Moreover, testing both ensureCleanShutdown and new options in the same 
time doesn't hit new code paths, so I decided to test new options with 
--no-ensure-shutdown for simplicity and stability of tests.


Regards

-- 
Alexey Kondratov

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

Hi!

On Tue, Dec 3, 2019 at 12:41 PM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
> On 01.12.2019 5:57, Michael Paquier wrote:
> > On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote:
> >> As Alvaro correctly pointed in the nearby thread [1], we've got an
> >> interference regarding -R command line argument. I agree that it's a good
> >> idea to reserve -R for recovery configuration write to be consistent with
> >> pg_basebackup, so I've updated my patch to use another letters:
> > The patch has rotten and does not apply anymore.  Could you please
> > send a rebased version?  I have moved the patch to next CF, waiting on
> > author for now.
>
> Rebased and updated patch is attached.
>
> There was a problem with testing new restore_command options altogether
> with recent ensureCleanShutdown. My test simply moves all WAL from
> pg_wal and generates restore_command for a new options testing, but this
> prevents startup recovery required by ensureCleanShutdown. To test both
> options in the same we have to leave some recent WAL segments in the
> pg_wal and be sure that they are enough for startup recovery, but not
> enough for successful pg_rewind run. I have manually figured out that
> required amount of inserted records (and generated WAL) to achieve this.
> However, I think that this approach is not good for test, since tests
> may be modified in the future (amount of writes to DB changed) or even
> volume of WAL written by Postgres will change. It will lead to falsely
> always failed or passed tests.
>
> Moreover, testing both ensureCleanShutdown and new options in the same
> time doesn't hit new code paths, so I decided to test new options with
> --no-ensure-shutdown for simplicity and stability of tests.

I think this patch is now in a good shape and already got enough of review.

I made some minor cleanup.  In particular, I've to fix usage of terms
"WAL" and "WALs".  This patch sometimes use term "WAL" to specify
single WAL file and term "WALs" to specify multiple WAL files.  But
WAL stands for Write Ahead Log.  So, "WALs" literally stands to
multiple logs.  And we don't use term "WALs" to describe multiple WAL
files anywhere else.  Usage of term "WAL" to describe single file is
not clear as well.

The revision of patch is attached.  I'm going to push it if no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Jan 18, 2020 at 06:21:22PM +0300, Alexander Korotkov wrote:
> I made some minor cleanup.  In particular, I've to fix usage of terms
> "WAL" and "WALs".  This patch sometimes use term "WAL" to specify
> single WAL file and term "WALs" to specify multiple WAL files.  But
> WAL stands for Write Ahead Log.  So, "WALs" literally stands to
> multiple logs.  And we don't use term "WALs" to describe multiple WAL
> files anywhere else.  Usage of term "WAL" to describe single file is
> not clear as well.

WAL is a method to ensure data integrity, as the docs mention:
https://www.postgresql.org/docs/11/wal-intro.html

So using WAL to tell about a WAL segment file is wrong, WALs is not a
term that actually exists.  So, in my opinion, it is fine to use "WAL
file", "WAL segment" or even "WAL segment file".

I have read through the patch, while on it..

+static int
+RestoreArchivedWALFile(const char *path, const char *xlogfname,
+                      off_t expectedSize, const char *restoreCommand)
Not a fan of putting that to pg_rewind/parsexlog.c.  It has nothing to
do with WAL parsing, and it seems to me that we could have an argument
for making this available as a frontend-only API in src/common/.

Do we actually need --target-restore-command at all?  It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..

+   printf(_("  -c, --restore-target-wal              use restore_command in the postgresql.conf\n"));
+   printf(_("                                        to retrieve WAL
files from archive\n"));
The command could be part of a different configuration file, included
by postgresql.conf.

+use File::Glob ':bsd_glob';
+use File::Path qw(remove_tree make_path);
+use File::Spec::Functions qw(catdir catfile);
Is this compatible with our minimum perl requirements for the TAP
tests?

+       # Add restore_command to postgresql.conf of target cluster.
+       open(my $conf_fd, ">>", $master_conf_path) or die;
+       print $conf_fd "\nrestore_command='$restore_command'";
+       close $conf_fd;
We have append_conf() for that.
--
Michael

On 2020-Jan-19, Michael Paquier wrote:

> So using WAL to tell about a WAL segment file is wrong, WALs is not a
> term that actually exists.

I agree.

> So, in my opinion, it is fine to use "WAL file", "WAL segment" or even
> "WAL segment file".

Agreed with these three terms -- "WAL file" seems to be the most common,
but the other two terms you suggest are just as good.

> +use File::Glob ':bsd_glob';
> +use File::Path qw(remove_tree make_path);
> +use File::Spec::Functions qw(catdir catfile);
> Is this compatible with our minimum perl requirements for the TAP
> tests?

By and large, we just join elements with a slash "foo/bar" to create
path names; no need for catdir or catfile, ISTM.

I *think* :bsd_glob should be available in all the Perl versions we
support, but I'm not sure that we really need it.  We seem to do just
fine with regular glob elsewhere.

PostgresNode already uses File::Path's rmtree.  Looking at its manual,
maybe the safest bet is to change all that to remove_tree.

Not sure about make_path.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Jan-19, Michael Paquier wrote:
>> +use File::Glob ':bsd_glob';
>> +use File::Path qw(remove_tree make_path);
>> +use File::Spec::Functions qw(catdir catfile);
>> Is this compatible with our minimum perl requirements for the TAP
>> tests?

> I *think* :bsd_glob should be available in all the Perl versions we
> support, but I'm not sure that we really need it.  We seem to do just
> fine with regular glob elsewhere.

We had some previous discussion about that, and Andrew seemed to
think that it wouldn't necessarily be a net win:

https://www.postgresql.org/message-id/fde787ee-9c2d-46da-3ece-f7ae64a70b33%402ndQuadrant.com

In any case, I'd say -1 to having just one place use that.

            regards, tom lane

Hi, Michael!

Thank you for your review!
The revised patch is attached.

On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Sat, Jan 18, 2020 at 06:21:22PM +0300, Alexander Korotkov wrote:
> > I made some minor cleanup.  In particular, I've to fix usage of terms
> > "WAL" and "WALs".  This patch sometimes use term "WAL" to specify
> > single WAL file and term "WALs" to specify multiple WAL files.  But
> > WAL stands for Write Ahead Log.  So, "WALs" literally stands to
> > multiple logs.  And we don't use term "WALs" to describe multiple WAL
> > files anywhere else.  Usage of term "WAL" to describe single file is
> > not clear as well.
>
> WAL is a method to ensure data integrity, as the docs mention:
> https://www.postgresql.org/docs/11/wal-intro.html
>
> So using WAL to tell about a WAL segment file is wrong, WALs is not a
> term that actually exists.  So, in my opinion, it is fine to use "WAL
> file", "WAL segment" or even "WAL segment file".
>
> I have read through the patch, while on it..
>
> +static int
> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
> +                      off_t expectedSize, const char *restoreCommand)
> Not a fan of putting that to pg_rewind/parsexlog.c.  It has nothing to
> do with WAL parsing, and it seems to me that we could have an argument
> for making this available as a frontend-only API in src/common/.

I've put it into src/common/fe_archive.c.

> Do we actually need --target-restore-command at all?  It seems to me
> that we have all we need with --restore-target-wal, and that's not
> really instinctive to pass down a command via another command..

I was going to argue that --restore-target-wal is useful.  But I
didn't find convincing example for that.  Naturally, if one uses
restore_command during pg_rewind then why the same restore_command
can't be used in new standby.  So, I've removed --restore-target-wal
argument.  If we will find convincing use case we can return it any
moment.

> +   printf(_("  -c, --restore-target-wal              use restore_command in the postgresql.conf\n"));
> +   printf(_("                                        to retrieve WAL
> files from archive\n"));
> The command could be part of a different configuration file, included
> by postgresql.conf.

I've rephrased the comment.

> +use File::Glob ':bsd_glob';
> +use File::Path qw(remove_tree make_path);
> +use File::Spec::Functions qw(catdir catfile);
> Is this compatible with our minimum perl requirements for the TAP
> tests?

All extra dependencies have been removed.

> +       # Add restore_command to postgresql.conf of target cluster.
> +       open(my $conf_fd, ">>", $master_conf_path) or die;
> +       print $conf_fd "\nrestore_command='$restore_command'";
> +       close $conf_fd;
> We have append_conf() for that.

Sure, thank you for pointing!


------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Jan 22, 2020 at 12:55:30AM +0300, Alexander Korotkov wrote:
> Thank you for your review!
> The revised patch is attached.

Thanks for the new version.

> On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier <michael@paquier.xyz> wrote:
>> +static int
>> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
>> +                      off_t expectedSize, const char *restoreCommand)
>> Not a fan of putting that to pg_rewind/parsexlog.c.  It has nothing to
>> do with WAL parsing, and it seems to me that we could have an argument
>> for making this available as a frontend-only API in src/common/.
>
> I've put it into src/common/fe_archive.c.

This split makes sense.  You have forgotten to update
@pgcommonfrontendfiles in Mkvcbuild.pm for the MSVC build.  Still, I
can see that we have a lot of duplication between the code of the
frontend and the backend, which is annoying..  The execution part is
tricky to split because the backend has pre- and post- callbacks, the
interruption handling is not the same and there is the problem of
elog() calls, with elevel that can be changed depending on the backend
configuration.  However, I can see one portion of the code which is
fully duplicated and could be refactored out: the construction of the
command to execute, by having in input the restore_command string and
the arguments that we expect to replace in the '%' markers which are
part of the command.  If NULL is specified for one of those values,
then the construction routine returns NULL, synonym of failure.  And
the result of the routine is the built command.  I think that you
would need an extra argument to give space for the error message
generated in the event of an error when building the command.

>> Do we actually need --target-restore-command at all?  It seems to me
>> that we have all we need with --restore-target-wal, and that's not
>> really instinctive to pass down a command via another command..
>
> I was going to argue that --restore-target-wal is useful.  But I
> didn't find convincing example for that.  Naturally, if one uses
> restore_command during pg_rewind then why the same restore_command
> can't be used in new standby.  So, I've removed --restore-target-wal
> argument.  If we will find convincing use case we can return it any
> moment.

Okay.  Let's consider it if it makes sense.  You can always go through
this restriction by first setting restore_command in the target
cluster, and then run pg_rewind.  And I would think that someone is
going to use the same restore_command even after restarting the
rewound target cluster.

>> +       # Add restore_command to postgresql.conf of target cluster.
>> +       open(my $conf_fd, ">>", $master_conf_path) or die;
>> +       print $conf_fd "\nrestore_command='$restore_command'";
>> +       close $conf_fd;
>> We have append_conf() for that.
>
> Sure, thank you for pointing!

+++ b/src/include/common/fe_archive.h
+#ifndef ARCHIVE_H
+#define ARCHIVE_H
This should be FE_ARCHIVE_H.

-   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
    &option_index)) != -1)
+   while ((c = getopt_long(argc, argv, "D:nNPRC:c", long_options,
    &option_index)) != -1)
This still includes 'C', but that should not be the case.

+   <application>pg_rewind</application> with the
<literal>-c</literal> or
+   <literal>-C</literal> option to automatically retrieve them from
the WAL
[...]
+      <term><option>-C <replaceable
class="parameter">restore_command</replaceable></option></term>
+      <term><option>--target-restore-command=<replaceable
class="parameter">restore_command</replaceable></option></term>
[...]
+      available, try re-running <application>pg_rewind</application> with
+      the <option>-c</option> or <option>-C</option> option to search
+      for the missing files in the WAL archive.
Three places in the docs need to be cleaned up.

Do we really need to test the new "archive" mode as well for
003_extrafiles and 002_databases?  I would imagine that only 001_basic
is enough.

+# Moves WAL files to the temporary location and returns restore_command
+# to get them back.
+sub move_wal
+{
+   my ($tmp_dir, $master_pgdata) = @_;
+   my $wal_archive_path = "$tmp_dir/master_wal_archive";
+   my $wal_path = "$master_pgdata/pg_wal";
+   my $wal_dir;
+   my $restore_command;
I think that this routine is wrongly designed.  First, in order to
copy the contents from one path to another, we have
RecursiveCopy::copy_path, and there is a long history about making
it safe for the TAP tests.  Second, there is in
PostgresNode::enable_restoring already a code path doing the
decision-making on how building restore_command.  We should keep this
code path unique.
--
Michael

On 2020-01-24 08:50, Michael Paquier wrote:
> On Wed, Jan 22, 2020 at 12:55:30AM +0300, Alexander Korotkov wrote:
>> On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier <michael@paquier.xyz> 
>> wrote:
>>> +static int
>>> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
>>> +                      off_t expectedSize, const char 
>>> *restoreCommand)
>>> Not a fan of putting that to pg_rewind/parsexlog.c.  It has nothing 
>>> to
>>> do with WAL parsing, and it seems to me that we could have an 
>>> argument
>>> for making this available as a frontend-only API in src/common/.
>> 
>> I've put it into src/common/fe_archive.c.
> 
> This split makes sense.  You have forgotten to update
> @pgcommonfrontendfiles in Mkvcbuild.pm for the MSVC build.  Still, I
> can see that we have a lot of duplication between the code of the
> frontend and the backend, which is annoying..  The execution part is
> tricky to split because the backend has pre- and post- callbacks, the
> interruption handling is not the same and there is the problem of
> elog() calls, with elevel that can be changed depending on the backend
> configuration.  However, I can see one portion of the code which is
> fully duplicated and could be refactored out: the construction of the
> command to execute, by having in input the restore_command string and
> the arguments that we expect to replace in the '%' markers which are
> part of the command.
> 

OK, I agree that duplication of this part directly is annoying. I did a 
refactoring and moved this restore_command building code into 
common/archive. Separate file was needed, since fe_archive is 
frontend-only, so I cannot put universal code there. I do not know is 
there any better place for it.

> 
> If NULL is specified for one of those values,
> then the construction routine returns NULL, synonym of failure.  And
> the result of the routine is the built command.  I think that you
> would need an extra argument to give space for the error message
> generated in the event of an error when building the command.
> 

I did it in a bit different way, but the overall logic is exactly as you 
proposed, I hope.
Also I have checked win/linux build in the similar to cfbot CI setup and 
now everything runs well.

> 
> +++ b/src/include/common/fe_archive.h
> +#ifndef ARCHIVE_H
> +#define ARCHIVE_H
> This should be FE_ARCHIVE_H.
> 

Changed.

> 
> -   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
>     &option_index)) != -1)
> +   while ((c = getopt_long(argc, argv, "D:nNPRC:c", long_options,
>     &option_index)) != -1)
> This still includes 'C', but that should not be the case.
> 
> +   <application>pg_rewind</application> with the
> <literal>-c</literal> or
> +   <literal>-C</literal> option to automatically retrieve them from
> the WAL
> [...]
> +      <term><option>-C <replaceable
> class="parameter">restore_command</replaceable></option></term>
> +      <term><option>--target-restore-command=<replaceable
> class="parameter">restore_command</replaceable></option></term>
> [...]
> +      available, try re-running <application>pg_rewind</application> 
> with
> +      the <option>-c</option> or <option>-C</option> option to search
> +      for the missing files in the WAL archive.
> Three places in the docs need to be cleaned up.
> 

I have fixed all the above, thanks.

> 
> Do we really need to test the new "archive" mode as well for
> 003_extrafiles and 002_databases?  I would imagine that only 001_basic
> is enough.
> 

We do not check any unique code paths with it, so I do it only for 
001_basic now. I have left it as a test mode, since it will be easy to 
turn this on for any other test in the future if needed and it fits well 
RewindTest.pm module.

> 
> +# Moves WAL files to the temporary location and returns 
> restore_command
> +# to get them back.
> +sub move_wal
> +{
> +   my ($tmp_dir, $master_pgdata) = @_;
> +   my $wal_archive_path = "$tmp_dir/master_wal_archive";
> +   my $wal_path = "$master_pgdata/pg_wal";
> +   my $wal_dir;
> +   my $restore_command;
> I think that this routine is wrongly designed.  First, in order to
> copy the contents from one path to another, we have
> RecursiveCopy::copy_path, and there is a long history about making
> it safe for the TAP tests.  Second, there is in
> PostgresNode::enable_restoring already a code path doing the
> decision-making on how building restore_command.  We should keep this
> code path unique.
> 

Yeah, thank you for pointing me to the RecursiveCopy::copypath and 
especially PostgresNode::enable_restoring, I have modified test to use 
them instead. The copypath does not work with existing destination 
directories and does not preserve initial permissions, so I had to do 
some dirty work to achieve what we need in the test and keep it simple 
in the same time. However, I think that using these unified routines is 
much better for a future support.

New version is attached. Do you have any other comments or objections?


Regards
--
Alexey

On Sat, Feb 1, 2020 at 2:16 AM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
> New version is attached. Do you have any other comments or objections?

Now patch looks much better.  Thanks to Michael Paquier for review.
I've just revised commit message reflecting we've removed one of the
new options.

But I have following question.

+       # Move all old master WAL files to the archive.
+       RecursiveCopy::copypath(
+           $node_master->data_dir . "/pg_wal",
+           $node_master->archive_dir
+       );
+       chmod(0700, $node_master->archive_dir);
+
+       # Fast way to remove entire directory content
+       rmtree($node_master->data_dir . "/pg_wal");
+       mkdir($node_master->data_dir . "/pg_wal");
+       chmod(0700, $node_master->data_dir . "/pg_wal");

I think usage of chmod() deserves comment.  As I get default
permissions are sufficient for work, but we need to set them to
satisfy 'check PGDATA permissions' test.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
>
> I think usage of chmod() deserves comment.  As I get default
> permissions are sufficient for work, but we need to set them to
> satisfy 'check PGDATA permissions' test.


I've added this comment myself.  I've also fixes some indentation.
Patch now looks good to me.  I'm going to push it if no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-02-26 22:03, Alexander Korotkov wrote:
> On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>> 
>> I think usage of chmod() deserves comment.  As I get default
>> permissions are sufficient for work, but we need to set them to
>> satisfy 'check PGDATA permissions' test.
> 
> 
> I've added this comment myself.
> 

Thanks for doing it yourself, I was going to answer tonight, but it 
would be obviously too late.

> 
> I've also fixes some indentation.
> Patch now looks good to me.  I'm going to push it if no objections.
> 

I think that docs should be corrected. Previously Michael was against 
the phrase 'restore_command defined in the postgresql.conf', since it 
also could be defined in any config file included there. We corrected it 
in the pg_rewind --help output, but now docs say:

+        Use the <varname>restore_command</varname> defined in
+        <filename>postgresql.conf</filename> to retrieve WAL files from
+        the WAL archive if these files are no longer available in the
+        <filename>pg_wal</filename> directory of the target cluster.

Probably it should be something like:

+        Use the <varname>restore_command</varname> defined in
+        the target cluster configuration to retrieve WAL files from
+        the WAL archive if these files are no longer available in the
+        <filename>pg_wal</filename> directory.

Here the only text split changed:

-     * Ignore restore_command when not in archive recovery (meaning
-     * we are in crash recovery).
+     * Ignore restore_command when not in archive recovery (meaning we are 
in
+     * crash recovery).

Should we do so in this patch?

I think that this extra dot at the end is not necessary here:

+        pg_log_debug("using config variable restore_command=\'%s\'.", 
restore_command);

If you agree then attached is a patch with all the corrections above. It 
is made with default git format-patch format, but yours were in a 
slightly different format, so I only was able to apply them with git am 
--patch-format=stgit.


--
Alexey Kondratov

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

On Wed, Feb 26, 2020 at 11:45 PM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
> On 2020-02-26 22:03, Alexander Korotkov wrote:
> > On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
> > <a.korotkov@postgrespro.ru> wrote:
> >>
> >> I think usage of chmod() deserves comment.  As I get default
> >> permissions are sufficient for work, but we need to set them to
> >> satisfy 'check PGDATA permissions' test.
> >
> >
> > I've added this comment myself.
> >
>
> Thanks for doing it yourself, I was going to answer tonight, but it
> would be obviously too late.
>
> >
> > I've also fixes some indentation.
> > Patch now looks good to me.  I'm going to push it if no objections.
> >
>
> I think that docs should be corrected. Previously Michael was against
> the phrase 'restore_command defined in the postgresql.conf', since it
> also could be defined in any config file included there. We corrected it
> in the pg_rewind --help output, but now docs say:
>
> +        Use the <varname>restore_command</varname> defined in
> +        <filename>postgresql.conf</filename> to retrieve WAL files from
> +        the WAL archive if these files are no longer available in the
> +        <filename>pg_wal</filename> directory of the target cluster.
>
> Probably it should be something like:
>
> +        Use the <varname>restore_command</varname> defined in
> +        the target cluster configuration to retrieve WAL files from
> +        the WAL archive if these files are no longer available in the
> +        <filename>pg_wal</filename> directory.
>
> Here the only text split changed:
>
> -        * Ignore restore_command when not in archive recovery (meaning
> -        * we are in crash recovery).
> +        * Ignore restore_command when not in archive recovery (meaning we are
> in
> +        * crash recovery).
>
> Should we do so in this patch?
>
> I think that this extra dot at the end is not necessary here:
>
> +               pg_log_debug("using config variable restore_command=\'%s\'.",
> restore_command);
>
> If you agree then attached is a patch with all the corrections above.

Thank you!  I'll push the v17 version of patch.

Regarding text split change, it was made by pgindent.  I didn't notice
it belongs to unchanged part of code.  Sure, we shouldn't include this
into the patch.

> It is made with default git format-patch format, but yours were in a
> slightly different format, so I only was able to apply them with git am
> --patch-format=stgit.

I made this patch using 'git show'. Yes, it would be better if I use
'git format-patch' instead.  Thank you for noticing.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote:
> Regarding text split change, it was made by pgindent.  I didn't notice
> it belongs to unchanged part of code.  Sure, we shouldn't include this
> into the patch.

I have read through v17 (not tested, sorry), and spotted a couple of
issues that need to be addressed.

+                               "--source-pgdata=$standby_pgdata",
+                               "--target-pgdata=$master_pgdata",
+                               "--no-sync", "--no-ensure-shutdown",
FWIW, I think that perl indenting would reshape this part.  I would
recommend to run src/tools/pgindent/pgperltidy and
./src/tools/perlcheck/pgperlcritic before commit.

+ * Copyright (c) 2020, PostgreSQL Global Development Group
Wouldn't it be better to just use the full copyright here?  I mean the
following:
Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
Portions Copyright (c) 1994, The Regents of the University of California

+++ b/src/common/archive.c
[...]
+#include "postgres.h"
+
+#include "common/archive.h"
This is incorrect.  All files shared between the backend and the
frontend in src/common/ have to include the following set of headers:
#ifndef FRONTEND
#include "postgres.h"
#else
#include "postgres_fe.h"
#endif

+++ b/src/common/fe_archive.c
[...]
+#include "postgres_fe.h"
This is incomplete.  The following piece should be added:
#ifndef FRONTEND
#error "This file is not expected to be compiled for backend code"
#endif

+               snprintf(postgres_cmd, sizeof(postgres_cmd), "%s -D %s
-C restore_command",
+                                postgres_exec_path, datadir_target);
+
I think that this is missing proper quoting.

I would rename ConstructRestoreCommand() to BuildRestoreCommand()
while on it..

I think that it would be saner to check the return status of
ConstructRestoreCommand() in xlogarchive.c as a sanity check, with an
elog(ERROR) if not 0, as that should never happen.
--
Michael

On 2020-02-27 04:52, Michael Paquier wrote:
> On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote:
>> Regarding text split change, it was made by pgindent.  I didn't notice
>> it belongs to unchanged part of code.  Sure, we shouldn't include this
>> into the patch.
> 
> I have read through v17 (not tested, sorry), and spotted a couple of
> issues that need to be addressed.
> 
> +                               "--source-pgdata=$standby_pgdata",
> +                               "--target-pgdata=$master_pgdata",
> +                               "--no-sync", "--no-ensure-shutdown",
> FWIW, I think that perl indenting would reshape this part.  I would
> recommend to run src/tools/pgindent/pgperltidy and
> ./src/tools/perlcheck/pgperlcritic before commit.
> 

Thanks, formatted this part with perltidy. It also has modified 
RecursiveCopy's indents. Pgperlcritic has no complains about this file. 
BTW, being executed on the whole project pgperltidy modifies dozens of 
perl files an even pgindent itself.

> 
> + * Copyright (c) 2020, PostgreSQL Global Development Group
> Wouldn't it be better to just use the full copyright here?  I mean the
> following:
> Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
> Portions Copyright (c) 1994, The Regents of the University of 
> California
> 

I think so, it contains some older code parts, so it is better to use 
unified copyrights.

> 
> +++ b/src/common/archive.c
> [...]
> +#include "postgres.h"
> +
> +#include "common/archive.h"
> This is incorrect.  All files shared between the backend and the
> frontend in src/common/ have to include the following set of headers:
> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #endif
> 
> +++ b/src/common/fe_archive.c
> [...]
> +#include "postgres_fe.h"
> This is incomplete.  The following piece should be added:
> #ifndef FRONTEND
> #error "This file is not expected to be compiled for backend code"
> #endif
> 

Fixed both.

> 
> +               snprintf(postgres_cmd, sizeof(postgres_cmd), "%s -D %s
> -C restore_command",
> +                                postgres_exec_path, datadir_target);
> +
> I think that this is missing proper quoting.
> 

Yep, added the same quoting as in pg_upgrade/options.

> 
> I would rename ConstructRestoreCommand() to BuildRestoreCommand()
> while on it..
> 

OK, shorter is better.

> 
> I think that it would be saner to check the return status of
> ConstructRestoreCommand() in xlogarchive.c as a sanity check, with an
> elog(ERROR) if not 0, as that should never happen.
> 

Added.

New version of the patch is attached. Thanks again for your review.


Regards
--
Alexey Kondratov

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

On 2020-02-27 16:41, Alexey Kondratov wrote:
> 
> New version of the patch is attached. Thanks again for your review.
> 

Last patch (v18) got a conflict with one of today commits (05d8449e73). 
Rebased version is attached.

--
Alexey Kondratov

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

On Thu, Feb 27, 2020 at 06:29:34PM +0300, Alexey Kondratov wrote:
> On 2020-02-27 16:41, Alexey Kondratov wrote:
> >
> > New version of the patch is attached. Thanks again for your review.
> >
>
> Last patch (v18) got a conflict with one of today commits (05d8449e73).
> Rebased version is attached.

The shape of the patch is getting better.  I have found some issues
when reading through the patch, but nothing huge.

+   printf(_("  -c, --restore-target-wal       use restore_command in target config\n"));
+   printf(_("                                 to retrieve WAL files from archive\n"));
[...]
        {"progress", no_argument, NULL, 'P'},
+       {"restore-target-wal", no_argument, NULL, 'c'},
It may be better to reorder that alphabetically.

+   if (rc != 0)
+       /* Sanity check, should never happen. */
+       elog(ERROR, "failed to build restore_command due to missing parameters");
No point in having this comment IMO.

+/* logging support */
+#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0)
Actually, I don't think that this is a good idea to name this
pg_fatal() as we have the same think in pg_rewind so it could be
confusing.

-   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
    &option_index)) != -1)
+   while ((c = getopt_long(argc, argv, "D:nNPRc", long_options,
    &option_index)) != -1)
Alphabetical order here.

+       rmdir($node_master->archive_dir);
rmtree() is used in all our other tests.

+               pg_log_error("archive file \"%s\" has wrong size: %lu instead of %lu, %s",
+                            xlogfname, (unsigned long) stat_buf.st_size,
+                            (unsigned long) expectedSize, strerror(errno));
I think that the error message should be reworded: "unexpected WAL
file size for \"%s\": %lu instead of %lu".  Please note that there is
no need for strerror() here at all, as errno should be 0.

+    if (xlogfd < 0)
+        pg_log_error("could not open file \"%s\" restored from archive: %s\n",
+                     xlogpath, strerror(errno));
[...]
+    pg_log_error("could not stat file \"%s\" restored from archive: %s",
+                        xlogpath, strerror(errno));
No need for strerror() as you can just use %m.  And no need for the
extra newline at the end as pg_log_* routines do that by themselves.

+   pg_log_error("could not restore file \"%s\" from archive\n",
+                xlogfname);
No need for a newline here.

--
Michael

On 2020-02-28 09:43, Michael Paquier wrote:
> On Thu, Feb 27, 2020 at 06:29:34PM +0300, Alexey Kondratov wrote:
>> On 2020-02-27 16:41, Alexey Kondratov wrote:
>> >
>> > New version of the patch is attached. Thanks again for your review.
>> >
>> 
>> Last patch (v18) got a conflict with one of today commits 
>> (05d8449e73).
>> Rebased version is attached.
> 
> The shape of the patch is getting better.  I have found some issues
> when reading through the patch, but nothing huge.
> 
> +   printf(_("  -c, --restore-target-wal       use restore_command in
> target config\n"));
> +   printf(_("                                 to retrieve WAL files
> from archive\n"));
> [...]
>         {"progress", no_argument, NULL, 'P'},
> +       {"restore-target-wal", no_argument, NULL, 'c'},
> It may be better to reorder that alphabetically.
> 

Sure, I put it in order. However, the recent -R option is out of order 
too.

> 
> +   if (rc != 0)
> +       /* Sanity check, should never happen. */
> +       elog(ERROR, "failed to build restore_command due to missing
> parameters");
> No point in having this comment IMO.
> 

I would prefer to keep it, since there are plenty of similar comments 
near Asserts and elogs all over the Postgres. Otherwise it may look like 
a valid error state. It may be obvious now, but for someone who is not 
aware of BuildRestoreCommand refactoring it may be not. So from my 
perspective there is nothing bad in this extra one line comment.

> 
> +/* logging support */
> +#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } 
> while(0)
> Actually, I don't think that this is a good idea to name this
> pg_fatal() as we have the same think in pg_rewind so it could be
> confusing.
> 

I have added explicit exit(1) calls, since pg_fatal was used only twice 
in the archive.c. Probably, pg_log_fatal from common/logging should obey 
the same logic as FATAL log-level in the backend and do exit the 
process, but for now including pg_rewind.h inside archive.c or vice 
versa does not look like a solution.

> 
> -   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
>     &option_index)) != -1)
> +   while ((c = getopt_long(argc, argv, "D:nNPRc", long_options,
>     &option_index)) != -1)
> Alphabetical order here.
> 

Done.

> 
> +       rmdir($node_master->archive_dir);
> rmtree() is used in all our other tests.
> 

Done. There was an unobvious logic that rmdir only deletes empty 
directories, which is true in the case of archive_dir in that test, but 
I have unified it for consistency.

> 
> +               pg_log_error("archive file \"%s\" has wrong size: %lu
> instead of %lu, %s",
> +                            xlogfname, (unsigned long) 
> stat_buf.st_size,
> +                            (unsigned long) expectedSize, 
> strerror(errno));
> I think that the error message should be reworded: "unexpected WAL
> file size for \"%s\": %lu instead of %lu".  Please note that there is
> no need for strerror() here at all, as errno should be 0.
> 
> +    if (xlogfd < 0)
> +        pg_log_error("could not open file \"%s\" restored from 
> archive: %s\n",
> +                     xlogpath, strerror(errno));
> [...]
> +    pg_log_error("could not stat file \"%s\" restored from archive: 
> %s",
> +                        xlogpath, strerror(errno));
> No need for strerror() as you can just use %m.  And no need for the
> extra newline at the end as pg_log_* routines do that by themselves.
> 
> +   pg_log_error("could not restore file \"%s\" from archive\n",
> +                xlogfname);
> No need for a newline here.
> 

Thanks, I have cleaned up these log statements.


--
Alexey Kondratov

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

On Fri, Feb 28, 2020 at 03:37:47PM +0300, Alexey Kondratov wrote:
> I would prefer to keep it, since there are plenty of similar comments near
> Asserts and elogs all over the Postgres. Otherwise it may look like a valid
> error state. It may be obvious now, but for someone who is not aware of
> BuildRestoreCommand refactoring it may be not. So from my perspective there
> is nothing bad in this extra one line comment.

elog() is called in the event of errors which should never happen by
definition, so your comment is just a duplication of this state.  I'd
still remove that.

> I have added explicit exit(1) calls, since pg_fatal was used only twice in
> the archive.c. Probably, pg_log_fatal from common/logging should obey the
> same logic as FATAL log-level in the backend and do exit the process, but
> for now including pg_rewind.h inside archive.c or vice versa does not look
> like a solution.

archive.c should not depend on anything from src/bin/.

> + * For fixed-size files, the caller may pass the expected size as an
> + * additional crosscheck on successful recovery.  If the file size is not
> + * known, set expectedSize = 0.
> + */
> +int
> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
> +                       off_t expectedSize, const char *restoreCommand)

Actually, expectedSize is IMO a bad idea, because any caller of this
routine passing down zero could be trapped with an incorrect file
size.  So let's remove the behavior where it is possible to bypass
this sanity check.  We don't need it in pg_rewind either.

> +        if ((output_fp = popen(postgres_cmd, "r")) == NULL ||
> +            fgets(cmd_output, sizeof(cmd_output), output_fp) == NULL)
> +            pg_fatal("could not get restore_command using %s: %s",
> +                     postgres_cmd, strerror(errno));

Still missing one %m here.

> +        /* Remove trailing newline */
> +        if (strchr(cmd_output, '\n') != NULL)
> +            *strchr(cmd_output, '\n') = '\0';

It seems to me that what you are looking here is to use
pg_strip_crlf().  Thinking harder, we have pipe_read_line() in
src/common/exec.c which does the exact same job..

> -    /*
> -     * construct the command to be executed
> -     */

Perhaps you meant "build" here.

> +    if (restore_wal)
> +    {
> +        int            rc;
> +        char        postgres_exec_path[MAXPGPATH],
> +                    postgres_cmd[MAXPGPATH],
> +                    cmd_output[MAXPGPATH];
> +        FILE       *output_fp;
> +
> +        /* Find postgres executable. */
> +        rc = find_other_exec(argv[0], "postgres",
> +                             PG_BACKEND_VERSIONSTR,
> +                             postgres_exec_path);

For readability, I would move that into its own separate routine.
--
Michael

On 2020-03-02 07:53, Michael Paquier wrote:

> 
>> + * For fixed-size files, the caller may pass the expected size as an
>> + * additional crosscheck on successful recovery.  If the file size is 
>> not
>> + * known, set expectedSize = 0.
>> + */
>> +int
>> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
>> +                       off_t expectedSize, const char *restoreCommand)
> 
> Actually, expectedSize is IMO a bad idea, because any caller of this
> routine passing down zero could be trapped with an incorrect file
> size.  So let's remove the behavior where it is possible to bypass
> this sanity check.  We don't need it in pg_rewind either.
> 

OK, sounds reasonable, but just to be clear. I will remove only a 
possibility to bypass this sanity check (with 0), but leave expectedSize 
argument intact. We still need it, since pg_rewind takes WalSegSz from 
ControlFile and should pass it further, am I right?

> 
>> +        /* Remove trailing newline */
>> +        if (strchr(cmd_output, '\n') != NULL)
>> +            *strchr(cmd_output, '\n') = '\0';
> 
> It seems to me that what you are looking here is to use
> pg_strip_crlf().  Thinking harder, we have pipe_read_line() in
> src/common/exec.c which does the exact same job..
> 

pg_strip_crlf fits well, but would you mind if I also make 
pipe_read_line external in this patch?

> 
>> -    /*
>> -     * construct the command to be executed
>> -     */
> 
> Perhaps you meant "build" here.
> 

Actually, the verb 'construct' is used historically applied to 
archive/restore commands (see also xlogarchive.c and pgarch.c), but it 
should be 'build' in (fe_)archive.c, since we have BuildRestoreCommand 
there now.

All other remarks look clear for me, so I fix them in the next patch 
version, thanks.


Regards
--
Alexey Kondratov

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

On Mon, Mar 02, 2020 at 08:59:49PM +0300, Alexey Kondratov wrote:
> OK, sounds reasonable, but just to be clear. I will remove only a
> possibility to bypass this sanity check (with 0), but leave expectedSize
> argument intact. We still need it, since pg_rewind takes WalSegSz from
> ControlFile and should pass it further, am I right?

We need to keep around the argument, but I think that we give any user
of this routine a favor by making mandatory a file size.

> pg_strip_crlf fits well, but would you mind if I also make pipe_read_line
> external in this patch?

I got to look at that more, and pipe_read_line() is a nice fit.

> Actually, the verb 'construct' is used historically applied to
> archive/restore commands (see also xlogarchive.c and pgarch.c), but it
> should be 'build' in (fe_)archive.c, since we have BuildRestoreCommand there
> now.
>
> All other remarks look clear for me, so I fix them in the next patch
> version, thanks.

Already done as per the attached, with a new routine named
getRestoreCommand() and more done.  There were a couple of extra
things I noticed on the way:
- Found some typos.
- The translation of pg_rewind --help was incorrect, with a sentence
cut in the middle (you used twice gettext).
- I did not actually get why you don't check for a missing command
when using wait_result_is_any_signal.  In this case I'd think that it
is better to exit immediately as follow-up calls would just fail.
- The code was rather careless about error handling and
RestoreArchivedWALFile(), and it seemed to me that it is rather
pointless to report an extra message "could not restore file \"%s\"
from archive" on top of the other error.
- I could not resist to add more documentation for the new routines of
src/common/..
- Used long options in the tests for readability, reworked the
comments.

On top of that, I have spent some time testing the reliability of the
test, and tested the whole on Windows and Linux.  This is in a rather
committable shape now.
--
Michael

On 04.03.2020 10:45, Michael Paquier wrote:
> On Mon, Mar 02, 2020 at 08:59:49PM +0300, Alexey Kondratov wrote:
>> All other remarks look clear for me, so I fix them in the next patch
>> version, thanks.
> Already done as per the attached, with a new routine named
> getRestoreCommand() and more done.

Many thanks for doing that. I went through the diff between v21 and v20. 
Most of the changes look good to me.

- *        Functions for finding and validating executable files
+ *        Functions for finding and validating from executables files

There is probably something missing here. Finding and validating what? 
And 'executables files' does not seem to be correct as well.

+        # First, remove all the content in the archive directory,
+        # as RecursiveCopy::copypath does not support copying to
+        # existing directories.

I think that 'remove all the content' is not completely correct in this 
case. We are simply removing archive directory. There is no content 
there yet, so 'First, remove archive directory...' should be fine.

> - I did not actually get why you don't check for a missing command
> when using wait_result_is_any_signal.  In this case I'd think that it
> is better to exit immediately as follow-up calls would just fail.

Believe me or not, but I put 'false' there intentionally. The idea was 
that if the reason is a signal, then maybe user tired of waiting and 
killed that restore_command process theirself or something like that, so 
it is better to exit immediately. If it was a missing command, then 
there is no hurry, so we can go further and complain that attempt of 
recovering WAL segment has failed.

Actually, I guess that there is no big difference if we include missing 
command here or not. There is no complicated logic further compared to 
real recovery process in Postgres, where we cannot simply return false 
in that case.

> - The code was rather careless about error handling and
> RestoreArchivedWALFile(), and it seemed to me that it is rather
> pointless to report an extra message "could not restore file \"%s\"
> from archive" on top of the other error.

Probably you mean several pg_log_error calls not followed by 'return 
-1;'. Yes, I did it to fall down to the function end and show this extra 
message, but I agree that there is no much sense in doing so.


Regards

-- 
Alexey Kondratov

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

On Wed, Mar 04, 2020 at 08:14:20PM +0300, Alexey Kondratov wrote:
> On 04.03.2020 10:45, Michael Paquier wrote:
> - *        Functions for finding and validating executable files
> + *        Functions for finding and validating from executables files
>
> There is probably something missing here. Finding and validating what? And
> 'executables files' does not seem to be correct as well.

Oops.  I was tweaking this part at some point of my review, but
discarded the change afterwards.

> +        # First, remove all the content in the archive directory,
> +        # as RecursiveCopy::copypath does not support copying to
> +        # existing directories.
>
> I think that 'remove all the content' is not completely correct in this
> case. We are simply removing archive directory. There is no content there
> yet, so 'First, remove archive directory...' should be fine.

Indeed, this can be improved.  I still need to do an extra pass on
this patch set.

>> - I did not actually get why you don't check for a missing command
>> when using wait_result_is_any_signal.  In this case I'd think that it
>> is better to exit immediately as follow-up calls would just fail.
>
> Believe me or not, but I put 'false' there intentionally. The idea was that
> if the reason is a signal, then maybe user tired of waiting and killed that
> restore_command process theirself or something like that, so it is better to
> exit immediately. If it was a missing command, then there is no hurry, so we
> can go further and complain that attempt of recovering WAL segment has
> failed.
>
> Actually, I guess that there is no big difference if we include missing
> command here or not. There is no complicated logic further compared to real
> recovery process in Postgres, where we cannot simply return false in that
> case.

On the contrary, it seems to me that the difference is very important.
Imagine for example a frontend tool which calls RestoreArchivedWALFile
in a loop, and that this one fails because the command called is
missing.  This tool would keep looping for nothing.  So checking for a
missing command and leaving immediately would be more helpful for the
user.  Can you think about scenarios where it would make sense to be
able to loop in this case instead of failing?
--
Michael

On 05.03.2020 09:24, Michael Paquier wrote:
> On Wed, Mar 04, 2020 at 08:14:20PM +0300, Alexey Kondratov wrote:
>>> - I did not actually get why you don't check for a missing command
>>> when using wait_result_is_any_signal.  In this case I'd think that it
>>> is better to exit immediately as follow-up calls would just fail.
>> Believe me or not, but I put 'false' there intentionally. The idea was that
>> if the reason is a signal, then maybe user tired of waiting and killed that
>> restore_command process theirself or something like that, so it is better to
>> exit immediately. If it was a missing command, then there is no hurry, so we
>> can go further and complain that attempt of recovering WAL segment has
>> failed.
>>
>> Actually, I guess that there is no big difference if we include missing
>> command here or not. There is no complicated logic further compared to real
>> recovery process in Postgres, where we cannot simply return false in that
>> case.
> On the contrary, it seems to me that the difference is very important.
> Imagine for example a frontend tool which calls RestoreArchivedWALFile
> in a loop, and that this one fails because the command called is
> missing.  This tool would keep looping for nothing.  So checking for a
> missing command and leaving immediately would be more helpful for the
> user.  Can you think about scenarios where it would make sense to be
> able to loop in this case instead of failing?


OK, I was still having in mind pg_rewind as the only one user of this 
routine. Now it is a part of the common and I could imagine a 
hypothetical tool that is polling the archive and waiting for a specific 
WAL segment to become available. In this case 'command not found' is 
definitely the end of game, while the absence of segment is expected 
error, so we can continue looping.


Regards

-- 
Alexey Kondratov

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

On Thu, Mar 05, 2020 at 07:52:24PM +0300, Alexey Kondratov wrote:
> OK, I was still having in mind pg_rewind as the only one user of this
> routine. Now it is a part of the common and I could imagine a hypothetical
> tool that is polling the archive and waiting for a specific WAL segment to
> become available. In this case 'command not found' is definitely the end of
> game, while the absence of segment is expected error, so we can continue
> looping.

Depending on the needs, we could also add in the future an option in
this API to not exit if the command is missing.  Who knows if a case
can be made..

By the way, as a matter of transparency, I have discussed this patch
with Alexander offlist and he has pinged me that I may be in a better
place to commit it per the amount of review I have done, so I'll try
to handle it.  I was also thinking to split the patch into two pieces:
- Introduction of common/archive.c and common/fe_archive.c (the former
is used by xlogarchive.c and the latter only by pg_rewind).  The
latter is dead code without the second patch, but this would validate
the MSVC build with the buildfarm.
- The pg_rewind pieces with the new option.
--
Michael

On 2020-Mar-06, Michael Paquier wrote:

> I was also thinking to split the patch into two pieces:
> - Introduction of common/archive.c and common/fe_archive.c (the former
> is used by xlogarchive.c and the latter only by pg_rewind).  The
> latter is dead code without the second patch, but this would validate
> the MSVC build with the buildfarm.
> - The pg_rewind pieces with the new option.

Hmm, doesn't the CF bot already validate the MSVC build?

Splitting in two seems all right, but I think one commit that introduces
dead code is not great.  It may make more sense to have one commit for
common/archive.c, and a second commit that does fe_archive plus
pg_rewind changes ...  If that doesn't work for whatever reason, then
doing a single commit may be preferrable.

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

On Thu, Mar 05, 2020 at 11:09:06PM -0300, Alvaro Herrera wrote:
> Hmm, doesn't the CF bot already validate the MSVC build?
>
> Splitting in two seems all right, but I think one commit that introduces
> dead code is not great.  It may make more sense to have one commit for
> common/archive.c, and a second commit that does fe_archive plus
> pg_rewind changes ...  If that doesn't work for whatever reason, then
> doing a single commit may be preferrable.

Thanks for the suggestion.  Avoiding dead code makes sense as well
here.  I'll think about this stuff a bit more first.
--
Michael

On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote:
> Thanks for the suggestion.  Avoiding dead code makes sense as well
> here.  I'll think about this stuff a bit more first.

Okay, after pondering more on that, here is a first cut with a patch
refactoring restore_command build to src/common/.  One thing I have
changed compared to the other versions is that BuildRestoreCommand()
now returns a boolean to tell if the command was successfully built or
not.

A second thing.  As of now the interface of BuildRestoreCommand()
assumes that the resulting buffer has an allocation of MAXPGPATH.
This should be fine because that's an assumption we rely on a lot in
the code, be it frontend or backend.  However, it could also be a trap
for any caller of this routine if they allocate something smaller.
Wouldn't it be cleaner to pass down the size of the result buffer
directly to BuildRestoreCommand() and use the length given by the
caller of the routine as a sanity check?

Any thoughts?
--
Michael

On Mon, Mar 9, 2020 at 1:46 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote:
> > Thanks for the suggestion.  Avoiding dead code makes sense as well
> > here.  I'll think about this stuff a bit more first.
>
> Okay, after pondering more on that, here is a first cut with a patch
> refactoring restore_command build to src/common/.  One thing I have
> changed compared to the other versions is that BuildRestoreCommand()
> now returns a boolean to tell if the command was successfully built or
> not.
>
Yeah. If we're returning only -1 and 0, it's better to use a boolean.
If we're planning to provide some information about which parameter is
missing, a few negative integer values might be useful. But, I feel
that's unnecessary in this case.

> A second thing.  As of now the interface of BuildRestoreCommand()
> assumes that the resulting buffer has an allocation of MAXPGPATH.
> This should be fine because that's an assumption we rely on a lot in
> the code, be it frontend or backend.  However, it could also be a trap
> for any caller of this routine if they allocate something smaller.
> Wouldn't it be cleaner to pass down the size of the result buffer
> directly to BuildRestoreCommand() and use the length given by the
> caller of the routine as a sanity check?
>
That's a good suggestion. But, it's unlikely that a caller would pass
something longer than MAXPGPATH and we indeed use that value a lot in
the code. IMHO, it looks okay to me to have that assumption here as
well.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

On Mon, Mar 09, 2020 at 03:38:29PM +0530, Kuntal Ghosh wrote:
> That's a good suggestion. But, it's unlikely that a caller would pass
> something longer than MAXPGPATH and we indeed use that value a lot in
> the code. IMHO, it looks okay to me to have that assumption here as
> well.

Well, a more serious problem would be to allocate something smaller
than MAXPGPATH.  This reminds me a bit of 09ec55b9 where we did not
correctly design from the start the base64 encode and decode routines
for SCRAM, so I'd rather design this one correctly from the start as
per the attached.  Alexey, Alexander, what do you think?
--
Michael

On Tue, Mar 10, 2020 at 7:28 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Mar 09, 2020 at 03:38:29PM +0530, Kuntal Ghosh wrote:
> > That's a good suggestion. But, it's unlikely that a caller would pass
> > something longer than MAXPGPATH and we indeed use that value a lot in
> > the code. IMHO, it looks okay to me to have that assumption here as
> > well.
>
> Well, a more serious problem would be to allocate something smaller
> than MAXPGPATH.  This reminds me a bit of 09ec55b9 where we did not
> correctly design from the start the base64 encode and decode routines
> for SCRAM, so I'd rather design this one correctly from the start as
> per the attached.  Alexey, Alexander, what do you think?

Two options seem reasonable to me in this case.  The first is to pass
length as additional argument as you did.  The second option is to
make argument a pointer to fixed-size array as following.

extern bool BuildRestoreCommand(const char *restoreCommand,
                                const char *xlogpath,   /* %p */
                                const char *xlogfname,  /* %f */
                                const char *lastRestartPointFname,  /* %r */
                                char (*commandResult)[MAXPGPATH]);

Passing pointer to array of different size would cause an error.  The
downside of this approach is that passing palloc'd chunk of memory as
commandResult would be less convenient.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote:
> Two options seem reasonable to me in this case.  The first is to pass
> length as additional argument as you did.  The second option is to
> make argument a pointer to fixed-size array as following.
>
> extern bool BuildRestoreCommand(const char *restoreCommand,
>                                 const char *xlogpath,   /* %p */
>                                 const char *xlogfname,  /* %f */
>                                 const char *lastRestartPointFname,  /* %r */
>                                 char (*commandResult)[MAXPGPATH]);
>
> Passing pointer to array of different size would cause an error.  The
> downside of this approach is that passing palloc'd chunk of memory as
> commandResult would be less convenient.

Thanks Alexander for the input.  Another argument is that what you are
suggesting here is not the usual project style, so I would still stick
with two arguments to be consistent.
--
Michael

On Tue, Mar 10, 2020 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote:
> > Two options seem reasonable to me in this case.  The first is to pass
> > length as additional argument as you did.  The second option is to
> > make argument a pointer to fixed-size array as following.
> >
> > extern bool BuildRestoreCommand(const char *restoreCommand,
> >                                 const char *xlogpath,   /* %p */
> >                                 const char *xlogfname,  /* %f */
> >                                 const char *lastRestartPointFname,  /* %r */
> >                                 char (*commandResult)[MAXPGPATH]);
> >
> > Passing pointer to array of different size would cause an error.  The
> > downside of this approach is that passing palloc'd chunk of memory as
> > commandResult would be less convenient.
>
> Thanks Alexander for the input.  Another argument is that what you are
> suggesting here is not the usual project style, so I would still stick
> with two arguments to be consistent.

Yes, another argument is valid as well.  I'm OK with current solution.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-Mar-10, Michael Paquier wrote:

> On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote:
> > Two options seem reasonable to me in this case.  The first is to pass
> > length as additional argument as you did.  The second option is to
> > make argument a pointer to fixed-size array as following.

Another option is to return the command as a palloc'ed string (per
psprintf), instead of using a caller-stack-allocated variable.  Passing
the buffer len is widely used, but more error prone (and I think getting
this one wrong might be more catastrophic than a mistake elsewhere.)
This is not a performance-critical path enough that we *need* the
optimization that avoids the palloc is important.  (Failure can be
reported by returning NULL.)  Also, I think the function comment could
stand some more detailing.

Also, I think Msvcbuild.pm could follow Makefile's ideas of one line per
file.  Maybe no need to fix all of that in this patch, but let's start
by adding the new file it its own line rather than reflowing two
adjacent lines (oh wait ... does perltidy put it that way?  if so,
nevermind.)

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

On Tue, Mar 10, 2020 at 12:39:53PM -0300, Alvaro Herrera wrote:
> Another option is to return the command as a palloc'ed string (per
> psprintf), instead of using a caller-stack-allocated variable.  Passing
> the buffer len is widely used, but more error prone (and I think getting
> this one wrong might be more catastrophic than a mistake elsewhere.)
> This is not a performance-critical path enough that we *need* the
> optimization that avoids the palloc is important.  (Failure can be
> reported by returning NULL.)

That's a better approach here.

> Also, I think the function comment could stand some more detailing.

What kind of additional information would you like to add on top of
what the updated version attached does?

> Also, I think Msvcbuild.pm could follow Makefile's ideas of one line per
> file.  Maybe no need to fix all of that in this patch, but let's start
> by adding the new file it its own line rather than reflowing two
> adjacent lines (oh wait ... does perltidy put it that way?  if so,
> nevermind.)

Good idea.  It happens that perltidy does not care about that, but I
would rather keep that stuff for a separate patch/thread.
--
Michael

On Wed, Mar 11, 2020 at 12:27:03PM +0900, Michael Paquier wrote:
>> Also, I think the function comment could stand some more detailing.
>
> What kind of additional information would you like to add on top of
> what the updated version attached does?

I have been working more on that, and attached is an updated version
of the main feature based on 0001, attached as 0002.  I have applied
the following changes, fixing some actual bugs while on it:
- Added back expectedSize, so as frontends can also fetch things other
than WAL segments from the archives, including history files.
pg_rewind needs only WAL segments, still other tools may want more.
- Fixed an issue with ENOENT when doing stat() on a segment after
running the restore command.  Like the backend, there is an argument
for looping here.
- Switched the several failures to issue exit() for the several
failure types possible instead of looping.  If a file has an incorrect
expected size or that stat() fails for a non-ENOENT, things are more
likely going to repeat if the frontend tool loops, so exiting
immediately is safer.

I think that we need a better name for RestoreArchivedFile() in
fe_archive.c.  The backend uses RestoreArchivedFile(), and the
previous versions of the patch switched to RestoreArchivedWALFile()
which is incorrect if working on something else than a WAL segment.
The version attached uses FrontendRestoreArchivedFile(), still we
could do better.

I'd like to commit the refactoring piece in 0001 tomorrow, then let's
move on with the rest as of 0002.  If more comments and docs are
needed for archive.c, let's continue discussing that.
--
Michael

On 12.03.2020 07:39, Michael Paquier wrote:
> I'd like to commit the refactoring piece in 0001 tomorrow, then let's
> move on with the rest as of 0002.  If more comments and docs are
> needed for archive.c, let's continue discussing that.

I just went through the both patches and realized that I cannot get into 
semantics of splitting frontend code between common and fe_utils. This 
applies only to 0002, where we introduce fe_archive.c. Should it be 
placed into fe_utils alongside of the recent recovery_gen.c also used by 
pg_rewind? This is a frontend-only code intended to be used by frontend 
applications, so fe_utils feels like the right place, doesn't it? Just 
tried to do so and everything went fine, so it seems that there is no 
obstacles from the build system.

BTW, most of 'common' is a really common code with only four exceptions 
like logging.c, which is frontend-only. Is it there for historical 
reasons only or something else?


Regards

-- 
Alexey Kondratov

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

On 2020-Mar-11, Michael Paquier wrote:

> On Tue, Mar 10, 2020 at 12:39:53PM -0300, Alvaro Herrera wrote:
> > Another option is to return the command as a palloc'ed string (per
> > psprintf), instead of using a caller-stack-allocated variable.  Passing
> > the buffer len is widely used, but more error prone (and I think getting
> > this one wrong might be more catastrophic than a mistake elsewhere.)
> > This is not a performance-critical path enough that we *need* the
> > optimization that avoids the palloc is important.  (Failure can be
> > reported by returning NULL.)
> 
> That's a better approach here.

Thanks, looks good.  I don't think we *need* the MAXPGPATH restriction
really -- I was thinking in a StringInfo kind of approach where you just
append the stuff you need without having to think about the buffer
length.

> > Also, I think Msvcbuild.pm could follow Makefile's ideas of one line per
> > file.  Maybe no need to fix all of that in this patch, but let's start
> > by adding the new file it its own line rather than reflowing two
> > adjacent lines (oh wait ... does perltidy put it that way?  if so,
> > nevermind.)
> 
> Good idea.  It happens that perltidy does not care about that, but I
> would rather keep that stuff for a separate patch/thread.

Aha, good.  I would still put the new "archive.c" entry on its own line,
and just keep the other two lines unchanged.  (That preserves the
perhaps non-obvious property that all entries that start with the same
letter are in the same line.)

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

On Thu, Mar 12, 2020 at 12:50:17PM -0300, Alvaro Herrera wrote:
> Thanks, looks good.  I don't think we *need* the MAXPGPATH restriction
> really -- I was thinking in a StringInfo kind of approach where you just
> append the stuff you need without having to think about the buffer
> length.

Oh, OK.  I missed your point then.  No problem with that by doing it
as the attached.  There is one small trick because of
make_native_path() though.

> Aha, good.  I would still put the new "archive.c" entry on its own line,
> and just keep the other two lines unchanged.  (That preserves the
> perhaps non-obvious property that all entries that start with the same
> letter are in the same line.)

No issues with that either.  Are you fine with the updated version
attached for 0001?
--
Michael

Dear Alvaro,

On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier <michael@paquier.xyz> wrote:
> No issues with that either.  Are you fine with the updated version
> attached for 0001?

I think patchset is almost ready for commit.  Michael is waiting for
your answer on whether you're fine with current shape of 0001.  Could
you please provide a feedback?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-Mar-23, Alexander Korotkov wrote:

> Dear Alvaro,
> 
> On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier <michael@paquier.xyz> wrote:
> > No issues with that either.  Are you fine with the updated version
> > attached for 0001?
> 
> I think patchset is almost ready for commit.  Michael is waiting for
> your answer on whether you're fine with current shape of 0001.  Could
> you please provide a feedback?

Hi, I didn't realize that this was waiting on me.  It looks good to me
-- I'd just remove the comment on the function prototype in archiver.h,
which is not customary (we only put these comments in the .c file).
Let's get it pushed.

Thanks for everybody's work on this.

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

On Mon, Mar 23, 2020 at 6:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Mar-23, Alexander Korotkov wrote:
> > On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > No issues with that either.  Are you fine with the updated version
> > > attached for 0001?
> >
> > I think patchset is almost ready for commit.  Michael is waiting for
> > your answer on whether you're fine with current shape of 0001.  Could
> > you please provide a feedback?
>
> Hi, I didn't realize that this was waiting on me.  It looks good to me
> -- I'd just remove the comment on the function prototype in archiver.h,
> which is not customary (we only put these comments in the .c file).
> Let's get it pushed.
>
> Thanks for everybody's work on this.

Great, thank you!

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Mar 23, 2020 at 07:17:13PM +0300, Alexander Korotkov wrote:
> On Mon, Mar 23, 2020 at 6:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Hi, I didn't realize that this was waiting on me.  It looks good to me
>> -- I'd just remove the comment on the function prototype in archiver.h,
>> which is not customary (we only put these comments in the .c file).
>> Let's get it pushed.

Thanks.  I have removed the comment in archive.h.

>> Thanks for everybody's work on this.
>
> Great, thank you!

Thanks Alvaro and Alexander.  0001 has been applied as of e09ad07.
Now for 0002, let's see about it later.  Attached is a rebased
version, with no actual changes.
--
Michael

On Tue, Mar 24, 2020 at 12:22:16PM +0900, Michael Paquier wrote:
> Thanks Alvaro and Alexander.  0001 has been applied as of e09ad07.
> Now for 0002, let's see about it later.  Attached is a rebased
> version, with no actual changes.

I was looking at this patch again today and I am rather fine with the
existing semantics.  Still I don't like much to name the frontend-side
routine FrontendRestoreArchivedFile() and use a different name than
the backend counterpart because we have to include xlog_internal.h in
fe_archive.c to be able to grab XLOGDIR.

So here is an idea: let's move the declaration of the routines part of
xlogarchive.c to a new header, called xlogarchive.h, and then let's
use the same routine name for the frontend and the backend in this
second patch.  We include xlog_internal.h already in many frontend
tools, so that would clean up things a bit.  Two extra things are the
routines for the checkpointer as well as the variables like
ArchiveRecoveryRequested.  It may be nice to move those while on it,
but I am not sure where and that's not actually required for this
patch set so that could be addressed later if need be.

Any thoughts?
--
Michael

On 2020-03-26 10:34, Michael Paquier wrote:
> On Tue, Mar 24, 2020 at 12:22:16PM +0900, Michael Paquier wrote:
>> Thanks Alvaro and Alexander.  0001 has been applied as of e09ad07.
>> Now for 0002, let's see about it later.  Attached is a rebased
>> version, with no actual changes.
> 
> I was looking at this patch again today and I am rather fine with the
> existing semantics.  Still I don't like much to name the frontend-side
> routine FrontendRestoreArchivedFile() and use a different name than
> the backend counterpart because we have to include xlog_internal.h in
> fe_archive.c to be able to grab XLOGDIR.
> 
> So here is an idea: let's move the declaration of the routines part of
> xlogarchive.c to a new header, called xlogarchive.h, and then let's
> use the same routine name for the frontend and the backend in this
> second patch.  We include xlog_internal.h already in many frontend
> tools, so that would clean up things a bit.  Two extra things are the
> routines for the checkpointer as well as the variables like
> ArchiveRecoveryRequested.  It may be nice to move those while on it,
> but I am not sure where and that's not actually required for this
> patch set so that could be addressed later if need be.
> 
> Any thoughts?
> 

The block of function declarations for xlogarchive.c inside 
xlog_internal.h looks a bit dangling already, since all other functions 
and variables defined/initialized in xlog.c. That way, it looks good to 
me to move it outside.

The only one concern about using the same name I have is that later 
someone may introduce a new variable or typedef inside xlogarchive.h. So 
someone else would be required to include both fe_archive.h and 
xlogarchive.h at once. But probably there should be a warning in the 
header comment section against doing so.

Anyway, I have tried to do what you proposed and attached is a small 
patch, that introduces xlogarchive.h.


Regards
-- 
Alexey Kondratov

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

On 2020-Mar-26, Michael Paquier wrote:

> I was looking at this patch again today and I am rather fine with the
> existing semantics.  Still I don't like much to name the frontend-side
> routine FrontendRestoreArchivedFile() and use a different name than
> the backend counterpart because we have to include xlog_internal.h in
> fe_archive.c to be able to grab XLOGDIR.

Uh, is XLOGDIR the only reason to include xlog_internal.h?  Maybe it
would be easier to have a routine in xlog.c that returns the path?
There's a few functions in xlog.c that could use it, as well.

Altough ... looking at xlog.h, that one is even worse, so I'm not sure
*where* you'd put the prototype for the new function I'm proposing.

> So here is an idea: let's move the declaration of the routines part of
> xlogarchive.c to a new header, called xlogarchive.h, and then let's
> use the same routine name for the frontend and the backend in this
> second patch.  We include xlog_internal.h already in many frontend
> tools, so that would clean up things a bit.

The patch downthread looks decent cleanup, but I'm not sure how it helps
further the objective.

(A really good cleanup could be a situation where frontend files don't
need xlog_internal.h -- for example, maybe a new file xlogpage.h could
contain struct defs that relate to page and segment headers and the
like, as well as useful macros.  I don't know if this can be made to
work -- but xlog_internal.h contains stuff like xl_parameter_change etc
as well as RmgrData which surely are of no interest to readers of wal
files ... or, say, RequestXLogSwitch.)

I don't think any such cleanup should hamper the patch at hand anyway.

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

On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
> Uh, is XLOGDIR the only reason to include xlog_internal.h?  Maybe it
> would be easier to have a routine in xlog.c that returns the path?
> There's a few functions in xlog.c that could use it, as well.

Yep, that's all.  We could also just hardcode the path as we did when
we worked on the exclusion filter lists for pg_rewind and basebackup.c,
though I'd prefer avoid that if possible.

> The patch downthread looks decent cleanup, but I'm not sure how it helps
> further the objective.

I actually think it does, because you get out of the way the conflicts
caused by RestoreArchivedFile() in the backend, as we are targetting
for fe_archive.c to be a frontend-only file, though I agree that it is
not the full of it.

> (A really good cleanup could be a situation where frontend files don't
> need xlog_internal.h -- for example, maybe a new file xlogpage.h could
> contain struct defs that relate to page and segment headers and the
> like, as well as useful macros.  I don't know if this can be made to
> work -- but xlog_internal.h contains stuff like xl_parameter_change etc
> as well as RmgrData which surely are of no interest to readers of wal
> files ... or, say, RequestXLogSwitch.)

A second header that could be created is xlogpaths.h (or xlogfiles.h?)
that includes all the routines and variables we use to build WAL
segment names and such, with XLogFileNameById, IsTLHistoryFileName,
etc.  I agree that splitting the record-related parts may make sense,
say xlogrecovery.h?

> I don't think any such cleanup should hamper the patch at hand anyway.

I don't think either, so I would do the split for the archive routines
at least.  It still feels strange to me to have a different routine
name for the frontend-side of RestoreArchivedFile().  That's not
really consistent with the current practice we have palloc() & co, as
well as the sync routines.
--
Michael

On Fri, Mar 27, 2020 at 12:24:19AM +0300, Alexey Kondratov wrote:
> The block of function declarations for xlogarchive.c inside xlog_internal.h
> looks a bit dangling already, since all other functions and variables
> defined/initialized in xlog.c. That way, it looks good to me to move it
> outside.

Yep, exactly my point of view.

> The only one concern about using the same name I have is that later someone
> may introduce a new variable or typedef inside xlogarchive.h. So someone
> else would be required to include both fe_archive.h and xlogarchive.h at
> once. But probably there should be a warning in the header comment section
> against doing so.
>
> Anyway, I have tried to do what you proposed and attached is a small patch,
> that introduces xlogarchive.h.

Thanks for sending a patch, that's the split I would have done.

+#include "access/xlogdefs.h"
Oh, I see.  You need that in xlogarchive.h for XLogSegNo.  Makes sense.

+ * xlogarchive.h
+ *     Prototypes of functions for archiving WAL files and restoring
+ *     from the archive.
The only tweak I would have done here is to reword that as "Utilities
for interacting with WAL archives in the backend."

Alvaro, Alexander, do you like this split?  FWIW, I do as it is
simple.
--
Michael

On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote:
> On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
>> I don't think any such cleanup should hamper the patch at hand anyway.
>
> I don't think either, so I would do the split for the archive routines
> at least.  It still feels strange to me to have a different routine
> name for the frontend-side of RestoreArchivedFile().  That's not
> really consistent with the current practice we have palloc() & co, as
> well as the sync routines.

Okay.  Hearing nothing, I have rebased the patch set this morning,
improving some comments and the code format while on it.  I would like
to commit both 0001 (creation of xlogarchiver.h) and 0002 attached in
the next couple of days.  If you have an objection, please feel free.
--
Michael

On Mon, Mar 30, 2020 at 4:57 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote:
> > On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
> >> I don't think any such cleanup should hamper the patch at hand anyway.
> >
> > I don't think either, so I would do the split for the archive routines
> > at least.  It still feels strange to me to have a different routine
> > name for the frontend-side of RestoreArchivedFile().  That's not
> > really consistent with the current practice we have palloc() & co, as
> > well as the sync routines.
>
> Okay.  Hearing nothing, I have rebased the patch set this morning,
> improving some comments and the code format while on it.  I would like
> to commit both 0001 (creation of xlogarchiver.h) and 0002 attached in
> the next couple of days.  If you have an objection, please feel free.

I'm fine with patchset attached.  Sorry for late reply.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-03-30 04:56, Michael Paquier wrote:
> On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote:
>> On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
>>> I don't think any such cleanup should hamper the patch at hand 
>>> anyway.
>> 
>> I don't think either, so I would do the split for the archive routines
>> at least.  It still feels strange to me to have a different routine
>> name for the frontend-side of RestoreArchivedFile().  That's not
>> really consistent with the current practice we have palloc() & co, as
>> well as the sync routines.
> 
> Okay.  Hearing nothing, I have rebased the patch set this morning,
> improving some comments and the code format while on it.  I would like
> to commit both 0001 (creation of xlogarchiver.h) and 0002 attached in
> the next couple of days.  If you have an objection, please feel free.
> 

0001:

What do think about adding following sanity check into xlogarchive.c?

+#ifdef FRONTEND
+#error "This file is not expected to be compiled for frontend code"
+#endif

It would prevent someone from adding typedefs and any other common 
definitions into xlogarchive.h in the future, leading to the accidental 
inclusion of both xlogarchive.h and fe_archive.h in the same time.

0002:

+ */
+int
+RestoreArchivedFile(const char *path, const char *xlogfname,
+                            off_t expectedSize, const char *restoreCommand)
+{

There is a couple of extra tabs IMO.

+extern int    RestoreArchivedFile(const char *path,
+                        const char *xlogfname,
+                        off_t expectedSize,
+                        const char *restoreCommand);

And the same here.

+ * This uses a logic based on "postgres -C" to get the value from
+ * from the cluster.

Repetitive 'from'.

  extractPageMap(const char *datadir, XLogRecPtr startpoint, int 
tliIndex,
-               XLogRecPtr endpoint)
+               XLogRecPtr endpoint, const char *restore_command)

Let us use camel case 'restoreCommand' here as in the header for 
tidiness.

I have left 0001 intact, but fixed all these small remarks in the 0002. 
Please, find it attached.


Regards
-- 
Alexey Kondratov

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

On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote:
> What do think about adding following sanity check into xlogarchive.c?
>
> +#ifdef FRONTEND
> +#error "This file is not expected to be compiled for frontend code"
> +#endif
>
> It would prevent someone from adding typedefs and any other common
> definitions into xlogarchive.h in the future, leading to the accidental
> inclusion of both xlogarchive.h and fe_archive.h in the same time.

I don't see much the point as this would fail to compile anyway, and
that's not project-style.  Note that we have already a clear
separation here between the backend and the frontend code here as
xlogarchive.h is backend-only and fe_archive.h is frontend-only.

> 0002:
>
> [format comments]
>
> Let us use camel case 'restoreCommand' here as in the header for tidiness.

All this makes sense, and also note the same formatting issue in
parsexlog.c for RestoreArchivedFile().  A run of pgindent was
missing.

> I have left 0001 intact, but fixed all these small remarks in the 0002.
> Please, find it attached.

Thanks, committed 0001 after fixing the order of the headers.  One
patch left.
--
Michael

On Tue, Mar 31, 2020 at 03:48:21PM +0900, Michael Paquier wrote:
> Thanks, committed 0001 after fixing the order of the headers.  One
> patch left.

And committed now 0002, meaning that we are officially done.  Thanks
Alexey for your patience.
--
Michael

On 2020-04-01 05:19, Michael Paquier wrote:
> On Tue, Mar 31, 2020 at 03:48:21PM +0900, Michael Paquier wrote:
>> Thanks, committed 0001 after fixing the order of the headers.  One
>> patch left.
> 
> And committed now 0002, meaning that we are officially done.  Thanks
> Alexey for your patience.
> 

Thanks for doing that! And to all others who participated.


-- 
Alexey Kondratov

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

On 2020-03-31 08:48, Michael Paquier wrote:
> On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote:
>> What do think about adding following sanity check into xlogarchive.c?
>>
>> +#ifdef FRONTEND
>> +#error "This file is not expected to be compiled for frontend code"
>> +#endif
>>
>> It would prevent someone from adding typedefs and any other common
>> definitions into xlogarchive.h in the future, leading to the accidental
>> inclusion of both xlogarchive.h and fe_archive.h in the same time.
> I don't see much the point as this would fail to compile anyway, and
> that's not project-style.  Note that we have already a clear
> separation here between the backend and the frontend code here as
> xlogarchive.h is backend-only and fe_archive.h is frontend-only.

Why is fe_archive.c in src/common/ and not in src/fe_utils/?  It is not 
"common" to frontend and backend.

It actually defines functions that clash with functions in the backend, 
so this seems doubly wrong.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi!

On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-03-31 08:48, Michael Paquier wrote:
> > On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote:
> >> What do think about adding following sanity check into xlogarchive.c?
> >>
> >> +#ifdef FRONTEND
> >> +#error "This file is not expected to be compiled for frontend code"
> >> +#endif
> >>
> >> It would prevent someone from adding typedefs and any other common
> >> definitions into xlogarchive.h in the future, leading to the accidental
> >> inclusion of both xlogarchive.h and fe_archive.h in the same time.
> > I don't see much the point as this would fail to compile anyway, and
> > that's not project-style.  Note that we have already a clear
> > separation here between the backend and the frontend code here as
> > xlogarchive.h is backend-only and fe_archive.h is frontend-only.
>
> Why is fe_archive.c in src/common/ and not in src/fe_utils/?  It is not
> "common" to frontend and backend.

Yep, this seems wrong to me.  I can propose following file rename.

src/common/fe_archive.c => src/fe_utils/archive.c
include/common/fe_archive.h => include/fe_utils/archive.h

> It actually defines functions that clash with functions in the backend,
> so this seems doubly wrong.


Let's have frontend version of RestoreArchivedFile() renamed as well.
What about RestoreArchivedFileFrontend()?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Jun 07, 2020 at 10:05:03PM +0300, Alexander Korotkov wrote:
> On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Why is fe_archive.c in src/common/ and not in src/fe_utils/?  It is not
>> "common" to frontend and backend.
>
> Yep, this seems wrong to me.  I can propose following file rename.
>
> src/common/fe_archive.c => src/fe_utils/archive.c
> include/common/fe_archive.h => include/fe_utils/archive.h

Is it technically a problem though?  fe_archive.c is listed as a
frontend-only file with OBJS_FRONTEND in src/common/Makefile.  Anyway,
for this one I would not mind to do the move so please see the
attached, but I am not completely sure either why src/fe_utils/ had
better be chosen than src/common/.

>> It actually defines functions that clash with functions in the backend,
>> so this seems doubly wrong.
>
> Let's have frontend version of RestoreArchivedFile() renamed as well.
> What about RestoreArchivedFileFrontend()?

-1 from me for the renaming, which was intentional to ease grepping
with the backend counterpart.  We have other cases like that, say
palloc(), fsync_fname(), etc.

Perhaps we have more things that are frontend-only in src/common/ that
could be moved to src/fe_utils/?  I am looking at restricted_token.c,
fe_memutils.c, logging.c and file_utils.c here, but that would mean
breaking a couple of declarations, something never free for plugin
developers.
--
Michael

On 2020-06-08 09:14, Michael Paquier wrote:
> On Sun, Jun 07, 2020 at 10:05:03PM +0300, Alexander Korotkov wrote:
>> On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> Why is fe_archive.c in src/common/ and not in src/fe_utils/?  It is 
>>> not
>>> "common" to frontend and backend.
>> 
>> Yep, this seems wrong to me.  I can propose following file rename.
>> 
>> src/common/fe_archive.c => src/fe_utils/archive.c
>> include/common/fe_archive.h => include/fe_utils/archive.h
> 
> Is it technically a problem though?  fe_archive.c is listed as a
> frontend-only file with OBJS_FRONTEND in src/common/Makefile.  Anyway,
> for this one I would not mind to do the move so please see the
> attached, but I am not completely sure either why src/fe_utils/ had
> better be chosen than src/common/.
> 

> 
> Perhaps we have more things that are frontend-only in src/common/ that
> could be moved to src/fe_utils/?  I am looking at restricted_token.c,
> fe_memutils.c, logging.c and file_utils.c here, but that would mean
> breaking a couple of declarations, something never free for plugin
> developers.
> 

I noticed this before in the thread [1], but it has been left unnoticed 
I guess:

"I just went through the both patches and realized that I cannot get 
into
semantics of splitting frontend code between common and fe_utils. This
applies only to 0002, where we introduce fe_archive.c. Should it be
placed into fe_utils alongside of the recent recovery_gen.c also used by
pg_rewind? This is a frontend-only code intended to be used by frontend
applications, so fe_utils feels like the right place, doesn't it? Just
tried to do so and everything went fine, so it seems that there is no
obstacles from the build system.

BTW, most of 'common' is a really common code with only four exceptions
like logging.c, which is frontend-only. Is it there for historical
reasons only or something else?"

Personally, I would prefer that everything in the 'common' was actually 
common. I also do not sure about moving an older code, because of 
possible backward compatibility breakage, but doing so for a newer one 
seems to be a good idea.

> 
>>> It actually defines functions that clash with functions in the 
>>> backend,
>>> so this seems doubly wrong.
>> 
>> Let's have frontend version of RestoreArchivedFile() renamed as well.
>> What about RestoreArchivedFileFrontend()?
> 
> -1 from me for the renaming, which was intentional to ease grepping
> with the backend counterpart.  We have other cases like that, say
> palloc(), fsync_fname(), etc.
> 

Do not like it either. Having the same naming and a guarantee that this 
code is always compiled separately looks more convenient for me.

[1] 
https://www.postgresql.org/message-id/784fa7dc-414b-9dc9-daae-138033db298c%40postgrespro.ru


Regards
-- 
Alexey Kondratov

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

On Mon, Jun 08, 2020 at 02:16:32PM +0300, Alexey Kondratov wrote:
> BTW, most of 'common' is a really common code with only four exceptions
> like logging.c, which is frontend-only. Is it there for historical
> reasons only or something else?"
>
> Personally, I would prefer that everything in the 'common' was actually
> common. I also do not sure about moving an older code, because of possible
> backward compatibility breakage, but doing so for a newer one seems to be a
> good idea.

src/fe_utils/ has been created much after src/common/ (588d963 vs
8396447).  I got to wonder if we should add a note to src/common/'s
Makefile to not add more stuff to OBJS_FRONTEND and just tell to use
src/fe_utils/.  Moving things from common/ to fe_utils/ would mean
breakages, which may be hard to justify just for the sake of being
clean and more consistent, and src/common/ APIs are usually quite
popular with external plugins.
--
Michael

On Mon, Jun 08, 2020 at 02:16:32PM +0300, Alexey Kondratov wrote:
> Do not like it either. Having the same naming and a guarantee that this code
> is always compiled separately looks more convenient for me.
>
> [1] https://www.postgresql.org/message-id/784fa7dc-414b-9dc9-daae-138033db298c%40postgrespro.ru

Okay.  After sleeping on it, it looks like would be better to move
this new fe_archive.c to src/fe_utils/.  I'll try to do that tomorrow,
and added an open item so as we don't forget about it.
--
Michael

On Wed, Jun 10, 2020 at 01:23:37PM +0900, Michael Paquier wrote:
> Okay.  After sleeping on it, it looks like would be better to move
> this new fe_archive.c to src/fe_utils/.  I'll try to do that tomorrow,
> and added an open item so as we don't forget about it.

Applied this part now as of a3b2bf1.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Wed, Jun 10, 2020 at 01:23:37PM +0900, Michael Paquier wrote:
> > Okay.  After sleeping on it, it looks like would be better to move
> > this new fe_archive.c to src/fe_utils/.  I'll try to do that tomorrow,
> > and added an open item so as we don't forget about it.
>
> Applied this part now as of a3b2bf1.

Uh, doesn't this pull these functions out of libpgcommon, where they
might have been being used by utilities outside of our client tools?

Thanks,

Stephen

On Thu, Jun 11, 2020 at 10:16:50AM -0400, Stephen Frost wrote:
> Uh, doesn't this pull these functions out of libpgcommon, where they
> might have been being used by utilities outside of our client tools?

This stuff is new to 13, and we are still in beta so it is fine in my
opinion to still change things how we think is best.  Suggesting to
change things once we are officially GA would not be possible.
--
Michael