Re: pg_rewind in contrib

Hi,

I'd like to include pg_rewind in contrib. I originally wrote it as an
external project so that I could quickly get it working with the
existing versions, and because I didn't feel it was quite ready for
production use yet. Now, with the WAL format changes in master, it is a
lot more maintainable than before. Many bugs have been fixed since the
first prototypes, and I think it's fairly robust now.

I propose that we include pg_rewind in contrib/ now. Attached is a patch
for that. It just includes the latest sources from the current pg_rewind
repository at https://github.com/vmware/pg_rewind. It is released under
the PostgreSQL license.

For those who are not familiar with pg_rewind, it's a tool that allows
repurposing an old master server as a new standby server, after
promotion, even if the old master was not shut down cleanly. That's a
very often requested feature.

- Heikki

Hi,

On 2014-12-12 16:13:13 +0200, Heikki Linnakangas wrote:
> I'd like to include pg_rewind in contrib. I originally wrote it as an
> external project so that I could quickly get it working with the existing
> versions, and because I didn't feel it was quite ready for production use
> yet. Now, with the WAL format changes in master, it is a lot more
> maintainable than before. Many bugs have been fixed since the first
> prototypes, and I think it's fairly robust now.

Obviously there's a need for a fair amount of review, but generally I
think it should be included.

Not sure if the copyright notices in the current form are actually ok?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Dec 12, 2014 at 03:20:47PM +0100, Andres Freund wrote:
> Hi,
>
> On 2014-12-12 16:13:13 +0200, Heikki Linnakangas wrote:
> > I'd like to include pg_rewind in contrib. I originally wrote it as an
> > external project so that I could quickly get it working with the existing
> > versions, and because I didn't feel it was quite ready for production use
> > yet. Now, with the WAL format changes in master, it is a lot more
> > maintainable than before. Many bugs have been fixed since the first
> > prototypes, and I think it's fairly robust now.
>
> Obviously there's a need for a fair amount of review, but generally I
> think it should be included.

I certainly think it is useful enough to be in /contrib.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Dec 12, 2014 at 11:13 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> I'd like to include pg_rewind in contrib. I originally wrote it as an
> external project so that I could quickly get it working with the existing
> versions, and because I didn't feel it was quite ready for production use
> yet. Now, with the WAL format changes in master, it is a lot more
> maintainable than before. Many bugs have been fixed since the first
> prototypes, and I think it's fairly robust now.
>
> I propose that we include pg_rewind in contrib/ now. Attached is a patch for
> that. It just includes the latest sources from the current pg_rewind
> repository at https://github.com/vmware/pg_rewind. It is released under the
> PostgreSQL license.
>
> For those who are not familiar with pg_rewind, it's a tool that allows
> repurposing an old master server as a new standby server, after promotion,
> even if the old master was not shut down cleanly. That's a very often
> requested feature.
Indeed the code got quite cleaner with the new WAL API. Btw, gitignore
has many unnecessary entries.
--
Michael

On 12/12/2014 04:20 PM, Andres Freund wrote:
> Not sure if the copyright notices in the current form are actually ok?

Hmm. We do have such copyright notices in the source tree, but I know
that we're trying to avoid it in new code. They had to be there when the
code lived as a separate project, but now that I'm contributing this to
PostgreSQL proper, I can remove them if necessary.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> I'd like to include pg_rewind in contrib.

I don't object to adding the tool as such, but let's wait to see what
happens with Peter's proposal to move contrib command-line tools into
src/bin/. If it should be there it'd be less code churn if it went
into there in the first place.

regards, tom lane

On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> > I'd like to include pg_rewind in contrib.
>
> I don't object to adding the tool as such, but let's wait to see
> what happens with Peter's proposal to move contrib command-line
> tools into src/bin/. If it should be there it'd be less code churn
> if it went into there in the first place.

+1 for putting it directly in src/bin.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sat, Dec 13, 2014 at 10:49 PM, David Fetter <david(at)fetter(dot)org> wrote:
>
> On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote:
> > Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> > > I'd like to include pg_rewind in contrib.
> >
> > I don't object to adding the tool as such, but let's wait to see
> > what happens with Peter's proposal to move contrib command-line
> > tools into src/bin/. If it should be there it'd be less code churn
> > if it went into there in the first place.
>
> +1 for putting it directly in src/bin.
>
>

Yeah, +1 for putting it under src/bin.

On Sat, Dec 13, 2014 at 12:01 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 12/12/2014 04:20 PM, Andres Freund wrote:
>>
>> Not sure if the copyright notices in the current form are actually ok?
>
>
> Hmm. We do have such copyright notices in the source tree, but I know that
> we're trying to avoid it in new code. They had to be there when the code
> lived as a separate project, but now that I'm contributing this to
> PostgreSQL proper, I can remove them if necessary.
Yep, it is fine to remove those copyright notices and to keep only the
references to PGDG when code is integrated in the tree.
--
Michael

On Tue, Dec 16, 2014 at 9:32 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Dec 13, 2014 at 12:01 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> On 12/12/2014 04:20 PM, Andres Freund wrote:
>>>
>>> Not sure if the copyright notices in the current form are actually ok?
>>
>>
>> Hmm. We do have such copyright notices in the source tree, but I know that
>> we're trying to avoid it in new code. They had to be there when the code
>> lived as a separate project, but now that I'm contributing this to
>> PostgreSQL proper, I can remove them if necessary.
> Yep, it is fine to remove those copyright notices and to keep only the
> references to PGDG when code is integrated in the tree.
In any case, I have a couple of comments about this patch as-is:
- If the move to src/bin is done, let's update the MSVC scripts accordingly
- contrib/pg_rewind/.gitignore should be cleaned up from its unnecessary entries
- README is incorrect, it is still mentioned for example that this
code should be copied inside PostgreSQL code tree as contrib/pg_rewind
- Code is going to need a brush to clean up things of this type:

--
Michael

On Tue, Dec 16, 2014 at 10:26 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> In any case, I have a couple of comments about this patch as-is:
> - If the move to src/bin is done, let's update the MSVC scripts accordingly
> - contrib/pg_rewind/.gitignore should be cleaned up from its unnecessary entries
> - README is incorrect, it is still mentioned for example that this
> code should be copied inside PostgreSQL code tree as contrib/pg_rewind
(Sorry email got sent...)
- Code is going to need a brush to clean up things of this type:
+ PG_9.4_201403261
+ printf("Report bugs to https://github.com/vmware/pg_rewind.\n");
- Versioning should be made the Postgres-way, aka not that:
+#define PG_REWIND_VERSION "0.1"
But a way similar to other utilities:
pg_rewind (PostgreSQL) 9.5devel
- Shouldn't we use $(SHELL) here at least?
+++ b/contrib/pg_rewind/launcher
@@ -0,0 +1,6 @@
+#!/bin/bash
+#
+# Normally, psql feeds the files in sql/ directory to psql, but we want to
+# run them as shell scripts instead.
+
+bash
I doubt that this would work for example with MinGW.
- There are a couple of TODO items which may be good to fix:
+ *
+ * TODO: This assumes that there are no timeline switches on the target
+ * cluster after the fork.
+ */
and:
+ /*
+ * TODO: move old file out of the way, if any. And perhaps create the
+ * file with temporary name first and rename in place after it's done.
+ */
- Regression tests, which have a good coverage btw are made using
shell scripts. There is some initialization process that could be
refactored IMO as this code is duplicated with pg_upgrade. For
example, listen_addresses initialization has checks fir MINGW,
environment variables PG* are unset, etc.
Regards,
--
Michael

Hi,

On 2014/12/12 23:13, Heikki Linnakangas wrote:
> Hi,
>
> I'd like to include pg_rewind in contrib. I originally wrote it as an
> external project so that I could quickly get it working with the
> existing versions, and because I didn't feel it was quite ready for
> production use yet. Now, with the WAL format changes in master, it is a
> lot more maintainable than before. Many bugs have been fixed since the
> first prototypes, and I think it's fairly robust now.
>
> I propose that we include pg_rewind in contrib/ now. Attached is a patch
> for that. It just includes the latest sources from the current pg_rewind
> repository at https://github.com/vmware/pg_rewind. It is released under
> the PostgreSQL license.
>
> For those who are not familiar with pg_rewind, it's a tool that allows
> repurposing an old master server as a new standby server, after
> promotion, even if the old master was not shut down cleanly. That's a
> very often requested feature.

I'm looking into pg_rewind with a very first scenario.
My scenario is here.

https://github.com/snaga/pg_rewind_test/blob/master/pg_rewind_test.sh

At least, I think a file descriptor "srcf" should be closed before
exiting copy_file_range(). I got "can't open file" error with
"too many open file" while running pg_rewind.

------------------------------------------------
diff --git a/contrib/pg_rewind/copy_fetch.c b/contrib/pg_rewind/copy_fetch.c
index bea1b09..5a8cc8e 100644
--- a/contrib/pg_rewind/copy_fetch.c
+++ b/contrib/pg_rewind/copy_fetch.c
@@ -280,6 +280,8 @@ copy_file_range(const char *path, off_t begin, off_t
end, bool trunc)
write_file_range(buf, begin, readlen);
begin += readlen;
}
+
+ close(srcfd);
}

