Thread: [HACKERS] [PATCH] Tap test support for backup with tablespace mapping

On Wed, Dec 27, 2017 at 12:58:28PM +1100, Vaishnavi Prabakaran wrote:
> I have added support in Postgres TAP test framework to backup a data
> directory with tablespace mapping. Also added support to move the backup
> directory contents to standby node, because current option to init the
> standby from backup does not support copying softlinks, which is needed
> when tablespace mapping is involved in backup.
>
> Added a new test to existing streaming replication tap test to demonstrate
> the usage of these new APIs.
>
> Attached the patch, Hope this enhancement is useful.


+sub backup_withtablespace
+{
+        my ($self, $backup_name, $old_tablespacedir, $new_tablespacedir) = @_;
+        my $backup_path = $self->backup_dir . '/' . $backup_name;
+        my $port        = $self->port;
+        my $name        = $self->name;
+
+        print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
+        TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p', $port,
+                '--no-sync','-T',"$old_tablespacedir=$new_tablespacedir");
+        print "# Backup with tablespace option finished\n";
+}
Let's not reinvent the wheel and instead let's extend the existing
PostgresNode::backup with a new option set. Please be careful that this
new option needs to be able to handle multiple sets of old/new
directories as pg_basebackup is able to handle that as well.

+=cut
+
+sub convert_backup_to_standby
+{
This is a copy/paste of the existing PostgresNode::init_from_backup,
except that you remove forcibly a backup. I don't understand what you
can actually win from that. A new routine which *removes* a past backup
could have some value, still those are automatically wiped out at the
end of the tests if the person who designed the test is smart enough to
use the default paths.

+$node_master->safe_psql('postgres',
+        "CREATE TABLESPACE testtblspc location '$master_tablespacedir';");
+$node_master->safe_psql('postgres',
+        "CREATE TABLE test_table(id integer,name varchar(20)) TABLESPACE testtblspc;");
Those tests overlap with the existing 010_pg_basebackup.pl.
--
Michael

Dear Vaishnavi,

On Tue, December 26, 2017 8:58 pm, Vaishnavi Prabakaran wrote:
> Hi All,
>
> I have added support in Postgres TAP test framework to backup a data
> directory with tablespace mapping. Also added support to move the backup
> directory contents to standby node, because current option to init the
> standby from backup does not support copying softlinks, which is needed
> when tablespace mapping is involved in backup.
>
> Added a new test to existing streaming replication tap test to demonstrate
> the usage of these new APIs.
>
> Attached the patch, Hope this enhancement is useful.
>
> Thanks & Regards,
> Vaishnavi,
> Fujitsu Australia.
>

Thank you for the path, I saw these things:

* backup_withtablespace() does not have any documentation?

* The mkdir calls do not set a mask for the created dir, defaulting to
0777 - is this what is wanted here?

* none of the mkdir, chdir etc. calls check any error code, e.g. what
happens if one of them fails?

* different indentation between rmdir and move (tab vs. spaces):

+        rmdir($data_path);
+        move("$backup_path", "$self->{_basedir}/pgdata")

Best regards,

Tels