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
Attachments:
pg_rewind-contrib.patchtext/x-diff; name=pg_rewind-contrib.patchDownload+3699-0
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 12, 2014 at 11:13 PM, Heikki Linnakangas
<hlinnakangas@vmware.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnakangas@vmware.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote:
Heikki Linnakangas <hlinnakangas@vmware.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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 13, 2014 at 10:49 PM, David Fetter <david@fetter.org> wrote:
On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote:
Heikki Linnakangas <hlinnakangas@vmware.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@vmware.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 16, 2014 at 9:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Sat, Dec 13, 2014 at 12:01 AM, Heikki Linnakangas
<hlinnakangas@vmware.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 16, 2014 at 10:26 AM, Michael Paquier
<michael.paquier@gmail.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--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@uptime.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@uptime.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
pg_rewind-contrib-2.patchtext/x-diff; name=pg_rewind-contrib-2.patchDownload+3714-0
On Tue, Jan 6, 2015 at 2:38 AM, Heikki Linnakangas
<hlinnakangas@vmware.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
againSome 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 7, 2015 at 5:39 AM, Peter Eisentraut <peter_e@gmx.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