/*
------------------------------------------------

And I have one question here.

pg_rewind assumes that the source PostgreSQL has, at least, one
checkpoint after getting promoted. I think the target timeline id
in the pg_control file to be read is only available after the first
checkpoint. Right?

Regards,

>
> - Heikki
>
>
>

--
NAGAYASU Satoshi <snaga(at)uptime(dot)jp>

On 12/16/2014 11:23 AM, Satoshi Nagayasu wrote:
> Hi,
>
> On 2014/12/12 23:13, Heikki Linnakangas wrote:
> > Hi,
> >
> > I'd like to include pg_rewind in contrib. I originally wrote it as an
> > external project so that I could quickly get it working with the
> > existing versions, and because I didn't feel it was quite ready for
> > production use yet. Now, with the WAL format changes in master, it is a
> > lot more maintainable than before. Many bugs have been fixed since the
> > first prototypes, and I think it's fairly robust now.
> >
> > I propose that we include pg_rewind in contrib/ now. Attached is a patch
> > for that. It just includes the latest sources from the current pg_rewind
> > repository at https://github.com/vmware/pg_rewind. It is released under
> > the PostgreSQL license.
> >
> > For those who are not familiar with pg_rewind, it's a tool that allows
> > repurposing an old master server as a new standby server, after
> > promotion, even if the old master was not shut down cleanly. That's a
> > very often requested feature.
>
> I'm looking into pg_rewind with a very first scenario.
> My scenario is here.
>
> https://github.com/snaga/pg_rewind_test/blob/master/pg_rewind_test.sh
>
> At least, I think a file descriptor "srcf" should be closed before
> exiting copy_file_range(). I got "can't open file" error with
> "too many open file" while running pg_rewind.
>
> ------------------------------------------------
> diff --git a/contrib/pg_rewind/copy_fetch.c b/contrib/pg_rewind/copy_fetch.c
> index bea1b09..5a8cc8e 100644
> --- a/contrib/pg_rewind/copy_fetch.c
> +++ b/contrib/pg_rewind/copy_fetch.c
> @@ -280,6 +280,8 @@ copy_file_range(const char *path, off_t begin, off_t
> end, bool trunc)
> write_file_range(buf, begin, readlen);
> begin += readlen;
> }
> +
> + close(srcfd);
> }
>
> /*
> ------------------------------------------------

Yep, good catch. I pushed a fix to the pg_rewind repository at github.

> And I have one question here.
>
> pg_rewind assumes that the source PostgreSQL has, at least, one
> checkpoint after getting promoted. I think the target timeline id
> in the pg_control file to be read is only available after the first
> checkpoint. Right?

Yes, it does assume that the source server (= old standby, new master)
has had at least one checkpoint after promotion. It probably should be
more explicit about it: If there hasn't been a checkpoint, you will
currently get an error "source and target cluster are both on the same
timeline", which isn't very informative.

I assume that by "target timeline ID" you meant the timeline ID of the
source server, i.e. the timeline that the target server should be
rewound to.

- Heikki

On 2014/12/16 18:37, Heikki Linnakangas wrote:
> On 12/16/2014 11:23 AM, Satoshi Nagayasu wrote:
>> pg_rewind assumes that the source PostgreSQL has, at least, one
>> checkpoint after getting promoted. I think the target timeline id
>> in the pg_control file to be read is only available after the first
>> checkpoint. Right?
>
> Yes, it does assume that the source server (= old standby, new master)
> has had at least one checkpoint after promotion. It probably should be
> more explicit about it: If there hasn't been a checkpoint, you will
> currently get an error "source and target cluster are both on the same
> timeline", which isn't very informative.

Yes, I got the message, so I could find the checkpoint thing.
It could be more informative, or some hint message could be added.

> I assume that by "target timeline ID" you meant the timeline ID of the
> source server, i.e. the timeline that the target server should be
> rewound to.

Yes.
Target timeline I mean here is the timeline id coming from the promoted
master (== source server == old standby).

I got it. Thanks.

I'm going to look into more details.

Regards,

>
> - Heikki
>

--
NAGAYASU Satoshi <snaga(at)uptime(dot)jp>

Here's an updated version of pg_rewind. The code itself is the same as
before, and corresponds to the sources in the current pg_rewind github
repository, as of commit a65e3754faf9ca9718e6b350abc736de586433b7. Based
mostly on Michael's comments, I have:

* replaced VMware copyright notices with PGDG ones.
* removed unnecessary cruft from .gitignore
* changed the --version line and "report bugs" notice in --help to match
other binaries in the PostgreSQL distribution
* moved documentation from README to the user manual.
* minor fixes to how the regression tests are launched so that they work
again

Some more work remains to be done on the regression tests. The way
they're launched now is quite weird. It was written that way to make it
work outside the PostgreSQL source tree, and also on Windows. Now that
it lives in contrib, it should be redesigned.

The documentation could also use some work; for now I just converted the
existing text from README to sgml format.

Anything else?

- Heikki

On Tue, Jan 6, 2015 at 2:38 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Here's an updated version of pg_rewind. The code itself is the same as
> before, and corresponds to the sources in the current pg_rewind github
> repository, as of commit a65e3754faf9ca9718e6b350abc736de586433b7. Based
> mostly on Michael's comments, I have:
>
> * replaced VMware copyright notices with PGDG ones.
> * removed unnecessary cruft from .gitignore
> * changed the --version line and "report bugs" notice in --help to match
> other binaries in the PostgreSQL distribution
> * moved documentation from README to the user manual.
> * minor fixes to how the regression tests are launched so that they work
> again
>
> Some more work remains to be done on the regression tests. The way they're
> launched now is quite weird. It was written that way to make it work outside
> the PostgreSQL source tree, and also on Windows. Now that it lives in
> contrib, it should be redesigned.
>
> The documentation could also use some work; for now I just converted the
> existing text from README to sgml format.

Some more comments:
- The binary pg_rewind needs to be ignored in contrib/pg_rewind/
- Be careful that ./launcher permission should be set to 755 when
doing a git add in it... Or regression tests will fail.
- It would be good to get make check working so as it includes both
check-local and check-remote
- installcheck should be a target ignored.
- Nitpicking: the header formats of filemap.c, datapagemap.c,
datapagemap.h and util.h are incorrect (I pushed a fix about that in
pg_rewind itself, feel free to pick it up).
- parsexlog.c has a copyright mention to Nippon Telegraph and
Telephone Corporation. Cannot we drop it safely?
- MSVC build is not supported yet. You need to do something similar to
pg_xlogdump, aka some magic with for example xlogreader.c.
- Error codes needs to be generated before building pg_rewind. If I do
for example a simple configure followed by make I get a failure:
$ ./configure
$ cd contrib/pg_rewind && make
In file included from parsexlog.c:16:
In file included from ../../src/include/postgres.h:48:
../../src/include/utils/elog.h:69:10: fatal error: 'utils/errcodes.h'
file not found
#include "utils/errcodes.h"
- Build fails with MinGW as there is visibly some unportable code:
copy_fetch.c: In function 'recurse_dir':
copy_fetch.c:112:3: warning: implicit declaration of function 'S_ISLNK' [-Wimpli
cit-function-declaration]
else if (S_ISLNK(fst.st_mode))
^
copy_fetch.c: In function 'check_samefile':
copy_fetch.c:298:2: warning: passing argument 2 of '_fstat64i32' from incompatib
le pointer type [enabled by default]
if (fstat(fd1, &statbuf1) < 0)
^
In file included from ../../src/include/port.h:283:0,
from ../../src/include/c.h:1050,
from ../../src/include/postgres_fe.h:25,
from copy_fetch.c:10:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but arg
ument is of type 'struct stat *'
__CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat)
{
^
copy_fetch.c:304:2: warning: passing argument 2 of '_fstat64i32' from incompatib
le pointer type [enabled by default]
if (fstat(fd2, &statbuf2) < 0)
^
In file included from ../../src/include/port.h:283:0,
from ../../src/include/c.h:1050,
from ../../src/include/postgres_fe.h:25,
from copy_fetch.c:10:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but arg
ument is of type 'struct stat *'
__CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat)
{
^
copy_fetch.c: In function 'truncate_target_file':
copy_fetch.c:436:2: warning: implicit declaration of function 'truncate' [-Wimpl
icit-function-declaration]
if (truncate(dstpath, newsize) != 0)
^
copy_fetch.c: In function 'slurpFile':
copy_fetch.c:561:2: warning: passing argument 2 of '_fstat64i32' from incompatib
le pointer type [enabled by default]
if (fstat(fd, &statbuf) < 0)
^
In file included from ../../src/include/port.h:283:0,
from ../../src/include/c.h:1050,
from ../../src/include/postgres_fe.h:25,
from copy_fetch.c:10:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but arg
ument is of type 'struct stat *'
__CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat)
{
^
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -We
ndif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -f
wrapv -fexcess-precision=standard -g -DG -fno-omit-frame-pointer -I../../src/int
erfaces/libpq -I. -I. -I../../src/include -DFRONTEND "-I../../src/include/port/
win32" -c -o libpq_fetch.o libpq_fetch.c -MMD -MP -MF .deps/libpq_fetch.Po
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -We
ndif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -f
wrapv -fexcess-precision=standard -g -DG -fno-omit-frame-pointer -I../../src/int
erfaces/libpq -I. -I. -I../../src/include -DFRONTEND "-I../../src/include/port/
win32" -c -o filemap.o filemap.c -MMD -MP -MF .deps/filemap.Po
filemap.c:12:19: fatal error: regex.h: No such file or directory
#include <regex.h>
^
compilation terminated.
../../src/Makefile.global:732: recipe for target 'filemap.o' failed
make: *** [filemap.o] Error 1
Hm. I think that this is something we should try to fix first
upstream. I feel as well that regression tests are going to need some
tuning as well to work properly.

Regards,
--
Michael

I applaud the ingenuity on all levels in this patch. But it seems to me
that there is way too much backend knowledge encoded and/or duplicated
in a front-end program.

If this ends up shipping, it's going to be a massively popular tool. I
see it as a companion to pg_basebackup. So it should sort of work the
same way. One problem is that it doesn't use the replication protocol,
so the setup is going to be inconsistent with pg_basebackup. Maybe the
replication protocol could be extended to provide the required data.
Maybe something as simple as "give me this file" would work.

That might lose the local copy mode, but how important is that?
pg_basebackup doesn't have that mode. In any case, the documentation
doesn't explain this distinction. The option documentation is a bit
short in any case, but it's not clear that you can choose between local
and remote mode.

The test suite should probably be reimplemented in Perl. (I might be
able to help.) Again, ingenious, but it's very hard to follow the
sequence of what is being tested. And some Windows person is going to
complain. ;-)

Also, since you have been maintaining this tool for a while, what is the
effort for maintaining it from version to version?

On Wed, Jan 7, 2015 at 5:39 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Also, since you have been maintaining this tool for a while, what is the
> effort for maintaining it from version to version?
From my own experience, the main source of maintenance across major
versions is the modification of the WAL record format to be able to
track the blocks that need to be copied from the newly promoted master
to the node rewound. That has been an ongoing effort on REL9_4_STABLE
and REL9_3_STABLE since this tool has been created when both versions
were in development to keep the tool compatible all the time. This
problem does not exist anymore on master thanks to the new WAL format
able to track easily the blocks modified, limiting the maintenance
necessary to actual bugs.
--
Michael

On 01/06/2015 03:39 PM, Peter Eisentraut wrote:
> I applaud the ingenuity on all levels in this patch. But it seems to me
> that there is way too much backend knowledge encoded and/or duplicated
> in a front-end program.
>
> If this ends up shipping, it's going to be a massively popular tool. I
> see it as a companion to pg_basebackup. So it should sort of work the
> same way. One problem is that it doesn't use the replication protocol,
> so the setup is going to be inconsistent with pg_basebackup. Maybe the
> replication protocol could be extended to provide the required data.
> Maybe something as simple as "give me this file" would work.
>
> That might lose the local copy mode, but how important is that?
> pg_basebackup doesn't have that mode. In any case, the documentation
> doesn't explain this distinction. The option documentation is a bit
> short in any case, but it's not clear that you can choose between local
> and remote mode.
>
> The test suite should probably be reimplemented in Perl. (I might be
> able to help.) Again, ingenious, but it's very hard to follow the
> sequence of what is being tested. And some Windows person is going to
> complain. ;-)
>
> Also, since you have been maintaining this tool for a while, what is the
> effort for maintaining it from version to version?
>
>

I also think it's a great idea. But I think we should consider the name
carefully. pg_resync might be a better name. Strictly, you might not be
quite rewinding, AIUI.

cheers

andrew

On 2015-01-06 15:39:29 -0500, Peter Eisentraut wrote:
> I applaud the ingenuity on all levels in this patch. But it seems to me
> that there is way too much backend knowledge encoded and/or duplicated
> in a front-end program.

Hm. There's really not that much in the current version anymore? Sure,
there's some xlog record specific knowledge, some about the whole data
directory layout and a bunch of timeline specific stuff. But that's not
that much.

Don't get me wrong - I personally think this shouldn't be in contrib but
in bin. The amount of prerequisite work to allow this to be
maintainable (2c03216d, 2076db2, ...) is a hint of how closely this is
linked and how much effort core community people have put into this. I
don't think contrib/ is the right place for that. Even though we haven't
found something we can agree on wrt moving other stuff (apprently at
least?) from contrib, we can still place new stuff in src/bin instead of
contrib.

It wouldn't hurt if we could share SimpleXLogPageRead() between
pg_xlogdump and pg_rewind as the differences are more or less
superficial, but that seems simple enough to achieve by putting a
frontend variant in xlogreader.c/h.

> If this ends up shipping, it's going to be a massively popular tool. I
> see it as a companion to pg_basebackup. So it should sort of work the
> same way. One problem is that it doesn't use the replication protocol,
> so the setup is going to be inconsistent with pg_basebackup. Maybe the
> replication protocol could be extended to provide the required data.

I'm not particularly bothered by the requirement of also requiring a
normal, not replication, connection. In most cases that'll already be
allowed.

> Maybe something as simple as "give me this file" would work.

Well, looking at libpq_fetch.c it seems there'd be a bit more required.

Not having to create a temporary table on the other side would be nice -
afaics there's otherwise not much stopping this from working against a
standby?

> That might lose the local copy mode, but how important is that?
> pg_basebackup doesn't have that mode.

But we have plain pg_start/stop_backup for that case. That alternative
doesn't really exist here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 01/07/2015 02:17 AM, Andres Freund wrote:
> On 2015-01-06 15:39:29 -0500, Peter Eisentraut wrote:
> It wouldn't hurt if we could share SimpleXLogPageRead() between
> pg_xlogdump and pg_rewind as the differences are more or less
> superficial, but that seems simple enough to achieve by putting a
> frontend variant in xlogreader.c/h.

It's not that much code. And although they're almost identical now, it's
not clear that pg_rewind and pg_xlogdump would want the same behaviour
in the future. pg_rewind might want to read files from a WAL archive,
for example. (Not that I have any plans for that right now)

>> If this ends up shipping, it's going to be a massively popular tool. I
>> see it as a companion to pg_basebackup. So it should sort of work the
>> same way. One problem is that it doesn't use the replication protocol,
>> so the setup is going to be inconsistent with pg_basebackup. Maybe the
>> replication protocol could be extended to provide the required data.
>
> I'm not particularly bothered by the requirement of also requiring a
> normal, not replication, connection. In most cases that'll already be
> allowed.

Yeah, it's not a big problem in practice, but it sure would be nice for
usability if it wasn't required. One less thing to document and prepare for.

>> Maybe something as simple as "give me this file" would work.
>
> Well, looking at libpq_fetch.c it seems there'd be a bit more required.
>
> Not having to create a temporary table on the other side would be nice -
> afaics there's otherwise not much stopping this from working against a
> standby?

Hmm, that could be done. The temporary table is convenient, but it could
be refactored to work without it.

>> That might lose the local copy mode, but how important is that?
>> pg_basebackup doesn't have that mode.
>
> But we have plain pg_start/stop_backup for that case. That alternative
> doesn't really exist here.

Also, the local mode works when the server is shut down. The
pg_basebackup analogue of that is just "cp -a $PGDATA backup".
- Heikki

On 01/07/2015 01:54 AM, Andrew Dunstan wrote:
> I also think it's a great idea. But I think we should consider the name
> carefully. pg_resync might be a better name. Strictly, you might not be
> quite rewinding, AIUI.

pg_resync sounds too generic. It's true that if the source server has
changes of its own, then it's more of a sideways movement than
rewinding, but I think it's nevertheless a good name.

It does always rewind the control file, so that after startup, WAL
replay begins from the last common point in history between the servers.
WAL replay will catch up with the source server, which might be ahead of
last common point, but strictly speaking pg_rewind is not involved at
that point anymore.

- Heikki

On 01/06/2015 10:39 PM, Peter Eisentraut wrote:
> If this ends up shipping, it's going to be a massively popular tool. I
> see it as a companion to pg_basebackup. So it should sort of work the
> same way. One problem is that it doesn't use the replication protocol,
> so the setup is going to be inconsistent with pg_basebackup. Maybe the
> replication protocol could be extended to provide the required data.
> Maybe something as simple as "give me this file" would work.

Yeah, that would be nice. But I think we can live with it as it is for
now, and add that later.

> That might lose the local copy mode, but how important is that?
> pg_basebackup doesn't have that mode. In any case, the documentation
> doesn't explain this distinction. The option documentation is a bit
> short in any case, but it's not clear that you can choose between local
> and remote mode.

Changing the libpq mode to use additional replication protocol commands
would be a localized change to libpq_fetch.c. No need to touch the local
copy mode.

> The test suite should probably be reimplemented in Perl. (I might be
> able to help.) Again, ingenious, but it's very hard to follow the
> sequence of what is being tested. And some Windows person is going to
> complain. ;-)

Yeah, totally agreed.

- Heikki

On 01/07/2015 03:22 AM, Heikki Linnakangas wrote:
> On 01/07/2015 01:54 AM, Andrew Dunstan wrote:
>> I also think it's a great idea. But I think we should consider the name
>> carefully. pg_resync might be a better name. Strictly, you might not be
>> quite rewinding, AIUI.
>
> pg_resync sounds too generic. It's true that if the source server has
> changes of its own, then it's more of a sideways movement than
> rewinding, but I think it's nevertheless a good name.
>
> It does always rewind the control file, so that after startup, WAL
> replay begins from the last common point in history between the
> servers. WAL replay will catch up with the source server, which might
> be ahead of last common point, but strictly speaking pg_rewind is not
> involved at that point anymore.
>
>

I understand, but I think "pg_rewind" is likely to be misleading to many
users who will say "but I don't want just to rewind".

I'm not wedded to the name I suggested, but I think we should look at
possible alternative names. We do have experience of misleading names
causing confusion (e.g. "initdb").

cheers

andrew

On 01/06/2015 10:39 PM, Peter Eisentraut wrote:
> The test suite should probably be reimplemented in Perl. (I might be
> able to help.) Again, ingenious, but it's very hard to follow the
> sequence of what is being tested. And some Windows person is going to
> complain. ;-)

I took a shot at rewriting the tests in perl. It works, but I would
appreciate help in reviewing and fixing any bad perl I may have written.
I have not added new tests or changed the things they test, this is just
a straightforward translation from the mix of bash scripts and
pg_regress to perl.

- Heikki

What is this define good for? I couldn't understand where it fits; is
it just a leftover?

+#define pageinfo_set_truncation(forkno, rnode, blkno) datapagemap_set_truncation(pagemap, forkno, rnode, blkno)

Please don't name files with generic names such as util.c/h. It's
annoying later; for instance when I want to open analyze.c I have to
choose the one in parser or commands. (pg_upgrade in particular is
already a mess; let's not copy that.)

Is the final destiny of this still contrib? There are several votes for
putting it in src/bin/ right from the start, which sounds reasonable to
me as well. We didn't put pg_basebackup in contrib initially for instance.
If we do that, we will need more attention to translatability markers of
user-visible error messages; if you want to continue using fprintf()
then you will need _() around the literals, but it might be wise to use
another function instead so that you can avoid them (say, have something
like pg_upgrade's pg_fatal() which you can then list in nls.mk).
...
Uh, I notice that you have _() in some places such as slurpFile().

I agree with Andres that it would be better to avoid a second copy of
the xlogreader helper stuff. A #FRONTEND define in xlogreader.c seems
acceptable, and pg_rewind can have a wrapper function to add extra
functionality later, if necessary -- or we can add a flags argument,
currently unused, which could later be extended to indicate to read from
archive, or something.

Copyright lines need updated to 2015 (meh)

Our policy on header inclusion says that postgres_fe.h comes first, then
system includes, then other postgres headers. So at least this one is
wrong:

+++ b/contrib/pg_rewind/copy_fetch.c
@@ -0,0 +1,586 @@
[...]
+#include "postgres_fe.h"
+
+#include "catalog/catalog.h"
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "pg_rewind.h"
+#include "fetch.h"
+#include "filemap.h"
+#include "datapagemap.h"
+#include "util.h"

The reason for this IIRC is that system includes could modify behavior
of some calls in obscure platforms under obscure circumstances.

+ while ((xlde = readdir(xldir)) != NULL)
+ {

You need to reset errno here, see commit 6f03927fce038096f53ca67eeab9a.

+static int dstfd = -1;
+static char dstpath[MAXPGPATH] = "";
+
+void
+open_target_file(const char *path, bool trunc)
+{

I think these file-level static vars ought to be declared at top of
file, probably with more distinctive names.

+void
+close_target_file(void)
+{
+ if (close(dstfd) != 0)
+ {
+ fprintf(stderr, "error closing destination file \"%s\": %s\n",
+ dstpath, strerror(errno));
+ exit(1);
+ }
+
+ dstfd = -1;
+ /* fsync? */
+}

