Re: Fwd: Add tablespace tap test to pg_rewind
On Thu, May 09, 2019 at 02:36:48PM +0900, Michael Paquier wrote: > Yes, I can see that. The issue is that even if we do a backup with > --tablespace-mapping then we still need a tweak to allow the copy of > symlinks. I am not sure that this is completely what we are looking > for either, as it means that any test setting a primary with a > tablespace and two standbys initialized from the same base backup > would fail. That's not really portable. So for now I am marking the patch as returned with feedback. I can see a simple way to put us through that by having an equivalent of --tablespace-mapping for the file-level backup routine which passes it down to RecursiveCopy.pm as well as PostgresNode::init_from_backup. At the end of the day, we would need to be able to point the path to different locations for: - the primary - any backup tables - any standbys which are restored from the previous backups. And on top of that there is of course the argument of pg_rewind which would need an option similar to pg_basebackup's --tablespace-mapping so as a target instance does not finish by using the same tablespace path as the source where there is a tablespace of difference during the operation. And it does not prevent an overlap if a tablespace needs to be created when the target instance replays WAL up to the consistent point of the source. So that's a lot of work which may be hard to justify. -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
On Fri, Mar 22, 2019 at 04:25:53PM +0800, Shaoqi Bai wrote: > Deleted the test for group permissions in updated patch. Well, there are a couple of things I am not really happy about in this patch: - There is not much point to have has_tablespace_mapping as it is not extensible. Instead I'd rather use a single "extra" parameter which can define a range of options. An example of that is within PostgresNode::init for "extra" and "auth_extra". - CREATE TABLESPACE is run once on the primary *before* promoting the standby, which causes the tablespace paths to map between both of them. This is not correct. Creating a tablespace on the primary before creating the standby, and use --tablespace-map would be the way to go... However per the next point... - standby_afterpromotion is created on the promoted standby, and then immediately dropped. pg_rewind is able to handle this case when working on different hosts. But with this test we finish by having the same problem as pg_basebackup: the source and the target server finish by eating each other. I think that this could actually be an interesting feature for pg_rewind. - A comment at the end refers to databases, and not tablespaces. You could work out the first problem with the backup by changing the backup()/init_from_backup() in RewindTest::create_standby by a pure call to pg_basebackup, but you still have the second problem, which we should still be able to test, and this requires more facility in pg_rewind so as it is basically possible to hijack create_target_symlink() to create a symlink to a different path than the initial one. > Checking the RecursiveCopy::copypath being called, only _backup_fs and > init_from_backup called it. > After runing cmd make -C src/bin check in updated patch, seeing no failure. Yes, I can see that. The issue is that even if we do a backup with --tablespace-mapping then we still need a tweak to allow the copy of symlinks. I am not sure that this is completely what we are looking for either, as it means that any test setting a primary with a tablespace and two standbys initialized from the same base backup would fail. That's not really portable. -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
On Fri, Apr 26, 2019 at 11:21:48AM -0400, Tom Lane wrote: > I don't think feature freeze precludes adding new test cases. I think as well that adding this stuff into v12 would be fine. Now if there is any objection let's wait for later. -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
Alvaro Herrera writes: > On 2019-Apr-26, Michael Paquier wrote: >> On Fri, Apr 26, 2019 at 10:11:24AM -0400, Alvaro Herrera wrote: >>> I just noticed this thread. What do we think of adding this test to >>> pg12? (The patch doesn't apply verbatim, but it's a small update to get >>> it to apply.) >> Could you let me have a look at it? > Absolutely. I was just trying to get a sense of how frozen the water is > at this point. I don't think feature freeze precludes adding new test cases. regards, tom lane
Re: Fwd: Add tablespace tap test to pg_rewind
On 2019-Apr-26, Michael Paquier wrote: > On Fri, Apr 26, 2019 at 10:11:24AM -0400, Alvaro Herrera wrote: > > I just noticed this thread. What do we think of adding this test to > > pg12? (The patch doesn't apply verbatim, but it's a small update to get > > it to apply.) > > Could you let me have a look at it? Absolutely. I was just trying to get a sense of how frozen the water is at this point. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Fwd: Add tablespace tap test to pg_rewind
On Fri, Apr 26, 2019 at 10:11:24AM -0400, Alvaro Herrera wrote: > I just noticed this thread. What do we think of adding this test to > pg12? (The patch doesn't apply verbatim, but it's a small update to get > it to apply.) Could you let me have a look at it? I have not tested on Windows, but I suspect that because of the symlink() part this would fail, so we may need to skip the tests. -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
I just noticed this thread. What do we think of adding this test to pg12? (The patch doesn't apply verbatim, but it's a small update to get it to apply.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Fwd: Add tablespace tap test to pg_rewind
On Fri, Mar 22, 2019 at 1:34 PM Michael Paquier wrote: > On Thu, Mar 21, 2019 at 11:41:01PM +0800, Shaoqi Bai wrote: > > Have updated the patch doing as you suggested > > + RewindTest::setup_cluster($test_mode, ['-g']); > + RewindTest::start_master(); > > There is no need to test for group permissions here, 002_databases.pl > already looks after that. > > Deleted the test for group permissions in updated patch. > + # Check for symlink -- needed only on source dir, only allow symlink > + # when under pg_tblspc > + # (note: this will fall through quietly if file is already gone) > + if (-l $srcpath) > So you need that but in RecursiveCopy.pm because of init_from_backup > when creating the standby, which makes sense when it comes to > pg_tblspc. I am wondering about any side effects though, and if it > would make sense to just remove the restriction for symlinks in > _copypath_recurse(). > -- > Michael > Checking the RecursiveCopy::copypath being called, only _backup_fs and init_from_backup called it. After runing cmd make -C src/bin check in updated patch, seeing no failure. 0002-Add-new-test-with-integration-in-RewindTest.pm.patch Description: Binary data 0001-Refactors-the-code-for-the-new-option-in-PostgresNod.patch Description: Binary data
Re: Fwd: Add tablespace tap test to pg_rewind
On Thu, Mar 21, 2019 at 11:41:01PM +0800, Shaoqi Bai wrote: > Have updated the patch doing as you suggested + RewindTest::setup_cluster($test_mode, ['-g']); + RewindTest::start_master(); There is no need to test for group permissions here, 002_databases.pl already looks after that. + # Check for symlink -- needed only on source dir, only allow symlink + # when under pg_tblspc + # (note: this will fall through quietly if file is already gone) + if (-l $srcpath) So you need that but in RecursiveCopy.pm because of init_from_backup when creating the standby, which makes sense when it comes to pg_tblspc. I am wondering about any side effects though, and if it would make sense to just remove the restriction for symlinks in _copypath_recurse(). -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
On Tue, Mar 12, 2019 at 10:27 AM Michael Paquier wrote: > It could be an idea to split the patch in two pieces: > - One patch which refactors the code for the new option in > PostgresNode.pm > - Second patch for the new test with integration in RewindTest.pm. > This should touch different parts of the code, so combining both would > be fine as well for me :) > -- > Michael > Have updated the patch doing as you suggested On Wed, Mar 20, 2019 at 7:45 AM Michael Paquier wrote: > I would have avoided > extra routines in the patch, like what you have done with > create_standby_tbl_mapping(), but instead do something like > init_from_backup() which is able to take extra parameters in a way > similar to has_streaming and has_restoring. However, the trick with > tablespace mapping is that the caller of backup() should be able to > pass down multiple tablespace mapping references to make that a > maximum portable. > -- > Michael Also updated the patch to achieve your suggestion. 0001-Refactors-the-code-for-the-new-option-in-PostgresNod.patch Description: Binary data 0002-Add-new-test-with-integration-in-RewindTest.pm.patch Description: Binary data
Re: Fwd: Add tablespace tap test to pg_rewind
On Tue, Mar 19, 2019 at 08:16:21PM +0800, Shaoqi Bai wrote: > Thanks for your advice, sorry for taking so long to give update in the > thread, because I am stuck in modifing Perl script, knowing little about > Perl language. No problem. It is true that using perl for the first time can be a certain gap, but once you get used to it it becomes really nice to be able to control how tests are run in the tree. I would have avoided extra routines in the patch, like what you have done with create_standby_tbl_mapping(), but instead do something like init_from_backup() which is able to take extra parameters in a way similar to has_streaming and has_restoring. However, the trick with tablespace mapping is that the caller of backup() should be able to pass down multiple tablespace mapping references to make that a maximum portable. -- Michael signature.asc Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
On Tue, Mar 12, 2019 at 10:27 AM Michael Paquier
wrote:
> On Mon, Mar 11, 2019 at 07:49:11PM +0800, Shaoqi Bai wrote:
> > Thanks, will work on it as you suggested
> > Add pg_basebackup --T olddir=newdir to support check the consistency of a
> > tablespace created before promotion
> > Add run_test('remote');
>
> Thanks for considering my input. Why don't you register your patch to
> the next commit fest then so as it goes through a formal review once
> you are able to provide a new version? The commit fest is here:
> https://commitfest.postgresql.org/23/
>
> We are currently in the process of wrapping up the last commit fest
> for v12, so this stuff will have to wait a bit :(
>
> It could be an idea to split the patch in two pieces:
> - One patch which refactors the code for the new option in
> PostgresNode.pm
> - Second patch for the new test with integration in RewindTest.pm.
> This should touch different parts of the code, so combining both would
> be fine as well for me :)
>
Thanks for your advice, sorry for taking so long to give update in the
thread, because I am stuck in modifing Perl script, knowing little about
Perl language.
I tried to do the following two things in this patch.
1. add pg_basebackup --T olddir=newdir to support check the consistency of
a tablespace created before promotion
2. run both run_test('local') and run_test('remote');
The code can pass make installcheck under src/bin/pg_rewind, but can not
pass make installcheck under src/bin/pg_basebackup.
Because the patch refactor is not done well.
The patch still need refactor, to make other tests pass, like tests under
src/bin/pg_basebackup.
Sending the letter is just to let you know the little progress on the
thread.
0001-Add-tablespace-tap-test-for-pg_rewind.patch
Description: Binary data
Re: Fwd: Add tablespace tap test to pg_rewind
On Mon, Mar 11, 2019 at 07:49:11PM +0800, Shaoqi Bai wrote:
> Thanks, will work on it as you suggested
> Add pg_basebackup --T olddir=newdir to support check the consistency of a
> tablespace created before promotion
> Add run_test('remote');
Thanks for considering my input. Why don't you register your patch to
the next commit fest then so as it goes through a formal review once
you are able to provide a new version? The commit fest is here:
https://commitfest.postgresql.org/23/
We are currently in the process of wrapping up the last commit fest
for v12, so this stuff will have to wait a bit :(
It could be an idea to split the patch in two pieces:
- One patch which refactors the code for the new option in
PostgresNode.pm
- Second patch for the new test with integration in RewindTest.pm.
This should touch different parts of the code, so combining both would
be fine as well for me :)
--
Michael
signature.asc
Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
Thanks, will work on it as you suggested
Add pg_basebackup --T olddir=newdir to support check the consistency of a
tablespace created before promotion
Add run_test('remote');
On Mon, Mar 11, 2019 at 6:50 AM Michael Paquier wrote:
> On Sat, Mar 09, 2019 at 09:09:24AM +0900, Michael Paquier wrote:
> > When working on the first version of pg_rewind for VMware with Heikki,
> > tablespace support has been added only after, so what you propose is
> > sensible I think.
> >
> > +# Run the test in both modes.
> > +run_test('local');
> > Small nit: we should test for the remote mode here as well.
>
> I got to think more about this one a bit more, and I think that it
> would be good to also check the consistency of a tablespace created
> before promotion. If you copy the logic from 002_databases.pl, this
> is not going to work because the primary and the standby would try to
> create the tablespace in the same path, stepping on each other's
> toes. So what you need to do is to create the tablespace on the
> primary because creating the standby. This requires a bit more work
> than what you propose here though as you basically need to extend
> RewindTest::create_standby so as it is possible to pass extra
> arguments to $node_master->backup. And in this case the extra option
> to use is --tablespace-mapping to make sure that the primary and the
> standby have the same tablespace, but defined on different paths.
> --
> Michael
>
Re: Fwd: Add tablespace tap test to pg_rewind
On Sat, Mar 09, 2019 at 09:09:24AM +0900, Michael Paquier wrote:
> When working on the first version of pg_rewind for VMware with Heikki,
> tablespace support has been added only after, so what you propose is
> sensible I think.
>
> +# Run the test in both modes.
> +run_test('local');
> Small nit: we should test for the remote mode here as well.
I got to think more about this one a bit more, and I think that it
would be good to also check the consistency of a tablespace created
before promotion. If you copy the logic from 002_databases.pl, this
is not going to work because the primary and the standby would try to
create the tablespace in the same path, stepping on each other's
toes. So what you need to do is to create the tablespace on the
primary because creating the standby. This requires a bit more work
than what you propose here though as you basically need to extend
RewindTest::create_standby so as it is possible to pass extra
arguments to $node_master->backup. And in this case the extra option
to use is --tablespace-mapping to make sure that the primary and the
standby have the same tablespace, but defined on different paths.
--
Michael
signature.asc
Description: PGP signature
Re: Fwd: Add tablespace tap test to pg_rewind
On Fri, Mar 08, 2019 at 09:42:29PM +0800, Shaoqi Bai wrote:
> There is already a databases tap tests in pg_rewind, wonder if there is a
> need for tablespace tap tests in pg_rewind. Attached is a initial
> patch from me.
When working on the first version of pg_rewind for VMware with Heikki,
tablespace support has been added only after, so what you propose is
sensible I think.
+# Run the test in both modes.
+run_test('local');
Small nit: we should test for the remote mode here as well.
--
Michael
signature.asc
Description: PGP signature
Fwd: Add tablespace tap test to pg_rewind
Hi hackers,
There is already a databases tap tests in pg_rewind, wonder if there is a
need for tablespace tap tests in pg_rewind.
Attached is a initial patch from me.
Here is a patch for runing pg_rewind, it is very similar to
src/bin/pg_rewind/t/002_databases.pl, but there is no master_psql("CREATE
TABLESPACE beforepromotion LOCATION '$tempdir/beforepromotion'"); after
create_standby() and before promote_standby(), because pg_rewind will error
out :
could not create directory
"/Users/sbai/work/postgres/src/bin/pg_rewind/tmp_check/t_006_tablespace_master_local_data/pgdata/pg_tblspc/24576/PG_12_201903063":
File exists
Failure, exiting
The patch is created on top of the
commit e1e0e8d58c5c70da92e36cb9d59c2f7ecf839e00 (origin/master, origin/HEAD)
Author: Michael Paquier
Date: Fri Mar 8 15:10:14 2019 +0900
Fix function signatures of pageinspect in documentation
tuple_data_split() lacked the type of the first argument, and
heap_page_item_attrs() has reversed the first and second argument,
with the bytea argument using an incorrect name.
Author: Laurenz Albe
Discussion:
https://postgr.es/m/8f9ab7b16daf623e87eeef5203a4ffc0dece8dfd.ca...@cybertec.at
Backpatch-through: 9.6
From 4280335c4b81aa04b967d93348ccc6874bc9c287 Mon Sep 17 00:00:00 2001
From: Shaoqi Bai
Date: Fri, 8 Mar 2019 18:04:20 +0800
Subject: [PATCH] Add tablespace tap test for pg_rewind
---
src/bin/pg_rewind/t/006_tablespace.pl | 51 +++
1 file changed, 51 insertions(+)
create mode 100644 src/bin/pg_rewind/t/006_tablespace.pl
diff --git a/src/bin/pg_rewind/t/006_tablespace.pl b/src/bin/pg_rewind/t/006_tablespace.pl
new file mode 100644
index 00..094041dd89
--- /dev/null
+++ b/src/bin/pg_rewind/t/006_tablespace.pl
@@ -0,0 +1,51 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests => 2;
+
+use FindBin;
+use lib $FindBin::RealBin;
+
+use RewindTest;
+
+my $tempdir = TestLib::tempdir;
+
+sub run_test
+{
+ my $test_mode = shift;
+
+ RewindTest::setup_cluster($test_mode, ['-g']);
+ RewindTest::start_master();
+
+ RewindTest::create_standby($test_mode);
+
+ RewindTest::promote_standby();
+
+ mkdir "$tempdir/master_afterpromotion";
+ mkdir "$tempdir/standby_afterpromotion";
+
+ # Create tablespaces in the old master and the new promoted standby.
+ master_psql("CREATE TABLESPACE master_afterpromotion LOCATION '$tempdir/master_afterpromotion'");
+ standby_psql("CREATE TABLESPACE standby_afterpromotion LOCATION '$tempdir/standby_afterpromotion'");
+
+ # The clusters are now diverged.
+
+ RewindTest::run_pg_rewind($test_mode);
+
+ # Check that the correct databases are present after pg_rewind.
+ check_query(
+ 'SELECT spcname FROM pg_tablespace ORDER BY spcname',
+ qq(pg_default
+pg_global
+standby_afterpromotion
+),
+ 'tablespace names');
+
+ RewindTest::clean_rewind_test();
+ return;
+}
+
+# Run the test in both modes.
+run_test('local');
+
+exit(0);
--
2.19.1
