On Sun, Feb 20, 2011 at 21:37, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Hi,
>
> Magnus Hagander <magnus@hagander.net> writes:
>> Better late than never (or?), here's the final cleanup of
>> pg_streamrecv for moving into the main distribution, per discussion
>> back in late dec or early jan. It also includes the "stream logs in
>> parallel to backup" part that was not completed on pg_basebackup.
>
> And that's something I've been so interested in! It's only fair game
> that I spend time reviewing after my insisting for having it :)
>
> The programs (pg_basebackup and pg_receivexlog) both work as expected,
> and show in the pg_stat_replication system view.
>
> pg_basebackup -x option should be revised so that it's easier to make
> the difference between streaming WAL while the base backup is ongoing
> and fetching them at the end, taking the risk to corrupt the whole
> backup as soon as wal_keep_segments is undersized.
>
> -x, --xlog[=stream] include required WAL files in backup
>
> It could be --xlog=stream|fetch or something that reads better.
Yeha, that's probably true. I wanted to avoid making it mandatory, but
it's actually easier this way. Will change it to that.
> Now, on to the code itself.
>
> I wonder if the segment_callback() routine would better be a while{}
> loop rather than a recursive construct. Also, it looks like a lib
> function but it's doing exit(1)…
Actually, it's even better to just reorder the checks in the other
order - that way we don't need a loop *or* a self-call.
> Unfortunately I can't comment (or won't risk learning enough details
> tonight to try to be smart here) on FindStreamingStart() implementation,
> that seems crucial.
It is - so if you can find the time to, that would be great...
> Will the server refrain from recycling a WAL file when all receivers
> sent_location are not known to be past the positions contained in it?
> If that's the case, the documentation should talk about pg_receivexlog
> as an alternative to archiving, relying on libpq. It that's not the
> case, is there a good reason for that not being the case? (even if
> that's not on this patch to fix that).
No, not at this point. It would be nice to have that option in the future...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/