Re: pg_basebackup and replication slots

On Wed, Jul 1, 2015 at 11:35 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 7/1/15 8:37 AM, Michael Paquier wrote:
>> On Wed, Jul 1, 2015 at 10:33 AM, Peter Eisentraut wrote:
>>> (If you're looking at the patch and wondering why there is no code to
>>> actually do anything with the replication slot, that's because the code
>>> that does the WAL streaming is already aware of replication slots
>>> because of the pg_receivexlog functionality that it also serves.  So all
>>> that needs to be done is set replication_slot.)
>>
>> This is cool to see this patch taking shape.
>>
>> +    my ($stdout, $stderr);
>> +    run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-' ],
>> '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
>> +    chomp $stdout;
>> +    return $stdout;
>>
>> Could it be possible to chomp and return $stderr as well here? It
>> seems useful to me to perform sanity checks after calling psql.
>
> Sure, makes sense.

OK, so here is more input about this set of patches.

Patch 1 looks good. It adds some tests to cover pg_basebackup -R and
checks if standby_mode and primary_conninfo are set correctly.
Patch 2 also looks fine. It adds tests for pg_basebackup -X. Both
patches are independent on the feature.

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.
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.
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.
Regards,
-- 
Michael