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
