pg_rewind : feature to rewind promoted standby is broken!
I think pg_rewind's feature to rewind the promoted standby as a new
standby is broken in 11
STEPS:
1. create master standby setup.
Use below script for same.
2. Promote the standby
[mithuncy@localhost pgrewmasterbin]$ ./bin/pg_ctl -D standby promote
waiting for server to promote.... done
server promoted
3. In promoted standby create a database and a table in the new database.
[mithuncy@localhost pgrewmasterbin]$ ./bin/psql -p 5433 postgres
postgres=# create database db1;
CREATE DATABASE
postgres=# \c db1
You are now connected to database "db1" as user "mithuncy".
db1=# create table t1 (t int);
CREATE TABLE
4. try to rewind the newly promoted standby (with old master as source)
[mithuncy@localhost pgrewmasterbin]$ ./bin/pg_ctl -D standby stop
waiting for server to shut down....... done
server stopped
[mithuncy@localhost pgrewmasterbin]$ ./bin/pg_rewind -D standby
--source-server="host=127.0.0.1 port=5432 user=mithuncy
dbname=postgres"
servers diverged at WAL location 0/3000060 on timeline 1
rewinding from last common checkpoint at 0/2000060 on timeline 1
could not remove directory "standby/base/16384": Directory not empty
Failure, exiting
Note: dry run was successful!
[mithuncy@localhost pgrewmasterbin]$ ./bin/pg_rewind -D standby
--source-server="host=127.0.0.1 port=5432 user=mithuncy
dbname=postgres" -n
servers diverged at WAL location 0/3000060 on timeline 1
rewinding from last common checkpoint at 0/2000060 on timeline 1
Done!
Also I have tested same in version 10 it works fine there.
Did below commit has broken this feature? (Thanks to kuntal for
identifying same)
commit 266b6acb312fc440c1c1a2036aa9da94916beac6
Author: Fujii Masao <fujii@postgresql.org>
Date: Thu Mar 29 04:56:52 2018 +0900
Make pg_rewind skip files and directories that are removed during server start.
--
Thanks and Regards
Mithun Chicklore Yogendra
EnterpriseDB: http://www.enterprisedb.com
Attachments:
On Tue, Mar 12, 2019 at 01:23:51PM +0530, Mithun Cy wrote:
I think pg_rewind's feature to rewind the promoted standby as a new
standby is broken in 11
Confirmed, it is.
Also I have tested same in version 10 it works fine there.
Did below commit has broken this feature? (Thanks to kuntal for
identifying same)
commit 266b6acb312fc440c1c1a2036aa9da94916beac6
Author: Fujii Masao <fujii@postgresql.org>
Date: Thu Mar 29 04:56:52 2018 +0900
Make pg_rewind skip files and directories that are removed during server start.
And you are pointing out to the correct commit. The issue is that
process_target_file() has added a call to check_file_excluded(), and
this skips all the folders which it thinks can be skipped. One
problem though is that we also filter out pg_internal.init, which is
present in each database folder, and remains in the target directory
marked for deletion. Then, when the deletion happens, the failure
happens as the directory is not fully empty.
We could consider using rmtree() instead, but it is a nice sanity
check to make sure that all the entries in a path have been marked for
deletion. Just removing the filter check on the target is fine I
think, as we only should have the filters anyway to avoid copying
unnecessary files from the source. Attached is a patch. What do you
think?
(check_file_excluded() could be simplified further but it's also nice
to keep some mirroring in this API if we finish by using it for target
files at some point.)
--
Michael
Attachments:
rewind-filter-fix.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index bb4ffd0e38..a2cd317c72 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -334,12 +334,11 @@ process_target_file(const char *path, file_type_t type, size_t oldsize,
file_entry_t *entry;
/*
- * Ignore any path matching the exclusion filters. This is not actually
- * mandatory for target files, but this does not hurt and let's be
- * consistent with the source processing.
+ * Do not apply any exclusion filters here, the folders marked for
+ * deletion on the target should be entirely empty, and we also
+ * check after pg_internal.init which is present in each database
+ * folder.
*/
- if (check_file_excluded(path, false))
- return;
snprintf(localpath, sizeof(localpath), "%s/%s", datadir_target, path);
if (lstat(localpath, &statbuf) < 0)
On Tue, Mar 12, 2019 at 06:23:01PM +0900, Michael Paquier wrote:
And you are pointing out to the correct commit. The issue is that
process_target_file() has added a call to check_file_excluded(), and
this skips all the folders which it thinks can be skipped. One
problem though is that we also filter out pg_internal.init, which is
present in each database folder, and remains in the target directory
marked for deletion. Then, when the deletion happens, the failure
happens as the directory is not fully empty.
Okay, here is a refined patch with better comments, the addition of a
test case (creating tables in the new databases in 002_databases.pl is
enough to trigger the problem).
Could you check that it fixes the issue on your side?
--
Michael
Attachments:
rewind-filter-fix-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index bb4ffd0e38..57b77e1840 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -334,12 +334,13 @@ process_target_file(const char *path, file_type_t type, size_t oldsize,
file_entry_t *entry;
/*
- * Ignore any path matching the exclusion filters. This is not actually
- * mandatory for target files, but this does not hurt and let's be
- * consistent with the source processing.
+ * Do not apply any exclusion filters here. The folders marked for
+ * deletion on the target should be entirely empty, and we also
+ * check after pg_internal.init which is present in each database
+ * folder. Not applying any exclusion filters here also has the
+ * advantage to remove any files on the target which have been filtered
+ * from the source.
*/
- if (check_file_excluded(path, false))
- return;
snprintf(localpath, sizeof(localpath), "%s/%s", datadir_target, path);
if (lstat(localpath, &statbuf) < 0)
diff --git a/src/bin/pg_rewind/t/002_databases.pl b/src/bin/pg_rewind/t/002_databases.pl
index 6dc05720a1..0562c21549 100644
--- a/src/bin/pg_rewind/t/002_databases.pl
+++ b/src/bin/pg_rewind/t/002_databases.pl
@@ -15,19 +15,27 @@ sub run_test
RewindTest::setup_cluster($test_mode, ['-g']);
RewindTest::start_master();
- # Create a database in master.
+ # Create a database in master with a table.
master_psql('CREATE DATABASE inmaster');
+ master_psql('CREATE TABLE inmaster_tab (a int)', 'inmaster');
RewindTest::create_standby($test_mode);
- # Create another database, the creation is replicated to the standby
+ # Create another database with another table, the creation is
+ # replicated to the standby.
master_psql('CREATE DATABASE beforepromotion');
+ master_psql('CREATE TABLE beforepromotion_tab (a int)',
+ 'beforepromotion');
RewindTest::promote_standby();
# Create databases in the old master and the new promoted standby.
master_psql('CREATE DATABASE master_afterpromotion');
+ master_psql('CREATE TABLE master_promotion_tab (a int)',
+ 'master_afterpromotion');
standby_psql('CREATE DATABASE standby_afterpromotion');
+ standby_psql('CREATE TABLE standby_promotion_tab (a int)',
+ 'standby_afterpromotion');
# The clusters are now diverged.
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 85cae7e47b..2c98fa5712 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -68,18 +68,20 @@ our $node_standby;
sub master_psql
{
my $cmd = shift;
+ my $dbname = shift || 'postgres';
system_or_bail 'psql', '-q', '--no-psqlrc', '-d',
- $node_master->connstr('postgres'), '-c', "$cmd";
+ $node_master->connstr($dbname), '-c', "$cmd";
return;
}
sub standby_psql
{
my $cmd = shift;
+ my $dbname = shift || 'postgres';
system_or_bail 'psql', '-q', '--no-psqlrc', '-d',
- $node_standby->connstr('postgres'), '-c', "$cmd";
+ $node_standby->connstr($dbname), '-c', "$cmd";
return;
}
On Wed, Mar 13, 2019 at 1:38 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Mar 12, 2019 at 06:23:01PM +0900, Michael Paquier wrote:
And you are pointing out to the correct commit. The issue is that
process_target_file() has added a call to check_file_excluded(), and
this skips all the folders which it thinks can be skipped. One
problem though is that we also filter out pg_internal.init, which is
present in each database folder, and remains in the target directory
marked for deletion. Then, when the deletion happens, the failure
happens as the directory is not fully empty.Okay, here is a refined patch with better comments, the addition of a
test case (creating tables in the new databases in 002_databases.pl is
enough to trigger the problem).
I have not looked into the patch but quick test show it has fixed the above
issue.
[mithuncy@localhost pgrewindbin]$ ./bin/pg_rewind -D standby
--source-server="host=127.0.0.1 port=5432 user=mithuncy dbname=postgres" -n
servers diverged at WAL location 0/3000000 on timeline 1
rewinding from last common checkpoint at 0/2000060 on timeline 1
Done!
[mithuncy@localhost pgrewindbin]$ ./bin/pg_rewind -D standby
--source-server="host=127.0.0.1 port=5432 user=mithuncy dbname=postgres"
servers diverged at WAL location 0/3000000 on timeline 1
rewinding from last common checkpoint at 0/2000060 on timeline 1
Done!
--
Thanks and Regards
Mithun Chicklore Yogendra
EnterpriseDB: http://www.enterprisedb.com
On Thu, Mar 14, 2019 at 12:15:57AM +0530, Mithun Cy wrote:
I have not looked into the patch but quick test show it has fixed the above
issue.
Thanks for confirming, Mythun. I'll think about the logic of this
patch for a couple of days in the background, then I'll try to commit
it likely at the beginning of next week.
--
Michael
On Thu, Mar 14, 2019 at 09:04:41AM +0900, Michael Paquier wrote:
Thanks for confirming, Mythun. I'll think about the logic of this
patch for a couple of days in the background, then I'll try to commit
it likely at the beginning of next week.
Committed. I have spent extra time polishing the comments to make the
filtering rules clearer when processing the source and target files,
particularly why they are useful the way they are.
--
Michael