Did you find an answer for the above question?

+static bool
+endswith(const char *haystack, const char *needle)
+{
+ int needlelen = strlen(needle);
+ int haystacklen = strlen(haystack);
+
+ if (haystacklen < needlelen)
+ return false;
+
+ return strcmp(&haystack[haystacklen - needlelen], needle) == 0;
+}

We already have this function elsewhere.

+ if (type != FILE_TYPE_REGULAR && isRelDataFile(path))
+ {
+ fprintf(stderr, "data file in source \"%s\" is a directory.\n", path);
+ exit(1);
+ }

Here you have error messages ending with periods; but not in most other
places.

+/*
+ * Does it look like a relation data file?
+ */
+static bool
+isRelDataFile(const char *path)

This doesn't consider forks ... why not? If correct, probably it deserves a comment.

+struct filemap_t
+{
+ /*
+ * New entries are accumulated to a linked list, in process_remote_file
+ * and process_local_file.
+ */
+ file_entry_t *first;
+ file_entry_t *last;
+ int nlist;

Uh, this is like the seventh open-coded list implementation in frontend
code. Can't we have this using ilist for a change?

Existing practice is that SQL keywords are uppercase:

+ sql =
+ "-- Create a recursive directory listing of the whole data directory\n"
+ "with recursive files (path, filename, size, isdir) as (\n"
+ " select '' as path, filename, size, isdir from\n"
+ " (select pg_ls_dir('.') as filename) as fn,\n"
+ " pg_stat_file(fn.filename) as this\n"
+ " union all\n"
+ " select parent.path || parent.filename || '/' as path,\n"
+ " fn, this.size, this.isdir\n"
+ " from files as parent,\n"
+ " pg_ls_dir(parent.path || parent.filename) as fn,\n"
+ " pg_stat_file(parent.path || parent.filename || '/' || fn) as this\n"
+ " where parent.isdir = 't'\n"
+ ")\n"
+ "-- Using the cte, fetch a listing of the all the files.\n"
+ "--\n"
+ "-- For tablespaces, use pg_tablespace_location() function to fetch\n"
+ "-- the link target (there is no backend function to get a symbolic\n"
+ "-- link's target in general, so if the admin has put any custom\n"
+ "-- symbolic links in the data directory, they won't be copied\n"
+ "-- correctly)\n"
+ "select path || filename, size, isdir,\n"
+ " pg_tablespace_location(pg_tablespace.oid) as link_target\n"
+ "from files\n"
+ "left outer join pg_tablespace on files.path = 'pg_tblspc/'\n"
+ " and oid::text = files.filename\n";

This FRONTEND game is pretty nasty -- why do you need it? Can we fix
the headers to avoid needing it?

+/*-------------------------------------------------------------------------
+ *
+ * parsexlog.c
+ * Functions for reading Write-Ahead-Log
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1996-2008, Nippon Telegraph and Telephone Corporation
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#define FRONTEND 1
+#include "c.h"
+#undef FRONTEND
+#include "postgres.h"
+
+#include "pg_rewind.h"
+#include "filemap.h"
+
+#include <unistd.h>
+
+#include "access/rmgr.h"
+#include "access/xlog_internal.h"
+#include "access/xlogreader.h"
+#include "catalog/pg_control.h"
+#include "catalog/storage_xlog.h"
+#include "commands/dbcommands.h"

(I think the answer is in dbcommands.h; we could split it to a new
dbcommands_xlog.h)

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

On 01/06/2015 09:13 AM, Michael Paquier wrote:
> Some more comments:
> - Nitpicking: the header formats of filemap.c, datapagemap.c,
> datapagemap.h and util.h are incorrect (I pushed a fix about that in
> pg_rewind itself, feel free to pick it up).

Ah, fixed.

> - parsexlog.c has a copyright mention to Nippon Telegraph and
> Telephone Corporation. Cannot we drop it safely?

Removed. The file used to contain code for handling different WAL record
types that was originally copied from pg_lesslog, hence the NTT
copyright. However, that code is no longer there.

> - Error codes needs to be generated before building pg_rewind. If I do
> for example a simple configure followed by make I get a failure:
> $ ./configure
> $ cd contrib/pg_rewind && make
> In file included from parsexlog.c:16:
> In file included from ../../src/include/postgres.h:48:
> ../../src/include/utils/elog.h:69:10: fatal error: 'utils/errcodes.h'
> file not found
> #include "utils/errcodes.h"

Many other contrib modules have the same problem. And other
subdirectories too. It's not something that this patch needs to fix.

> - MSVC build is not supported yet. You need to do something similar to
> pg_xlogdump, aka some magic with for example xlogreader.c.
> - Build fails with MinGW as there is visibly some unportable code:

Fixed all the errors I got on MSVC. The biggest change was rewriting the
code that determines if a file is a relation file, based on its
filename. It used a regular expression, which I replaced with a bunch of
sscanf calls, and a cross-check that GetRelationPath() returns the same
filename.

> copy_fetch.c: In function 'check_samefile':
> copy_fetch.c:298:2: warning: passing argument 2 of '_fstat64i32' from incompatib
> le pointer type [enabled by default]
> if (fstat(fd1, &statbuf1) < 0)
> ^
> In file included from ../../src/include/port.h:283:0,
> from ../../src/include/c.h:1050,
> from ../../src/include/postgres_fe.h:25,
> from copy_fetch.c:10:
> c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but arg
> ument is of type 'struct stat *'
> __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat)
> {

Strange. There isn't anything special about the fstat() calls in
pg_rewind. Do you get these from other modules that call fstat, e.g.
pg_stat_statements?

I did not see these warnings when building with MSVC, and don't have
MinGW installed currently.

> Hm. I think that this is something we should try to fix first
> upstream.

Yeah, possibly. But here's a new patch anyway.

- Heikki

On 1/6/15 7:17 PM, Andres Freund wrote:
>> One problem is that it doesn't use the replication protocol,
>> > so the setup is going to be inconsistent with pg_basebackup. Maybe the
>> > replication protocol could be extended to provide the required data.
> I'm not particularly bothered by the requirement of also requiring a
> normal, not replication, connection. In most cases that'll already be
> allowed.

I don't agree. We have separated out replication access, especially in
pg_hba.conf and with the replication role attribute, for a reason. (I
hope there was a reason; I don't remember.) It is not unreasonable to
set things up so that non-replication access is only from the
application tier, and replication access is only from within the
database tier.

Now we're saying, well, we didn't really mean that, in order to use the
latest replication management tools, you also need to open up
non-replication access, but we assume you already do that anyway.

Now I understand that making pg_rewind work over a replication
connection is a lot more work, and maybe we don't want to spend it, at
least right now. But then we either need to document this as an
explicit deficiency and think about fixing it later, or we should
revisit how replication access control is handled.

On Fri, Jan 9, 2015 at 1:02 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Fixed all the errors I got on MSVC. The biggest change was rewriting the
> code that determines if a file is a relation file, based on its filename. It
> used a regular expression, which I replaced with a bunch of sscanf calls,
> and a cross-check that GetRelationPath() returns the same filename.
It looks definitely better like that. Thanks.

>> copy_fetch.c: In function 'check_samefile':
>> copy_fetch.c:298:2: warning: passing argument 2 of '_fstat64i32' from
>> incompatib
>> le pointer type [enabled by default]
>> if (fstat(fd1, &statbuf1) < 0)
>> ^
>> In file included from ../../src/include/port.h:283:0,
>> from ../../src/include/c.h:1050,
>> from ../../src/include/postgres_fe.h:25,
>> from copy_fetch.c:10:
>> c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *'
>> but arg
>> ument is of type 'struct stat *'
>> __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32
>> *_stat)
>> {
>
>
> Strange. There isn't anything special about the fstat() calls in pg_rewind.
> Do you get these from other modules that call fstat, e.g.
> pg_stat_statements?
>
> I did not see these warnings when building with MSVC, and don't have MinGW
> installed currently.
Don't worry about those ones, it is discussed here already:
http://www.postgresql.org/message-id/CAB7nPqTrmmZo2y92DfZEd-mWo1cenEoaUhCZppv=OB84-4C9vA@mail.gmail.com

MSVC build still has a warning:
"C:\Users\mpaquier\git\postgres\pg_rewind.vcxproj" (default target) (60) ->
(Link target) ->
xlogreader.obj : warning LNK4049: locally defined symbol
pg_crc32c_table imported
[C:\Users\ioltas\git\postgres\pg_rewind.vcxproj]

The documentation needs some more polishing:
1) s/pg_reind/pg_rewind
2) by settings "wal_log_hints = on" => by setting
<varname>wal_log_hints</> to <literal>on</>
3) You should avoid using an hardcoded list of items in a block <para>
to list how pg_rewind works. Each item in the list should be changed
to use <orderedlist>:
<para>
The basic idea is to copy everything from the blah...

<orderedlist>
<listitem>
<para>
Scan the WAL log of blah..
</para>
</listitem>
<listitem>
<para>
paragraph2
</para>
<listitem>
[blah.]
</orderedlist>
</para>
4) --source-server and --target-pgdata are not listed in the list of options.
5) The synopsis should be written like that IMO: pg_rewind [option...]
6) pg_rewind is listed several times, doesn't it need <application>?
7) pg_xlog should use <filename>
8) Perhaps a section "See also" at the bottom could be added to
mention pg_basebackup?

Regards,
--
Michael

On 01/08/2015 10:44 PM, Peter Eisentraut wrote:
> On 1/6/15 7:17 PM, Andres Freund wrote:
>>> One problem is that it doesn't use the replication protocol,
>>>> so the setup is going to be inconsistent with pg_basebackup. Maybe the
>>>> replication protocol could be extended to provide the required data.
>> I'm not particularly bothered by the requirement of also requiring a
>> normal, not replication, connection. In most cases that'll already be
>> allowed.
>
> I don't agree. We have separated out replication access, especially in
> pg_hba.conf and with the replication role attribute, for a reason. (I
> hope there was a reason; I don't remember.) It is not unreasonable to
> set things up so that non-replication access is only from the
> application tier, and replication access is only from within the
> database tier.

I agree. A big difference between a replication connection and a normal
database connection is that a replication connection is not bound to any
particular database. It also cannot run transactions, and many other things.

It would nevertheless be handy to be able to do more stuff in a
replication connection. For example, you might want to run functions
like pg_create_restore_point(), pg_xlog_replay_pause/resume etc. in a
replication connection. We should extend the replication protocol to
allow such things. I'm not sure what it would look like; we can't run
arbitrary queries when not being connected to a database, or arbitrary
functions, but there are a lot of functions that would make sense.

> Now I understand that making pg_rewind work over a replication
> connection is a lot more work, and maybe we don't want to spend it, at
> least right now. But then we either need to document this as an
> explicit deficiency and think about fixing it later, or we should
> revisit how replication access control is handled.

Right, that's how I've been thinking about this. I don't want to start
working on the replication protocol right now, I think we can live with
it as it is, but it sure would be nicer if it worked over a replication
connection.

- Heikki

There are two ways in which access control for replication connections
is separate:

- replication pseudo-database in pg_hba.conf

- replication role attribute

If someone has a restrictive setup for replication and pg_basebackup,
and then pg_rewind enters, they will have to

- add a line to pg_hba.conf to allow non-replication access

- connect as superuser

The first is an inconvenience, although it might be useful for this and
other reasons to have a variant of "all" includes "replication".

The second, however, is a problem, because you have to change from a
restricted, quasi-ready-only user to full superuser access.

One way to address that might be if we added backend functions that are
variants of pg_ls_dir() and pg_stat_file() that are accessible only with
the replication attribute. Or we allow calling pg_ls_dir() and
pg_stat_file() with relative paths with the replication attribute.
(While we're at it, add a recursive variant of pg_ls_dir().) Or some
variant of that.

On 01/07/2015 06:19 PM, Alvaro Herrera wrote:

Fixed most of the issues you listed. More on a few remaining ones below.

> Please don't name files with generic names such as util.c/h. It's
> annoying later; for instance when I want to open analyze.c I have to
> choose the one in parser or commands. (pg_upgrade in particular is
> already a mess; let's not copy that.)

There was only one function in util.c, with only one caller, so I just
moved it to the calling file and removed util.c and util.h altogether.

There are few other files with fairly generic names: fetch.c, timeline.c
and (a new file I just added) logging.c. I agree it's annoying when
there are multiple files with same name, although I don't think it's
reasonable to totally avoid it either. We could call e.g. logging.c
pg_rewind_logging.c instead, but I'm not convinced that that is better.

> Is the final destiny of this still contrib? There are several votes for
> putting it in src/bin/ right from the start, which sounds reasonable to
> me as well.

Seems that the majority opinion is to put this straight into src/bin.
Works for me, moved.

> If we do that, we will need more attention to translatability markers of
> user-visible error messages; if you want to continue using fprintf()
> then you will need _() around the literals, but it might be wise to use
> another function instead so that you can avoid them (say, have something
> like pg_upgrade's pg_fatal() which you can then list in nls.mk).
> ...
> Uh, I notice that you have _() in some places such as slurpFile().

I copy-pasted pg_fatal and pg_log from pg_upgrade, replaced all the
fprintf(stderr) calls with them, and created an nls.mk file. It's now
ready for translation.

I also removed the --verbose option and replaced it with --progress,
which shows a progress indicator similar to pg_basebackup's, and
--debug, which spews out the list of actions on each file and other
debugging output.

> +void
> +close_target_file(void)
> +{
> + if (close(dstfd) != 0)
> + {
> + fprintf(stderr, "error closing destination file \"%s\": %s\n",
> + dstpath, strerror(errno));
> + exit(1);
> + }
> +
> + dstfd = -1;
> + /* fsync? */
> +}
>
> Did you find an answer for the above question?

