Re: pg_basebackup and replication slots

On 7/2/15 3:37 AM, Michael Paquier wrote:
> Regarding patch 3, I have more comments:
> 1) I think that documentation should clearly mention that if -R and -S
> are used together, a primary_slot_name entry is added in the
> recovery.conf generated with the slot name defined.

Updated proposal attached.

> 2)
>  sub psql
>  {
>         my ($dbname, $sql) = @_;
> -       run [ 'psql', '-X', '-q', '-d', $dbname, '-f', '-' ], '<', \$sql or die;
> +       my ($stdout, $stderr);
> +       run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-'
> ], '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
> +       chomp $stdout;
> +       return $stdout;
>  }
> RewindTest.pm has a routine called check_query defined. I would be
> great to extend a bit more psql than what I thought previously by
> returning from it ($result, $stdout, $strerr) and make check_query
> directly use it. This way we have a unique interface to run psql and
> capture output. I don't mind writing this refactoring patch on top of
> your set if that's necessary.

Let's do that as a separate patch.  Adding multiple return values makes
calling awkward, so I'd like to sort out the actual use cases before we
do that.

> 3)
> +command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxs_sl_R", '-X',
> 'stream', '-S', 'slot1', '-R' ],
> +       'pg_basebackup with replication slot and -R runs');
> +$recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
> +like(slurp_file("$tempdir/backupxs_sl_R/recovery.conf"),
> slurp_file is slurping an incorrect file and $recovery_conf is used
> nowhere after, so you could simply remove this line.

Yeah, that was some leftover garbage.