Re: pg_basebackup and replication slots

On Wed, Jul 22, 2015 at 10:15 AM, Peter Eisentraut wrote:
> On 7/2/15 3:37 AM, Michael Paquier wrote:
>> 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.

OK.

>> 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.

OK, thanks for the updated versions. Those ones look good to me.

Now, do we plan to do something about the creation of a slot. I
imagine that it would be useful if we could have --create-slot to
create a slot when beginning a base backup and if-not-exists to
control the error flow. There is already CreateReplicationSlot for
this purpose in streamutils.c so most of the work is already done.
Also, when a base backup fails for a reason or another, we should try
to drop the slot in disconnect_and_exit() if it has been created by
pg_basebackup. if if-not-exists is true and the slot already existed
when beginning, we had better not dropping it perhaps...
--
Michael