On Wed, Mar 30, 2022 at 3:08 AM Robert Haas <robertmh...@gmail.com> wrote:
> On Fri, Mar 25, 2022 at 5:52 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> > On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro <thomas.mu...@gmail.com> wrote:
> > This line doesn't look too healthy:
> >
> > pg_basebackup: error: backup failed: ERROR: shell command "type con >
> > "C:cirruscontrib asebackup_to_shell mp_check mp_test_tch3\base.tar""
> > failed
> >
> > I guess it's an escaping problem around \ characters.
>
> Oh, right. I didn't copy the usual incantation as completely as I
> should have done.
>
> Here's a new version, hopefully rectifying that deficiency. I also add
> a second patch here, documenting basebackup_to_shell.required_role,
> because Joe Conway observed elsewhere that I forgot to do that.
Here are your patches again, plus that kludge to make the CI run your
TAP test on Windows.
From de0eca033142d1fde643e503e5409887f38a628b Mon Sep 17 00:00:00 2001
From: Robert Haas <rh...@postgresql.org>
Date: Tue, 29 Mar 2022 10:05:45 -0400
Subject: [PATCH v2 1/2] basebackup_to_shell: Add TAP test.
Per gripe from Andres Freund.
---
contrib/basebackup_to_shell/Makefile | 4 +
contrib/basebackup_to_shell/t/001_basic.pl | 114 +++++++++++++++++++++
2 files changed, 118 insertions(+)
create mode 100644 contrib/basebackup_to_shell/t/001_basic.pl
diff --git a/contrib/basebackup_to_shell/Makefile b/contrib/basebackup_to_shell/Makefile
index f31dfaae9c..bbfebcc9a1 100644
--- a/contrib/basebackup_to_shell/Makefile
+++ b/contrib/basebackup_to_shell/Makefile
@@ -7,6 +7,10 @@ OBJS = \
PGFILEDESC = "basebackup_to_shell - target basebackup to shell command"
+TAP_TESTS = 1
+
+export TAR
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/basebackup_to_shell/t/001_basic.pl b/contrib/basebackup_to_shell/t/001_basic.pl
new file mode 100644
index 0000000000..df0f6d9926
--- /dev/null
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -0,0 +1,114 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init('allows_streaming' => 1);
+$node->append_conf('postgresql.conf',
+ "shared_preload_libraries = 'basebackup_to_shell'");
+$node->start;
+$node->safe_psql('postgres', 'CREATE USER backupuser REPLICATION');
+$node->safe_psql('postgres', 'CREATE ROLE trustworthy');
+
+# For nearly all pg_basebackup invocations some options should be specified,
+# to keep test times reasonable. Using @pg_basebackup_defs as the first
+# element of the array passed to IPC::Run interpolate the array (as it is
+# not a reference to an array)...
+my @pg_basebackup_defs = ('pg_basebackup', '--no-sync', '-cfast');
+
+# This particular test module generally wants to run with -Xfetch, because
+# -Xstream is not supported with a backup target, and with -U backupuser.
+my @pg_basebackup_cmd = (@pg_basebackup_defs, '-U', 'backupuser', '-Xfetch');
+
+# Can't use this module without setting basebackup_to_shell.command.
+$node->command_fails_like(
+ [ @pg_basebackup_cmd, '--target', 'shell' ],
+ qr/shell command for backup is not configured/,
+ 'fails if basebackup_to_shell.command is not set');
+
+# Configure basebackup_to_shell.command and reload the configuation file.
+my $backup_path = PostgreSQL::Test::Utils::tempdir;
+my $escaped_backup_path = $backup_path;
+$escaped_backup_path =~ s{\\}{\\\\}g if ($PostgreSQL::Test::Utils::windows_os);
+my $shell_command =
+ $PostgreSQL::Test::Utils::windows_os
+ ? qq{type con > "$escaped_backup_path\\\\%f"}
+ : qq{cat > "$escaped_backup_path/%f"};
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.command='$shell_command'");
+$node->reload();
+
+# Should work now.
+$node->command_ok(
+ [ @pg_basebackup_cmd, '--target', 'shell' ],
+ 'backup with no detail: pg_basebackup');
+verify_backup('', $backup_path, "backup with no detail");
+
+# Should fail with a detail.
+$node->command_fails_like(
+ [ @pg_basebackup_cmd, '--target', 'shell:foo' ],
+ qr/a target detail is not permitted because the configured command does not include %d/,
+ 'fails if detail provided without %d');
+
+# Reconfigure to restrict access and require a detail.
+$shell_command =
+ $PostgreSQL::Test::Utils::windows_os
+ ? qq{type con > "$escaped_backup_path\\\\%d.%f"}
+ : qq{cat > "$escaped_backup_path/%d.%f"};
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.command='$shell_command'");
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.required_role='trustworthy'");
+$node->reload();
+
+# Should fail due to lack of permission.
+$node->command_fails_like(
+ [ @pg_basebackup_cmd, '--target', 'shell' ],
+ qr/permission denied to use basebackup_to_shell/,
+ 'fails if required_role not granted');
+
+# Should fail due to lack of a detail.
+$node->safe_psql('postgres', 'GRANT trustworthy TO backupuser');
+$node->command_fails_like(
+ [ @pg_basebackup_cmd, '--target', 'shell' ],
+ qr/a target detail is required because the configured command includes %d/,
+ 'fails if %d is present and detail not given');
+
+# Should work.
+$node->command_ok(
+ [ @pg_basebackup_cmd, '--target', 'shell:bar' ],
+ 'backup with detail: pg_basebackup');
+verify_backup('bar.', $backup_path, "backup with detail");
+
+done_testing();
+
+sub verify_backup
+{
+ my ($prefix, $backup_dir, $test_name) = @_;
+
+ ok(-f "$backup_dir/${prefix}backup_manifest",
+ "$test_name: backup_manifest was created");
+ ok(-f "$backup_dir/${prefix}base.tar",
+ "$test_name: base.tar was created");
+
+ SKIP: {
+ my $tar = $ENV{TAR};
+ skip "no tar program available", 1 if (!defined $tar || $tar eq '');
+
+ # Untar.
+ my $extract_path = PostgreSQL::Test::Utils::tempdir;
+ system_or_bail($tar, 'xf', $backup_dir . '/' . $prefix . 'base.tar',
+ '-C', $extract_path);
+
+ # Verify.
+ $node->command_ok([ 'pg_verifybackup', '-n',
+ '-m', "${backup_dir}/${prefix}backup_manifest",
+ '-e', $extract_path ],
+ "$test_name: backup verifies ok");
+ }
+}
--
2.24.3 (Apple Git-128)
From 1819340def47fe7f207c79373390fca8259b9132 Mon Sep 17 00:00:00 2001
From: Robert Haas <rh...@postgresql.org>
Date: Tue, 29 Mar 2022 10:06:07 -0400
Subject: [PATCH v2 2/2] Document basebackup_to_shell.required_role.
Omission noted by Joe Conway.
---
doc/src/sgml/basebackup-to-shell.sgml | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/doc/src/sgml/basebackup-to-shell.sgml b/doc/src/sgml/basebackup-to-shell.sgml
index f36f37e510..9f44071d50 100644
--- a/doc/src/sgml/basebackup-to-shell.sgml
+++ b/doc/src/sgml/basebackup-to-shell.sgml
@@ -55,6 +55,23 @@
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term>
+ <varname>basebackup_to_shell.required_role</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>basebackup_to_shell.required_role</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ A role which replication whose privileges users are required to possess
+ in order to make use of the <literal>shell</literal> backup target.
+ If this is not set, any replication user may make use of the
+ <literal>shell</literal> backup target.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</sect2>
--
2.24.3 (Apple Git-128)
From cc3c9af5c3868a5ce783338290c5cd4e56880a46 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 26 Mar 2022 09:35:06 +1300
Subject: [PATCH 2/2] HACK: run tap tests in contrib/basebackup_to_shell
---
.cirrus.yml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index 171bd29cf0..edbb8b2966 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -412,6 +412,8 @@ task:
test_regress_parallel_script: |
%T_C% perl src/tools/msvc/vcregress.pl check parallel
+ test_contrib_basebackup_to_shell_script: |
+ %T_C% perl src/tools/msvc/vcregress.pl taptest ./contrib/basebackup_to_shell
startcreate_script: |
rem paths to binaries need backslashes
tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l tmp_check/initdb.log --options=--no-sync
--
2.30.2
