Обсуждение: 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

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

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

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

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

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

At Sat, 27 Apr 2019 09:35:19 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190427003519.GC2032@paquier.xyz>
> 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.

The patch seems to be using the tablespace directory in backups
directly from standbys. In other words, multiple standbys created
from A backup shares the tablespace directory in the backup.

Another nitpicking is it sound a bit strainge that the parameter
"has_tablespace_mapping" is not a boolean.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

On Tue, May 07, 2019 at 10:31:59AM +0900, Kyotaro HORIGUCHI wrote:
> The patch seems to be using the tablespace directory in backups
> directly from standbys. In other words, multiple standbys created
> from A backup shares the tablespace directory in the backup.

Yes, I noticed that, and I am not happy about that either.  I'd like
to think that what we are looking for is an equivalent of the
tablespace mapping of pg_basebackup, but for init_from_backup().  At
least that sounds like a plausible solution.
--
Michael

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