Re: [HACKERS] pg_rewind in contrib
On 03/26/2015 09:45 PM, Arthur Silva wrote: On Mar 26, 2015 4:20 AM, Vladimir Borodin r...@simply.name wrote: 26 марта 2015 г., в 7:32, Michael Paquier michael.paqu...@gmail.com написал(а): On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N nag1...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Mar 26, 2015 4:20 AM, Vladimir Borodin r...@simply.name wrote: 26 марта 2015 г., в 7:32, Michael Paquier michael.paqu...@gmail.com написал(а): On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N nag1...@gmail.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/A298 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Да пребудет с вами сила... https://simply.name/ru
Re: [HACKERS] pg_rewind in contrib
26 марта 2015 г., в 7:32, Michael Paquier michael.paqu...@gmail.com написал(а): On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N nag1...@gmail.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/A298 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Да пребудет с вами сила… https://simply.name/ru
Re: [HACKERS] pg_rewind in contrib
On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N nag1...@gmail.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/A298 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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/A298 on timeline 1.* *No rewind required.* I am not getting this too. Regards, Venkata Balaji N
Re: [HACKERS] pg_rewind in contrib
On 03/14/2015 02:31 PM, Amit Kapila wrote:
Getting below linking error with Asserts enabled in Windows build.
1xlogreader.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.7060...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Wed, Mar 11, 2015 at 2:23 PM, Heikki Linnakangas hlinn...@iki.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
Re: [HACKERS] pg_rewind in contrib
Amit Kapila wrote: Getting below linking error with Asserts enabled in Windows build. 1xlogreader.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] pg_rewind in contrib
On Fri, Mar 13, 2015 at 1:13 AM, Heikki Linnakangas hlinn...@iki.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.
1xlogreader.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
Re: [HACKERS] pg_rewind in contrib
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 pg_rewind-bin-9.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Wed, Mar 11, 2015 at 2:23 PM, Heikki Linnakangas hlinn...@iki.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 amits-test-modify.sh Description: Bourne shell script -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On 03/11/2015 05:01 AM, Amit Kapila wrote: On Wed, Mar 11, 2015 at 3:44 AM, Heikki Linnakangas hlinn...@iki.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
Re: [HACKERS] pg_rewind in contrib
On Mon, Mar 9, 2015 at 11:59 PM, Alvaro Herrera alvhe...@2ndquadrant.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 + applicationpg_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] pg_rewind in contrib
On Wed, Mar 11, 2015 at 3:44 AM, Heikki Linnakangas hlinn...@iki.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
Re: [HACKERS] pg_rewind in contrib
On Mon, Mar 9, 2015 at 7:32 PM, Heikki Linnakangas hlinn...@iki.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),'');
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),'');
Step-6
Node-2
Now insert one more records in table t1
insert into t1 values(116,'');
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.
+ applicationpg_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
Re: [HACKERS] pg_rewind in contrib
On 03/10/2015 07:46 AM, Amit Kapila wrote: On Mon, Mar 9, 2015 at 7:32 PM, Heikki Linnakangas hlinn...@iki.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 pg_rewind-bin-7.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] pg_rewind in contrib
On Mon, Jan 19, 2015 at 2:50 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier michael.paqu...@gmail.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
Re: [HACKERS] pg_rewind in contrib
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 productnamePostgreSQL/ 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier michael.paqu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 pg_rewind-bin-6.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:
Re: [HACKERS] pg_rewind in contrib
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On 2015-01-15 13:21:56 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Wed, Jan 7, 2015 at 8:44 AM, Andrew Dunstan and...@dunslane.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Wed, Jan 14, 2015 at 6:43 PM, Heikki Linnakangas hlinnakan...@vmware.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 applicationpg_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 applicationpg_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 diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c index 180818d..deb1b8f 100644 --- a/contrib/pg_xlogdump/rmgrdesc.c +++ b/contrib/pg_xlogdump/rmgrdesc.c @@ -23,6 +23,7 @@ #include access/xlog_internal.h #include catalog/storage_xlog.h #include commands/dbcommands.h +#include commands/dbcommands_xlog.h #include commands/sequence.h #include commands/tablespace.h #include rmgrdesc.h -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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.ga18...@toroid.org 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
Re: [HACKERS] pg_rewind in contrib
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
On Fri, Jan 9, 2015 at 1:02 AM, Heikki Linnakangas
hlinnakan...@vmware.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-4c...@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
varnamewal_log_hints/ to literalon/
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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
pg_rewind-contrib-4.patch.gz
Description: application/gzip
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 pg_rewind-contrib-3.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
On Wed, Jan 7, 2015 at 5:39 AM, Peter Eisentraut pete...@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
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
On Tue, Jan 6, 2015 at 2:38 AM, Heikki Linnakangas
hlinnakan...@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
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
Re: [HACKERS] pg_rewind in contrib
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
diff --git a/contrib/Makefile b/contrib/Makefile
index 195d447..2fe861f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -32,6 +32,7 @@ SUBDIRS = \
pg_buffercache \
pg_freespacemap \
pg_prewarm \
+ pg_rewind \
pg_standby \
pg_stat_statements \
pg_test_fsync \
diff --git a/contrib/pg_rewind/.gitignore b/contrib/pg_rewind/.gitignore
new file mode 100644
index 000..816a91d
--- /dev/null
+++ b/contrib/pg_rewind/.gitignore
@@ -0,0 +1,9 @@
+# Files generated during build
+/xlogreader.c
+
+# Generated by test suite
+/tmp_check/
+/regression.diffs
+/regression.out
+/results/
+/regress_log/
diff --git a/contrib/pg_rewind/Makefile b/contrib/pg_rewind/Makefile
new file mode 100644
index 000..241c775
--- /dev/null
+++ b/contrib/pg_rewind/Makefile
@@ -0,0 +1,51 @@
+# Makefile for pg_rewind
+#
+# Copyright (c) 2013-2014, PostgreSQL Global Development Group
+#
+
+PGFILEDESC = pg_rewind - repurpose an old master server as standby
+PGAPPICON = win32
+
+PROGRAM = pg_rewind
+OBJS = pg_rewind.o parsexlog.o xlogreader.o util.o datapagemap.o timeline.o \
+ fetch.o copy_fetch.o libpq_fetch.o filemap.o
+
+REGRESS = basictest extrafiles databases
+REGRESS_OPTS=--use-existing --launcher=./launcher
+
+PG_CPPFLAGS = -I$(libpq_srcdir)
+PG_LIBS = $(libpq_pgport)
+
+override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
+
+EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c
+
+all: pg_rewind
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/pg_rewind
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/%
+ rm -f $@ $(LN_S) $ .
+
+# The regression tests can be run separately against, using the libpq or local
+# method to copy the files. For local mode, the makefile target is
+# check-local, and for libpq mode, check-remote. The target check-both
+# runs the tests in both modes.
+check-local:
+ echo Running tests against local data directory, in copy-mode
+ bindir=$(bindir) TEST_SUITE=local $(MAKE) installcheck
+
+check-remote:
+ echo Running tests against a running standby, via libpq
+ bindir=$(bindir) TEST_SUITE=remote $(MAKE) installcheck
+
+check-both: check-local check-remote
diff --git a/contrib/pg_rewind/copy_fetch.c b/contrib/pg_rewind/copy_fetch.c
new file mode 100644
index 000..5c40ec7
--- /dev/null
+++ b/contrib/pg_rewind/copy_fetch.c
@@ -0,0 +1,586 @@
+/*-
+ *
+ * copy_fetch.c
+ * Functions for copying a PostgreSQL data directory
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ *
+ *-
+ */
+#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
+
+static void recurse_dir(const char *datadir, const char *path,
+ process_file_callback_t callback);
+
+static void execute_pagemap(datapagemap_t *pagemap, const char *path);
+
+static void remove_target_file(const char *path);
+static void create_target_dir(const char *path);
+static void remove_target_dir(const char *path);
+static void create_target_symlink(const char *path, const char *link);
+static void remove_target_symlink(const char *path);
+
+/*
+ * Traverse through all files in a data directory, calling 'callback'
+ * for each file.
+ */
+void
+traverse_datadir(const char *datadir, process_file_callback_t callback)
+{
+ /* should this copy config files or not? */
+ recurse_dir(datadir, NULL, callback);
+}
+
+/*
+ * recursive part of traverse_datadir
+ */
+static void
+recurse_dir(const char *datadir, const char *parentpath,
+ process_file_callback_t callback)
+{
+
Re: [HACKERS] pg_rewind in contrib
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 sn...@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
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
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 sn...@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
Re: [HACKERS] pg_rewind in contrib
On Sat, Dec 13, 2014 at 10:49 PM, David Fetter da...@fetter.org wrote: On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote: Heikki Linnakangas hlinnakan...@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.
Re: [HACKERS] pg_rewind in contrib
On Sat, Dec 13, 2014 at 12:01 AM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] pg_rewind in contrib
On Tue, Dec 16, 2014 at 9:32 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Dec 13, 2014 at 12:01 AM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] pg_rewind in contrib
On Tue, Dec 16, 2014 at 10:26 AM, Michael Paquier michael.paqu...@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
Re: [HACKERS] pg_rewind in contrib
On Fri, Dec 12, 2014 at 10:06:32AM -0500, Tom Lane wrote: Heikki Linnakangas hlinnakan...@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 da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@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
[HACKERS] 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 commit 2300e28b0d07328c7b37a92f7150e75edf24b10c Author: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Fri Dec 12 16:08:14 2014 +0200 Add pg_rewind to contrib. diff --git a/contrib/Makefile b/contrib/Makefile index 195d447..2fe861f 100644 --- a/contrib/Makefile +++ b/contrib/Makefile @@ -32,6 +32,7 @@ SUBDIRS = \ pg_buffercache \ pg_freespacemap \ pg_prewarm \ + pg_rewind \ pg_standby \ pg_stat_statements \ pg_test_fsync \ diff --git a/contrib/pg_rewind/.gitignore b/contrib/pg_rewind/.gitignore new file mode 100644 index 000..cb50df2 --- /dev/null +++ b/contrib/pg_rewind/.gitignore @@ -0,0 +1,32 @@ +# Object files +*.o + +# Libraries +*.lib +*.a + +# Shared objects (inc. Windows DLLs) +*.dll +*.so +*.so.* +*.dylib + +# Executables +*.exe +*.app + +# Dependencies +.deps + +# Files generated during build +/xlogreader.c + +# Binaries +/pg_rewind + +# Generated by test suite +/tmp_check/ +/regression.diffs +/regression.out +/results/ +/regress_log/ diff --git a/contrib/pg_rewind/Makefile b/contrib/pg_rewind/Makefile new file mode 100644 index 000..d50a8cf --- /dev/null +++ b/contrib/pg_rewind/Makefile @@ -0,0 +1,47 @@ +# Makefile for pg_rewind +# +# Copyright (c) 2013 VMware, Inc. All Rights Reserved. +# + +PGFILEDESC = pg_rewind - repurpose an old master server as standby +PGAPPICON = win32 + +PROGRAM = pg_rewind +OBJS = pg_rewind.o parsexlog.o xlogreader.o util.o datapagemap.o timeline.o \ + fetch.o copy_fetch.o libpq_fetch.o filemap.o + +REGRESS = basictest extrafiles databases +REGRESS_OPTS=--use-existing --launcher=./launcher + +PG_CPPFLAGS = -I$(libpq_srcdir) +PG_LIBS = $(libpq_pgport) + +override CPPFLAGS := -DFRONTEND $(CPPFLAGS) + +EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c + +all: pg_rewind + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = contrib/pg_rewind +top_builddir = ../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif + +xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/% + rm -f $@ $(LN_S) $ . + +check-local: + echo Running tests against local data directory, in copy-mode + bindir=$(bindir) TEST_SUITE=local $(MAKE) installcheck + +check-remote: + echo Running tests against a running standby, via libpq + bindir=$(bindir) TEST_SUITE=remote $(MAKE) installcheck + +check-both: check-local check-remote diff --git a/contrib/pg_rewind/README b/contrib/pg_rewind/README new file mode 100644 index 000..cac6095 --- /dev/null +++ b/contrib/pg_rewind/README @@ -0,0 +1,100 @@ +pg_rewind += + +pg_rewind is a tool for synchronizing a PostgreSQL data directory with another +PostgreSQL data directory that was forked from the first one. The result is +equivalent to rsyncing the first data directory (referred to as the old cluster +from now on) with the second one (the new cluster). The advantage of pg_rewind +over rsync is that pg_rewind uses the WAL to determine changed data blocks, +and does not require reading through all files in the cluster. That makes it +a lot faster when the database is large and only a small portion of it differs +between the clusters. + +Download + + +The latest version of this software can be found on the project website at +https://github.com/vmware/pg_rewind. + +Installation + + +Compiling pg_rewind requires the PostgreSQL source tree to be available. +There are two ways to do that: + +1. Put pg_rewind project directory inside PostgreSQL source tree as +contrib/pg_rewind, and use make to compile + +or + +2. Pass the path to the PostgreSQL source tree to make, in the top_srcdir +variable: make USE_PGXS=1 top_srcdir=path to PostgreSQL source tree + +In addition, you must have pg_config in $PATH. + +The current version of pg_rewind is compatible with PostgreSQL version 9.4. + +Usage +- + +pg_rewind --target-pgdata=path \ +--source-server=new server's conn string + +The contents of the old data directory will be overwritten with the new data +so that after pg_rewind finishes, the
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
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 br...@momjian.ushttp://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
Re: [HACKERS] pg_rewind in contrib
On Fri, Dec 12, 2014 at 11:13 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] pg_rewind in contrib
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
Re: [HACKERS] pg_rewind in contrib
Heikki Linnakangas hlinnakan...@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
