On Wed, Mar 30, 2022 at 3:08 AM Robert Haas <[email protected]> wrote: > On Fri, Mar 25, 2022 at 5:52 PM Thomas Munro <[email protected]> wrote: > > On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