No. The question is, should pg_rewind fsync() every file that it
modifies? It would be a reasonable thing to do, to make sure that if you
crash immediately after running pg_rewind, the cluster is in a
consistent state. However, pg_basebackup for example doesn't do that.
initdb does, but that was added fairly recently.

I'm not thrilled about sprinkling fsync() calls everywhere that we touch
files. But I guess that would be the right thing to do. I'm planning to
do that as an add-on patch later, fixing also pg_basebackup and any
other utilities that need it.

> +/*
> + * Does it look like a relation data file?
> + */
> +static bool
> +isRelDataFile(const char *path)
>
> This doesn't consider forks ... why not? If correct, probably it deserves a comment.

Added a comment. (Non-main forks are always copied in toto, so they are
not considered as relation data files for pg_rewind's purposes.)

> +struct filemap_t
> +{
> + /*
> + * New entries are accumulated to a linked list, in process_remote_file
> + * and process_local_file.
> + */
> + file_entry_t *first;
> + file_entry_t *last;
> + int nlist;
>
> Uh, this is like the seventh open-coded list implementation in frontend
> code. Can't we have this using ilist for a change?

ilist is backend code. I'm not eager to move it to src/common. A linked
list is a trivial data structure, I don't think it's too bad to
re-invent that wheel.

In this case, it might make sense to get rid of the linked list
altogether. pg_rewind currently accumulates new entries in a list, and
turns that into a sorted array after all remote files have been
accumulated. But it could be rewritten to use an array to begin with.

> This FRONTEND game is pretty nasty -- why do you need it? Can we fix
> the headers to avoid needing it?
>
> +/*-------------------------------------------------------------------------
> + *
> + * parsexlog.c
> + * Functions for reading Write-Ahead-Log
> + *
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1996-2008, Nippon Telegraph and Telephone Corporation
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *-------------------------------------------------------------------------
> + */
> +
> +#define FRONTEND 1
> +#include "c.h"
> +#undef FRONTEND
> +#include "postgres.h"
> +
> +#include "pg_rewind.h"
> +#include "filemap.h"
> +
> +#include <unistd.h>
> +
> +#include "access/rmgr.h"
> +#include "access/xlog_internal.h"
> +#include "access/xlogreader.h"
> +#include "catalog/pg_control.h"
> +#include "catalog/storage_xlog.h"
> +#include "commands/dbcommands.h"
>
> (I think the answer is in dbcommands.h; we could split it to a new
> dbcommands_xlog.h)

Ah, nice, that was the only thing left that needed the FRONTEND hack. Done.

Here is a new version. There are now a fair amount of code changes
compared to the version on github, like the logging and progress
information, and a lot of comment changes. I also improved the
documentation.

This patch is also available at my git repository at
git://git.postgresql.org/git/users/heikki/postgres.git, branch
"pg_rewind". The commit history of that isn't very clean, so don't pay
too much attention to that.

- Heikki

On 2015-01-14 11:43:00 +0200, Heikki Linnakangas wrote:
> No. The question is, should pg_rewind fsync() every file that it
> modifies?

Not immediately, but before the end, yes.

> It would be a reasonable thing to do, to make sure that if you crash
> immediately after running pg_rewind, the cluster is in a consistent state.
> However, pg_basebackup for example doesn't do that. initdb does, but that
> was added fairly recently.

initdb -S can be used for this if you want to - that's why Bruce added
it. It only works correctly for tablespaces since a couple weeks
however.

> I'm not thrilled about sprinkling fsync() calls everywhere that we touch
> files. But I guess that would be the right thing to do. I'm planning to do
> that as an add-on patch later, fixing also pg_basebackup and any other
> utilities that need it.

Yea, we really need to do this. We also need it in the server, right now
there's a bunch of rather ugly corner cases where we rely on not fsynced
files being present after e.g. two consecutive crashes. Abhijit has sent
a patch.

> >+struct filemap_t
> >+{
> >+ /*
> >+ * New entries are accumulated to a linked list, in process_remote_file
> >+ * and process_local_file.
> >+ */
> >+ file_entry_t *first;
> >+ file_entry_t *last;
> >+ int nlist;
> >
> >Uh, this is like the seventh open-coded list implementation in frontend
> >code. Can't we have this using ilist for a change?
>
> ilist is backend code. I'm not eager to move it to src/common. A linked list
> is a trivial data structure, I don't think it's too bad to re-invent that
> wheel.

Not a fan. The amount of bugs in open coded list manipulations tends to
be high.

Greetings,

Andres Freund

On 2015-01-09 10:48:39 +0200, Heikki Linnakangas wrote:
> It would nevertheless be handy to be able to do more stuff in a replication
> connection. For example, you might want to run functions like
> pg_create_restore_point(), pg_xlog_replay_pause/resume etc. in a replication
> connection. We should extend the replication protocol to allow such things.
> I'm not sure what it would look like; we can't run arbitrary queries when
> not being connected to a database, or arbitrary functions, but there are a
> lot of functions that would make sense.

Well, it's possible now to have replication connections connect to a
database if you choose now (replication=database dbname=...). We could
just add e.g. the fastpath function interface and add allow a couple of
builtin functions to be called. That'd probably be fairly simple.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2015-01-14 10:53:40 +0100, Andres Freund wrote:
> On 2015-01-14 11:43:00 +0200, Heikki Linnakangas wrote:
> > I'm not thrilled about sprinkling fsync() calls everywhere that we touch
> > files. But I guess that would be the right thing to do. I'm planning to do
> > that as an add-on patch later, fixing also pg_basebackup and any other
> > utilities that need it.
>
> Yea, we really need to do this. We also need it in the server, right now
> there's a bunch of rather ugly corner cases where we rely on not fsynced
> files being present after e.g. two consecutive crashes. Abhijit has sent
> a patch.

explanation
http://archives.postgresql.org/message-id/20140918083148.GA17265%40alap3.anarazel.de
Abhijit's patch
http://www.postgresql.org/message-id/20141106122653.GA18963@toroid.org

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jan 7, 2015 at 8:44 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> I understand, but I think "pg_rewind" is likely to be misleading to many
> users who will say "but I don't want just to rewind".
>
> I'm not wedded to the name I suggested, but I think we should look at
> possible alternative names. We do have experience of misleading names
> causing confusion (e.g. "initdb").

FWIW, pg_rewind sounds pretty good to me. But maybe I'm just not
understanding what the use case for this, apart from rewinding?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 14, 2015 at 6:43 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Here is a new version. There are now a fair amount of code changes compared
> to the version on github, like the logging and progress information, and a
> lot of comment changes. I also improved the documentation.

Perhaps this could a direct link to the page of pg_rewind with <xref
linkend="app-pgrewind">?
- possibly new, system. Once complete, the primary and standby can be
+ possibly new, system. The <application>pg_rewind</> utility can be
+ used to speed up this process on large clusters.
+ Once complete, the primary and standby can be

Missing a dot a the end of the phrase of this paragraph:
+ <para>
+ This option specifies the target data directory that is synchronized
+ with the source. The target server must shut down cleanly before
+ running <application>pg_rewind</application>
+ </para>

Compilation of pg_rewind on MSVC is not done. It is still listed as a
contrib/ in Mkvcbuild.pm.

The compilation of pg_xlogdump fails because inclusion of
dbcommands_xlog.h is missing in rmgrdesc.c (patch attached).

src/bin/Makefile has not been updated to compile pg_rewind.
--
Michael

I must have missed this, how did you some the hint bit problem with
pg_rewind? Last I understood you ran the risk that the server has unlogged
hint bit updates that you wouldn't know to rewind.

On 2015-01-15 13:21:56 +0000, Greg Stark wrote:
> I must have missed this, how did you some the hint bit problem with
> pg_rewind? Last I understood you ran the risk that the server has unlogged
> hint bit updates that you wouldn't know to rewind.

wal_log_hints = on

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 01/15/2015 03:21 PM, Greg Stark wrote:
> I must have missed this, how did you some the hint bit problem with
> pg_rewind? Last I understood you ran the risk that the server has unlogged
> hint bit updates that you wouldn't know to rewind.

There's a new GUC in 9.4, wal_log_hints, for that. It has to be turned
on for pg_rewind to work. Or data checksums must be enabled, which also
causes hint bit updates to be logged.

- Heikki

Here is a random bag of comments for the v5 patch:

pg_xlogdump fails to build:

CC xlogreader.o
CC rmgrdesc.o
../../src/include/access/rmgrlist.h:32:46: error: 'dbase_desc' undeclared here (not in a function)
PG_RMGR(RM_DBASE_ID, "Database", dbase_redo, dbase_desc, dbase_identify, NULL, NULL)
^
rmgrdesc.c:33:10: note: in definition of macro 'PG_RMGR'
{ name, desc, identify},
^
../../src/include/access/rmgrlist.h:32:58: error: 'dbase_identify' undeclared here (not in a function)
PG_RMGR(RM_DBASE_ID, "Database", dbase_redo, dbase_desc, dbase_identify, NULL, NULL)
^
rmgrdesc.c:33:16: note: in definition of macro 'PG_RMGR'
{ name, desc, identify},
^
../../src/Makefile.global:732: recipe for target 'rmgrdesc.o' failed
make[2]: *** [rmgrdesc.o] Error 1

SGML files should use 1-space indentation, not 2.

In high-availability.sgml, pg_rewind could be a link.

In ref/pg_rewind.sgml,

-P option listed twice.

The option --source-server had be confused at first, because the entry
above under --source-pgdata also talks about a "source server". Maybe
--source-connection would be clearer?

Reference pages have standardized top-level headers, so "Theory of
operation" should be under something like "Notes".

Similarly for "Restrictions", but that seems important enough to go
into the description.

src/bin/pg_rewind/.gitignore lists files for pg_regress use, which is not used anymore.

src/bin/pg_rewind/Makefile says "Makefile for src/bin/pg_basebackup".

There should be an installcheck target.

RewindTest.pm should be in the t/ directory.

Code like this:

+ if (map->bitmap == NULL)
+ map->bitmap = pg_malloc(newsize);
+ else
+ map->bitmap = pg_realloc(map->bitmap, newsize);

is unnecessary. You can just write

map->bitmap = pg_realloc(map->bitmap, newsize);

because realloc handles NULL.

Instead of FILE_TYPE_DIRECTORY etc., why not use S_IFDIR etc.?

About this code

+ if (!exists)
+ action = FILE_ACTION_CREATE;
+ else
+ action = FILE_ACTION_NONE;

action is earlier initialized as FILE_ACTION_NONE, so the second
branch is redundant. Alternatively, remove the earlier
initialization, so that maybe the compiler can detect if action is not
assigned in some paths.

Error messages should not end with a period.

Some calls to pg_fatal() don't have the string end with a newline.

In libpqProcessFileList(), I would tend to put the comment outside of
the SQL command string.

Mkvcbuild.pm changes still refer to pg_rewind in contrib.

TestLib.pm addition command_is sounds a bit wrong. It's evidently
modelled after command_like, but that now sounds wrong too. How about
command_stdout_is?

The test suite needs to silence all non-TAP output. So psql needs to
be run with -q pg_ctl with -s etc. Any important output needs to be
through diag() or note().

Test cases like

ok 6 - psql -A -t --no-psqlrc -c port=10072 -c SELECT datname FROM pg_database exit code 0

should probably get a real name.

The whole structure of the test suite still looks too much like the
old hack. I'll try to think of other ways to structure it.

On 01/16/2015 03:30 AM, Peter Eisentraut wrote:
> Here is a random bag of comments for the v5 patch:

Addressed most of your comments, and Michael's. Another version
attached. More on a few of the comments below.

> The option --source-server had be confused at first, because the entry
> above under --source-pgdata also talks about a "source server". Maybe
> --source-connection would be clearer?

Hmm. I would rather emphasize that the source is a running server, than
the connection to the server. I can see the confusion, though. What
about phrasing it as:

--source-pgdata

Specifies path to the data directory of the source server, to
synchronize the target with. When <option>--source-pgdata</> is used,
the source server must be cleanly shut down.

The point is that whether you use --source-pgdata or --source-server,
the source is a PostgreSQL server. Perhaps it should be mentioned here
that you only need one of --source-pgdata and --source-server, even
though the synopsis says that too.

Another idea is to rename --source-server to just --source. That would
make sense if we assume that it's more common to connect to a live server:

pg_rewind --target mypgdata --source "host=otherhost user=dba"

pg_rewind --target mypgdata --source-pgdata /other/pgdata

> Reference pages have standardized top-level headers, so "Theory of
> operation" should be under something like "Notes".
>
> Similarly for "Restrictions", but that seems important enough to go
> into the description.

Oh. That's rather limiting. I'll rename the "Restrictions" to "Notes" -
that seems to be where we have listed limitations like that in many
other pages. I also moved Theory of operation as a "How it works"
subsection under "Notes".

The top-level headers aren't totally standardized in man pages. Googling
around, a few seem to have a section called "IMPLEMENTATION NOTES",
which would be a good fit here. We don't have such sections currently,
but how about it?

> There should be an installcheck target.

Doesn't seem appropriate, as there are no regression tests that would
run against an existing cluster. Or should it just use the installed
pg_rewind and postgres binary, but create the temporary clusters all the
same?

> RewindTest.pm should be in the t/ directory.

I used a similar layout in src/test/ssl, so that ought to be changed too
if we're not happy with it.

"make check" will not find the module if I just move it to t/, so that
would require changes to Makefiles or something. I don't know how to do
that offhand.

> Instead of FILE_TYPE_DIRECTORY etc., why not use S_IFDIR etc.?

I don't see what that would buy us. Most of the code only knows about a
few S_IF* types, namely regular files, directories and symlinks. Those
are the types that there are FILE_TYPE_* codes for. If we started using
the more general S_IF* constants, it would not be clear which types can
appear in which parts of the code. Also, the compiler would no longer
warn about omitting one of the types in a "switch(file->type)" statement.

> TestLib.pm addition command_is sounds a bit wrong. It's evidently
> modelled after command_like, but that now sounds wrong too. How about
> command_stdout_is?

Works for me. How about also renaming command_like to
command_stdout_like, and committing that along with the new
command_stdout_is function as a separate patch, before pg_rewind?

> The test suite needs to silence all non-TAP output. So psql needs to
> be run with -q pg_ctl with -s etc. Any important output needs to be
> through diag() or note().
>
> Test cases like
>
> ok 6 - psql -A -t --no-psqlrc -c port=10072 -c SELECT datname FROM pg_database exit code 0
>
> should probably get a real name.
>
> The whole structure of the test suite still looks too much like the
> old hack. I'll try to think of other ways to structure it.

Yeah. It currently works with callback functions, like:

test program:
-> call RewindTest::run_rewind_test
set up both cluster
-> call before_standby callback
start standby
-> call standby_following_master callback
promote standby
-> call after_promotion callback
stop master
run pg_rewind
restart master
-> call after_rewind callback

It might be better to turn the control around, so that all the code
that's now in the callbacks are in the test program's main flow, and
test program calls functions in RewindTest.sh to execute the parts that
are currently between the callbacks:

test program
-> call RewindTest::setup_cluster
do stuff that's now in before_standby callback
-> call RewindTest::start_standby
do stuff that's now in standby_following_master callback
-> call RewindTest::promote_standby
do stuff that's now in after_promotion callback
-> call RewindTest::run_pg_rewind
do stuff that's now in after_rewind callback

- Heikki

Heikki Linnakangas wrote:
> Addressed most of your comments, and Michael's. Another version attached.

Looking at the set of TAP tests, I think that those lines open again
the door of CVE-2014-0067 (vulnerability with make check) on Windows:
# Initialize master, data checksums are mandatory
remove_tree($test_master_datadir);
system_or_bail("initdb -N -A trust -D $test_master_datadir
>>$log_path");
IMO we should use standard_initdb in TestLib.pm instead as pg_regress
--config-auth would be used for SSPI. standard_initdb should be
extended a bit as well to be able to pass a path to logs with
/dev/null as default. TAP tests do not run on Windows, still I think
that it would be better to cover any eventuality in this area before
we forget. Already mentioned by Peter, but I think as well that the
new additions to TAP should be a separate patch.

Random thought (not related to this patch), have a new option in
initdb doing this legwork:
+ # Accept replication connections on master
+ append_to_file("$test_master_datadir/pg_hba.conf", qq(
+local replication all trust
+host replication all 127.0.0.1/32 trust
+host replication all ::1/128 trust
+));

I am still getting a warning when building with MSVC:
xlogreader.obj : warning LNK4049: locally defined symbol
pg_crc32c_table imported
[C:\Users\ioltas\git\postgres\pg_rewind.vcxproj]
1 Warning(s)
0 Error(s)

Nitpicking: number of spaces here is incorrect:
+ that when <productname>PostgreSQL</> is started, it will start replay
+ from that checkpoint and apply all the required WAL.)
+ </para>

The header of src/bin/pg_rewind/Makefile mentions pg_basebackup:
+#-------------------------------------------------------------------------
+#
+# Makefile for src/bin/pg_basebackup

In this Makefile as well, I think that EXTRA_CLEAN can be removed:
+EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c
Regards,
--
Michael

On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Heikki Linnakangas wrote:
>> Addressed most of your comments, and Michael's. Another version attached.

Extra thing: src/bin/pg_rewind/Makefile surely forgets to clean up the
stuff from the regression tests:
+clean distclean maintainer-clean:
+ rm -f pg_rewind$(X) $(OBJS) xlogreader.c
--
Michael

On Mon, Jan 19, 2015 at 2:50 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > Heikki Linnakangas wrote:
> >> Addressed most of your comments, and Michael's. Another version
> attached.
>
> Extra thing: src/bin/pg_rewind/Makefile surely forgets to clean up the
> stuff from the regression tests:
> +clean distclean maintainer-clean:
> + rm -f pg_rewind$(X) $(OBJS) xlogreader.c
>

Heikki, what's your status here?
--
Michael

On 01/19/2015 07:38 AM, Michael Paquier wrote:
> Looking at the set of TAP tests, I think that those lines open again
> the door of CVE-2014-0067 (vulnerability with make check) on Windows:
> # Initialize master, data checksums are mandatory
> remove_tree($test_master_datadir);
> system_or_bail("initdb -N -A trust -D $test_master_datadir
>>> $log_path");
> IMO we should use standard_initdb in TestLib.pm instead as pg_regress
> --config-auth would be used for SSPI. standard_initdb should be
> extended a bit as well to be able to pass a path to logs with
> /dev/null as default. TAP tests do not run on Windows, still I think
> that it would be better to cover any eventuality in this area before
> we forget. Already mentioned by Peter, but I think as well that the
> new additions to TAP should be a separate patch.

Agreed, fixed to use standard_initdb. .

> Random thought (not related to this patch), have a new option in
> initdb doing this legwork:
> + # Accept replication connections on master
> + append_to_file("$test_master_datadir/pg_hba.conf", qq(
> +local replication all trust
> +host replication all 127.0.0.1/32 trust
> +host replication all ::1/128 trust
> +));

Yeah, that would be good. Perhaps as part of the pg_regress
--config-auth. If it's an initdb, then it might make sense to have the
same option to set wal_level=hot_standby, and max_wal_senders, so that
the cluster is immediately ready for replication. But that's a different
topic, I'm going to just leave it as it is in this pg_rewind patch.

Attached is a new patch version, fixing all the little things you
listed. I believe this is pretty much ready for commit. I'm going to
read it through myself one more time before committing, but I don't have
anything mind now that needs fixing anymore. I just pushed the change to
split dbcommands.h into dbcommands.h and dbcommands_xlog.h, as that
seems like a nice-to-have anyway.

- Heikki

Heikki Linnakangas wrote:

> Attached is a new patch version, fixing all the little things you listed. I
> believe this is pretty much ready for commit. I'm going to read it through
> myself one more time before committing, but I don't have anything mind now
> that needs fixing anymore.

I do -- it's obvious that you've given some consideration to
translatability, but it's not complete: anything that's using fprintf()
and friends are not marked for translation with _(). There are lots of
things using pg_fatal etc and those are probably fine.

One big thing that's not currently translated is usage(), but there are
others.

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

On Mon, Mar 9, 2015 at 11:59 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Heikki Linnakangas wrote:
>
>> Attached is a new patch version, fixing all the little things you listed. I
>> believe this is pretty much ready for commit. I'm going to read it through
>> myself one more time before committing, but I don't have anything mind now
>> that needs fixing anymore.
>
> I do -- it's obvious that you've given some consideration to
> translatability, but it's not complete: anything that's using fprintf()
> and friends are not marked for translation with _(). There are lots of
> things using pg_fatal etc and those are probably fine.
>
> One big thing that's not currently translated is usage(), but there are
> others.

Definitely.

On top of that, I had an extra look at this patch, testing pg_rewind
with OSX, Linux, MinGW-32 and MSVC. Particularly on Windows, I have
been able to rewind a node and to reconnect it to a promoted standby
using this utility compiled on both MinGW-32 and MSVC (nothing fancy
with two services, one master and a standby on a local server). I
think nobody has tested that until now though...

A minor comment:
+ <para>
+ <application>pg_rewind</> is a tool for synchronizing a PostgreSQL cluster
+ with another copy of the same cluster, after the clusters' timelines have
PostgreSQL needs the markup <productname>.
--
Michael

On Mon, Mar 9, 2015 at 7:32 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> Attached is a new patch version, fixing all the little things you listed.
I believe this is pretty much ready for commit. I'm going to read it
through myself one more time before committing, but I don't have anything
mind now that needs fixing anymore. I just pushed the change to split
dbcommands.h into dbcommands.h and dbcommands_xlog.h, as that seems like a
nice-to-have anyway.
>

Few assorted comments:

1.
+ <step>
+ <para>
+ Copy all those changed blocks from the new cluster to the old
cluster.
+
</para>
+ </step>

Isn't it possible incase of async replication that old cluster has
some blocks which new cluster doesn't have, what will it do
in such a case?

I have tried to test some form of such a case and it seems to be
failing with below error:

pg_rewind.exe -D ..\..\Data\ --source-pgdata=..\..\Database1
The servers diverged at WAL position 0/16DE858 on timeline 1.
Rewinding from last common checkpoint at 0/16B8A70 on timeline 1

could not open file "..\..\Data\/base/12706/16391" for truncation: No such
file
or directory
Failure, exiting

Exact scenario is:

Node -1 (master):
Step-1
Create table t1(c1 int, c2 char(500)) with (fillfactor=10);
insert into t1 values(generate_series(1,110),'aaaa');
Stop Node-1 (pg_ctl stop ..)

Step-2
Copy manually the data-directory (it contains WAL log as well)
to new location say Database1

Node-2 (standby)
Step-3
Change settings to make it stand-by (recovery.conf and change
postgresql.conf)
Start Node and verify all data exists.

Step-4
use pg_ctl promote to make Node-2 as master

Step-5
Start Node-1
insert few more records
insert into t1 values(generate_series(110,115),'aaaa');

Step-6
Node-2
Now insert one more records in table t1
insert into t1 values(116,'aaaa');

Stop both the nodes.

Now if I run pg_rewind on old-master(Node-1), it will lead to above error.
I think above scenario can be possible in async replication.

If I insert more records (greater than what I inserted in Step-5)
in Step-6, then pg_rewind works fine.

2.
diff --git a/src/bin/pg_rewind/RewindTest.pm
b/src/bin/pg_rewind/RewindTest.pm

+# To run a test, the test script (in t/ subdirectory) calls the functions

What do you mean by t/ subdirectory?

3.
+ <application>pg_rewind</> was run, and thereforce could not be copied by

typo /thereforce

4.
+static void
+sanityChecks(void)
+{
+ /* Check that there's no backup_label in either cluster */

I could not see such a check in code. Am I missing anything?

5.
+ /*
+ * TODO: move old file out of the way, if any. And perhaps create the
+ * file with temporary name first and rename in place after it's done.
+ */
+ snprintf(BackupLabelFilePath, MAXPGPATH,
+ "%s/backup_label" /* BACKUP_LABEL_FILE */, datadir_target);
+

There are couple of other TODO's in the patch, are these for future?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Michael Paquier wrote:

> On top of that, I had an extra look at this patch, testing pg_rewind
> with OSX, Linux, MinGW-32 and MSVC. Particularly on Windows, I have
> been able to rewind a node and to reconnect it to a promoted standby
> using this utility compiled on both MinGW-32 and MSVC (nothing fancy
> with two services, one master and a standby on a local server). I
> think nobody has tested that until now though...

Sweet!

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

On 03/10/2015 07:46 AM, Amit Kapila wrote:
> On Mon, Mar 9, 2015 at 7:32 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>
>> Attached is a new patch version, fixing all the little things you listed.
> I believe this is pretty much ready for commit. I'm going to read it
> through myself one more time before committing, but I don't have anything
> mind now that needs fixing anymore. I just pushed the change to split
> dbcommands.h into dbcommands.h and dbcommands_xlog.h, as that seems like a
> nice-to-have anyway.
>>
>
> Few assorted comments:
>
> 1.
> + <step>
> + <para>
> + Copy all those changed blocks from the new cluster to the old
> cluster.
> +
> </para>
> + </step>
>
> Isn't it possible incase of async replication that old cluster has
> some blocks which new cluster doesn't have, what will it do
> in such a case?

Sure, that's certainly possible. If the source cluster doesn't have some
blocks that exist in the target, IOW a file in the source cluster is
shorter than the same file in the target, that means that the relation
was truncated in the source. pg_rewind will truncate the file in the
target to match the source's size, although that's not strictly
necessary as there will also be a WAL record in the source about the
truncation. That will be replayed on the first startup after pg_rewind
and would do the truncation anyway.

> I have tried to test some form of such a case and it seems to be
> failing with below error:
>
> pg_rewind.exe -D ..\..\Data\ --source-pgdata=..\..\Database1
> The servers diverged at WAL position 0/16DE858 on timeline 1.
> Rewinding from last common checkpoint at 0/16B8A70 on timeline 1
>
> could not open file "..\..\Data\/base/12706/16391" for truncation: No such
> file
> or directory
> Failure, exiting

Hmm, could that be just because of the funny business with the Windows
path separators? Does it work if you use "-D ..\..\Data" instead,
without the last backslash?

> +# To run a test, the test script (in t/ subdirectory) calls the functions
>
> What do you mean by t/ subdirectory?

There is a directory, "src/bin/pg_rewind/t", which contains the
regression tests.

- Heikki

On Wed, Mar 11, 2015 at 3:44 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> On 03/10/2015 07:46 AM, Amit Kapila wrote:
>
>>
>> Isn't it possible incase of async replication that old cluster has
>> some blocks which new cluster doesn't have, what will it do
>> in such a case?
>>
>
> Sure, that's certainly possible. If the source cluster doesn't have some
> blocks that exist in the target, IOW a file in the source cluster is
> shorter than the same file in the target, that means that the relation was
> truncated in the source.

Can't that happen if the source database (new-master) haven't
received all of the data from target database (old-master) at the
time of promotion?
If yes, then source database won't have WAL for truncation and
the way current mechanism works is must.

Now I think for such a case doing truncation in the target database
is the right solution, however should we warn user in some way
(either by mentioning about it in docs or in the pg_rewind utility after
it does truncation) that some of it's data that belongs to old-master
will be overridden by this operation, so if he wants he can keep a
backup copy of the same.

> I have tried to test some form of such a case and it seems to be
>> failing with below error:
>>
>> pg_rewind.exe -D ..\..\Data\ --source-pgdata=..\..\Database1
>> The servers diverged at WAL position 0/16DE858 on timeline 1.
>> Rewinding from last common checkpoint at 0/16B8A70 on timeline 1
>>
>> could not open file "..\..\Data\/base/12706/16391" for truncation: No such
>> file
>> or directory
>> Failure, exiting
>>
>
> Hmm, could that be just because of the funny business with the Windows
> path separators? Does it work if you use "-D ..\..\Data" instead, without
> the last backslash?
>
>
I have tried without backslash as well, but still it returns
same error.

pg_rewind.exe -D ..\..\Data --source-pgdata=..\..\Database1
The servers diverged at WAL position 0/1769BD8 on timeline 5.
Rewinding from last common checkpoint at 0/1769B30 on timeline 5

could not open file "..\..\Data/base/12706/16394" for truncation: No such
file or directory
Failure, exiting

I have even tried with complete path:
pg_rewind.exe -D E:\WorkSpace\PostgreSQL\master\Data
--source-pgdata=E:\WorkSpace\PostgreSQL\master\Database1
The servers diverged at WAL position 0/1782830 on timeline 6.
Rewinding from last common checkpoint at 0/1782788 on timeline 6

could not open file "E:\WorkSpace\PostgreSQL\master\Data/base/12706/16395"
for truncation: No such file or directory
Failure, exiting

Another point is that after above error, target database
gets corrupted. Basically the target database contains
an extra data of source database and part of it's data.
I think thats because truncation didn't happened.

On retry it gives below message:
pg_rewind.exe -D ..\..\Data --source-pgdata=..\..\Database1

source and target cluster are on the same timeline
Failure, exiting

I think message displayed in this case is okay, however
displaying it as 'Failure' looks slightly odd.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 03/11/2015 05:01 AM, Amit Kapila wrote:
> On Wed, Mar 11, 2015 at 3:44 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
>> On 03/10/2015 07:46 AM, Amit Kapila wrote:
>>
>>> Isn't it possible incase of async replication that old cluster has
>>> some blocks which new cluster doesn't have, what will it do
>>> in such a case?
>>
>> Sure, that's certainly possible. If the source cluster doesn't have some
>> blocks that exist in the target, IOW a file in the source cluster is
>> shorter than the same file in the target, that means that the relation was
>> truncated in the source.
>
> Can't that happen if the source database (new-master) haven't
> received all of the data from target database (old-master) at the
> time of promotion?
> If yes, then source database won't have WAL for truncation and
> the way current mechanism works is must.
>
> Now I think for such a case doing truncation in the target database
> is the right solution,

Yeah, that can happen, and truncation is the correct fix for it. The
logic is pretty well explained by this comment in filemap.c:

/*
* It's a data file that exists in both.
*
* If it's larger in target, we can truncate it. There will
* also be a WAL record of the truncation in the source
* system, so WAL replay would eventually truncate the target
* too, but we might as well do it now.
*
* If it's smaller in the target, it means that it has been
* truncated in the target, or enlarged in the source, or
* both. If it was truncated locally, we need to copy the
* missing tail from the remote system. If it was enlarged in
* the remote system, there will be WAL records in the remote
* system for the new blocks, so we wouldn't need to copy them
* here. But we don't know which scenario we're dealing with,
* and there's no harm in copying the missing blocks now, so
* do it now.
*
* If it's the same size, do nothing here. Any locally
* modified blocks will be copied based on parsing the local
* WAL, and any remotely modified blocks will be updated after
* rewinding, when the remote WAL is replayed.
*/

> however should we warn user in some way
> (either by mentioning about it in docs or in the pg_rewind utility after
> it does truncation) that some of it's data that belongs to old-master
> will be overridden by this operation, so if he wants he can keep a
> backup copy of the same.

Well, pg_rewind *always* overwrites any transactions that were committed
in the old master but not streamed to the standby. That's the whole
point. You're probably right that we should stress that out more in the
docs, though.

I've been thinking that it would be nice to print out a list of such
transactions that pg_rewind is going to overwrite. The admin could look
at the (hopefully short) list and perhaps try to fix up the data
manually afterwards, to recover the lost transactions. Perhaps we could
use the logical decoding stuff for that, to print out the transactions
in a human-readable format. But that's a TODO for the future.

>> I have tried to test some form of such a case and it seems to be
>>> failing with below error:
>>>
>>> pg_rewind.exe -D ..\..\Data\ --source-pgdata=..\..\Database1
>>> The servers diverged at WAL position 0/16DE858 on timeline 1.
>>> Rewinding from last common checkpoint at 0/16B8A70 on timeline 1
>>>
>>> could not open file "..\..\Data\/base/12706/16391" for truncation: No such
>>> file
>>> or directory
>>> Failure, exiting
>>
>> Hmm, could that be just because of the funny business with the Windows
>> path separators? Does it work if you use "-D ..\..\Data" instead, without
>> the last backslash?
>>
> I have tried without backslash as well, but still it returns
> same error.
>
> pg_rewind.exe -D ..\..\Data --source-pgdata=..\..\Database1
> The servers diverged at WAL position 0/1769BD8 on timeline 5.
> Rewinding from last common checkpoint at 0/1769B30 on timeline 5
>
> could not open file "..\..\Data/base/12706/16394" for truncation: No such
> file or directory
> Failure, exiting

I tried to reproduce this, but it tripped the "Assert(entry->isrelfile)"
assertion in process_block_change. However, that seems to be an
unrelated issue - pg_rewind was not handling FSM blocks correctly. It's
supposed to ignore them but extactPageInfo didn't get the memo. I think
I broke that when doing the changes for the new WAL record format.

After fixing that (new patch attached), your test case works fine for
me. I'm using the attached bash script to test it. Can you test if the
attached script works for you, and if it does, see if you can "fix" the
script so that it reproduces the error you're seeing?

> Another point is that after above error, target database
> gets corrupted. Basically the target database contains
> an extra data of source database and part of it's data.
> I think thats because truncation didn't happened.

Yeah, if something goes wrong during pg_rewind, the target database is
toast. Taking a backup, and using --dry-run is highly recommended ;-).
As a safety feature, perhaps pg_rewind should temporarily rename the
control file or something like that, so that if it's interrupted, the
target database will refuse to start up. Then again, that would make it
more difficult to do forensics or disaster recovery on the database, if
you didn't take a backup.

> On retry it gives below message:
> pg_rewind.exe -D ..\..\Data --source-pgdata=..\..\Database1
>
> source and target cluster are on the same timeline
> Failure, exiting
>
> I think message displayed in this case is okay, however
> displaying it as 'Failure' looks slightly odd.

Hmm. In other similar scenarios, pg_rewind will return Success with
message "No rewind required". Yeah, probably should do that in this case
too.

- Heikki

On Wed, Mar 11, 2015 at 2:23 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> On 03/11/2015 05:01 AM, Amit Kapila wrote:
>>
>> I have tried without backslash as well, but still it returns
>> same error.
>>
>> pg_rewind.exe -D ..\..\Data --source-pgdata=..\..\Database1
>> The servers diverged at WAL position 0/1769BD8 on timeline 5.
>> Rewinding from last common checkpoint at 0/1769B30 on timeline 5
>>
>> could not open file "..\..\Data/base/12706/16394" for truncation: No such
>> file or directory
>> Failure, exiting
>>
>
> I tried to reproduce this, but it tripped the "Assert(entry->isrelfile)"
> assertion in process_block_change. However, that seems to be an unrelated
> issue - pg_rewind was not handling FSM blocks correctly. It's supposed to
> ignore them but extactPageInfo didn't get the memo. I think I broke that
> when doing the changes for the new WAL record format.
>
> After fixing that (new patch attached), your test case works fine for me.
> I'm using the attached bash script to test it. Can you test if the attached
> script works for you, and if it does, see if you can "fix" the script so
> that it reproduces the error you're seeing?
>
>
With attached modified script, I am able to reproduce the
error (I have used the latest pg_rewind patch (pg_rewind-bin-8))

The servers diverged at WAL position 0/1693400 on timeline 1.
Rewinding from last common checkpoint at 0/1693390 on timeline 1

could not open file "data-master/base/12706/16384" for truncation: No such
file or directory
Failure, exiting

I am able to reproduce it on Windows (haven't tried it on linux).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 03/12/2015 08:49 AM, Amit Kapila wrote:
> With attached modified script, I am able to reproduce the
> error (I have used the latest pg_rewind patch (pg_rewind-bin-8))

Thanks! That reproduced the error for me too. Not sure why I couldn't
reproduce it earlier.

The cause was a silly typo in truncate_target_file:

> @@ -397,7 +397,7 @@ truncate_target_file(const char *path, off_t newsize)
>
> snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target, path);
>
> - fd = open(path, O_WRONLY, 0);
> + fd = open(dstpath, O_WRONLY, 0);
> if (fd < 0)
> pg_fatal("could not open file \"%s\" for truncation: %s\n",
> dstpath, strerror(errno));

Attached is a new version of the patch, including that fix, and rebased
over current git master.

- Heikki

On Fri, Mar 13, 2015 at 1:13 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> The cause was a silly typo in truncate_target_file:
>
>> @@ -397,7 +397,7 @@ truncate_target_file(const char *path, off_t newsize)
>>
>> snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target,
path);
>>
>> - fd = open(path, O_WRONLY, 0);
>> + fd = open(dstpath, O_WRONLY, 0);
>> if (fd < 0)
>> pg_fatal("could not open file \"%s\" for truncation:
%s\n",
>> dstpath, strerror(errno));
>
>
> Attached is a new version of the patch, including that fix, and rebased
over current git master.
>

I have verified that new patch has fixed the problem.

Few more observations:

Getting below linking error with Asserts enabled in Windows build.

1>xlogreader.obj : error LNK2019: unresolved external symbol
ExceptionalCondition referenced in function
XLogReadRecord
1>.\Debug\pg_rewind\pg_rewind.exe : fatal error LNK1120: 1 unresolved
externals

Am I doing anything wrong while building?

2.
msvc\clean.bat has below way to clean xlogreader.c for pg_xlogdump,
shouldn't something similar required for pg_rewind?

REM clean up files copied into contrib\pg_xlogdump
if exist contrib\pg_xlogdump\xlogreader.c del /q contrib
\pg_xlogdump\xlogreader.c
for %%f in (contrib\pg_xlogdump\*desc.c) do if not %%f==contrib\pg_xlogdump
\rmgrdesc.c del /q %%f

3.
+void
+remove_target(file_entry_t *entry)
{
..
+ case FILE_TYPE_REGULAR:
+ remove_target_symlink(entry->path);
+
break;
+
+ case FILE_TYPE_SYMLINK:
+ remove_target_file(entry-
>path);
+ break;
}

It seems function calls *_symlink and *_file are reversed, in reality it
might not matter much because the code for both functions is same,
but still it might diverge in future.

4.
Copyright notice contains variation in terms of years

+ * Copyright (c) 2010-2015, PostgreSQL Global Development Group
+ * Copyright (c) 2013-2015, PostgreSQL Global Development Group

+ * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group

Is there any particular reason for the same?

5.
+ * relation files. Other forks are alwayes copied in toto, because we
cannot
+ * reliably track changes to
the, because WAL only contains block references
+ * for the main fork.
+ */
+static bool
+isRelDataFile(const
char *path)

Sentence near "track changes to the, .." looks incomplete.

6.
+libpqConnect(const char *connstr)
{
..
+ /*
+ * Also check that full_page-writes are enabled. We can get torn pages if
+ * a page is
modified while we read it with pg_read_binary_file(), and we
+ * rely on full page images to fix them.
+
*/
+ str = run_simple_query("SHOW full_page_writes");
+ if (strcmp(str, "on") != 0)
+
pg_fatal("full_page_writes must be enabled in the source server\n");
+ pg_free(str);
..
}

Do you think it is worth to mention this information in docs?

7.
Function execute_pagemap() exists in both copy_fetch.c and
libpq_fetch.c, are you expecting that they will get diverged
in future?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila wrote:

> Getting below linking error with Asserts enabled in Windows build.
>
> 1>xlogreader.obj : error LNK2019: unresolved external symbol
> ExceptionalCondition referenced in function
> XLogReadRecord
> 1>.\Debug\pg_rewind\pg_rewind.exe : fatal error LNK1120: 1 unresolved
> externals
>
> Am I doing anything wrong while building?

Assert() gets defined in terms of ExceptionalCondition if building in
!FRONTEND, so it seems like the compile flags are wrong for pg_rewind's
copy of xlogreader.

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

On Wed, Mar 11, 2015 at 2:23 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 03/11/2015 05:01 AM, Amit Kapila wrote:
>>
>>
>>
>> Can't that happen if the source database (new-master) haven't
>> received all of the data from target database (old-master) at the
>> time of promotion?
>> If yes, then source database won't have WAL for truncation and
>> the way current mechanism works is must.
>>
>> Now I think for such a case doing truncation in the target database
>> is the right solution,
>
>
> Yeah, that can happen, and truncation is the correct fix for it. The
logic is pretty well explained by this comment in filemap.c:
>
>
> *
> * If it's the same size, do nothing here. Any locally
> * modified blocks will be copied based on parsing the local
> * WAL, and any remotely modified blocks will be updated after
> * rewinding, when the remote WAL is replayed.
> */
>

What about unlogged table, how will they handle this particular case?

I think after old-master and new-master got diverged any operations
on unlogged table won't guarantee that we can get those modified
blocks from new-master during pg_rewind and I think it can lead
to a case where unlogged tables have some data from old-master
and some data from new master considering user always take of
clean shut-down.

Typo in patch:

+ * For our purposes, only files belonging to the main fork are considered
+ * relation files. Other forks are alwayes copied in toto, because we
cannot

/alwayes/always

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 03/14/2015 02:31 PM, Amit Kapila wrote:
> Getting below linking error with Asserts enabled in Windows build.
>
> 1>xlogreader.obj : error LNK2019: unresolved external symbol
> ExceptionalCondition referenced in function
> XLogReadRecord
> 1>.\Debug\pg_rewind\pg_rewind.exe : fatal error LNK1120: 1 unresolved
> externals
>
> Am I doing anything wrong while building?

Works for me. Perhaps there were some changes to #includes that
inadvertently fixed it..

> 2.
> msvc\clean.bat has below way to clean xlogreader.c for pg_xlogdump,
> shouldn't something similar required for pg_rewind?
>
> REM clean up files copied into contrib\pg_xlogdump
> if exist contrib\pg_xlogdump\xlogreader.c del /q contrib
> \pg_xlogdump\xlogreader.c
> for %%f in (contrib\pg_xlogdump\*desc.c) do if not %%f==contrib\pg_xlogdump
> \rmgrdesc.c del /q %%f
> y.

I changed the way pg_xlogdump does that, and pg_rewind follows the new
example. (see http://www.postgresql.org/message-id/550B14A5.7060708@iki.fi)

> 4.
> Copyright notice contains variation in terms of years
>
> + * Copyright (c) 2010-2015, PostgreSQL Global Development Group
> + * Copyright (c) 2013-2015, PostgreSQL Global Development Group
>
> + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
>
> Is there any particular reason for the same?

I've created many of the files by copying an old file and modifying
heavily. The copyright notices have been carried over from the original
files. Many of the files would still contain some of the original copied
code, while others might not. I'm not sure what the best way to deal
with that is - stamp everything as 2015, 2013-2015, or leave them as
they are. It doesn't really matter in practice.

> 5.
> + * relation files. Other forks are alwayes copied in toto, because we
> cannot
> + * reliably track changes to
> the, because WAL only contains block references
> + * for the main fork.
> + */
> +static bool
> +isRelDataFile(const
> char *path)
>
> Sentence near "track changes to the, .." looks incomplete.

Fixed, it was supposed to be "them", not "the".

> 6.
> +libpqConnect(const char *connstr)
> {
> ..
> + /*
> + * Also check that full_page-writes are enabled. We can get torn pages if
> + * a page is
> modified while we read it with pg_read_binary_file(), and we
> + * rely on full page images to fix them.
> +
> */
> + str = run_simple_query("SHOW full_page_writes");
> + if (strcmp(str, "on") != 0)
> +
> pg_fatal("full_page_writes must be enabled in the source server\n");
> + pg_free(str);
> ..
> }
>
> Do you think it is worth to mention this information in docs?

Added.

> 7.
> Function execute_pagemap() exists in both copy_fetch.c and
> libpq_fetch.c, are you expecting that they will get diverged
> in future?

They look pretty much identical, but the copy_file_range functions they
call are in fact separate functions, and differ a lot. I have renamed
the libpq version to avoid confusion.

I have committed this, with some more kibitzing. hope I have not missed
any comments given so far. Many thanks for the review, and please
continue reviewing and testing it :-).

- Heikki

> I have committed this, with some more kibitzing. hope I have not missed
> any comments given so far. Many thanks for the review, and please continue
> reviewing and testing it :-).

I have been testing the pg_rewind and have an analysis to share along with
few questions -

I had a streaming replication setup with one master and one slave running
successfully.

Test 1 :

- Killed postgres process on master and promoted slave. Both were in sync
earlier.
- performed some operations (data changes) on newly promoted slave node and
shutdown
- Executed pg_rewind on old master and got the below message

*target server must be shut down cleanly*
*Failure, exiting*

If the master is crashed or killed abruptly, it may not be possible to do a
rewind. Is my understanding correct ?

Test 2 :

- On a successfully running streaming replication with one master and one
slave, i did a clean shutdown of master
- promoted slave
- performed some operations (data changes) on newly promoted slave and did
a clean shutdown
- Executed pg_rewind on the old master to sync with the latest changes on
new master. I got the below message

*The servers diverged at WAL position 0/A2000098 on timeline 1.*
*No rewind required.*

I am not getting this too.

Regards,
Venkata Balaji N

On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N <nag1010(at)gmail(dot)com> wrote:
> Test 1 :
>
> [...]
>
> If the master is crashed or killed abruptly, it may not be possible to do a
> rewind. Is my understanding correct ?

Yep. This is mentioned in the documentation:
http://www.postgresql.org/docs/devel/static/app-pgrewind.html
"The target server must shut down cleanly before running pg_rewind".

> Test 2 :
>
> - On a successfully running streaming replication with one master and one
> slave, i did a clean shutdown of master
> - promoted slave
> - performed some operations (data changes) on newly promoted slave and did a
> clean shutdown
> - Executed pg_rewind on the old master to sync with the latest changes on
> new master. I got the below message
>
> The servers diverged at WAL position 0/A2000098 on timeline 1.
> No rewind required.
>
> I am not getting this too.

In this case the master WAL visibly did not diverge from the slave WAL
line. A rewind is done if the master touches new relation pages after
the standby has been promoted, and before the master is shutdown.
--
Michael

> 26 марта 2015 г., в 7:32, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> написал(а):
>
> On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N <nag1010(at)gmail(dot)com> wrote:
>> Test 1 :
>>
>> [...]
>>
>> If the master is crashed or killed abruptly, it may not be possible to do a
>> rewind. Is my understanding correct ?
>
> Yep. This is mentioned in the documentation:
> http://www.postgresql.org/docs/devel/static/app-pgrewind.html
> "The target server must shut down cleanly before running pg_rewind».

You can start old master, wait for crash recovery to complete, stop it cleanly and then use pg_rewind. It works.

>
>> Test 2 :
>>
>> - On a successfully running streaming replication with one master and one
>> slave, i did a clean shutdown of master
>> - promoted slave
>> - performed some operations (data changes) on newly promoted slave and did a
>> clean shutdown
>> - Executed pg_rewind on the old master to sync with the latest changes on
>> new master. I got the below message
>>
>> The servers diverged at WAL position 0/A2000098 on timeline 1.
>> No rewind required.
>>
>> I am not getting this too.
>
> In this case the master WAL visibly did not diverge from the slave WAL
> line. A rewind is done if the master touches new relation pages after
> the standby has been promoted, and before the master is shutdown.
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Да пребудет с вами сила…
https://simply.name/ru

On Mar 26, 2015 4:20 AM, "Vladimir Borodin" <root(at)simply(dot)name> wrote:
>
>
>> 26 марта 2015 г., в 7:32, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
написал(а):
>>
>> On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N <nag1010(at)gmail(dot)com>
wrote:
>>>
>>> Test 1 :
>>>
>>> [...]
>>>
>>> If the master is crashed or killed abruptly, it may not be possible to
do a
>>> rewind. Is my understanding correct ?
>>
>>
>> Yep. This is mentioned in the documentation:
>> http://www.postgresql.org/docs/devel/static/app-pgrewind.html
>> "The target server must shut down cleanly before running pg_rewind>>.
>
>
> You can start old master, wait for crash recovery to complete, stop it
cleanly and then use pg_rewind. It works.
>

Shouldn't we have a flag so it does that automatically if necessary?

>>
>>> Test 2 :
>>>
>>> - On a successfully running streaming replication with one master and
one
>>> slave, i did a clean shutdown of master
>>> - promoted slave
>>> - performed some operations (data changes) on newly promoted slave and
did a
>>> clean shutdown
>>> - Executed pg_rewind on the old master to sync with the latest changes
on
>>> new master. I got the below message
>>>
>>> The servers diverged at WAL position 0/A2000098 on timeline 1.
>>> No rewind required.
>>>
>>> I am not getting this too.
>>
>>
>> In this case the master WAL visibly did not diverge from the slave WAL
>> line. A rewind is done if the master touches new relation pages after
>> the standby has been promoted, and before the master is shutdown.
>> --
>> Michael
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
>
> --
> Да пребудет с вами сила...
> https://simply.name/ru
>

On 03/26/2015 09:45 PM, Arthur Silva wrote:
> On Mar 26, 2015 4:20 AM, "Vladimir Borodin" <root(at)simply(dot)name> wrote:
>>
>>
>>> 26 марта 2015 г., в 7:32, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> написал(а):
>>>
>>> On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N <nag1010(at)gmail(dot)com>
> wrote:
>>>> If the master is crashed or killed abruptly, it may not be possible to
> do a
>>>> rewind. Is my understanding correct ?
>>>
>>> Yep. This is mentioned in the documentation:
>>> http://www.postgresql.org/docs/devel/static/app-pgrewind.html
>>> "The target server must shut down cleanly before running pg_rewind>>.
>>
>> You can start old master, wait for crash recovery to complete, stop it
> cleanly and then use pg_rewind. It works.
>
> Shouldn't we have a flag so it does that automatically if necessary?

Might be handy, but currently pg_rewind never invokes postgres or other
binaries, so it would be a fair amount of new functionality to implement
that. Let's keep it simple for now.
- Heikki