Antonin Houska escribió:
> > Why did you choose "bytes per second" as a valid rate which we can specify?
> > Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
> > If we do that, we can easily increase the maximum rate from 1GB to very large
> > number in the future if required.
>
> The attached version addresses all the comments above.
I pushed this patch with a few further tweaks. In your changes to
address the above point, you made the suffix mandatory in the
pg_basebackup -r option. This seemed a strange restriction, so I
removed it. It seems more user-friendly to me to accept the value as
being expressed in kilobytes per second without requiring the suffix to
be there; the 'k' suffix is then also accepted and has no effect. I
amended the docs to say that also.
If you or others feel strongly about this, we can still tweak it, of
course.
I also moved the min/max #defines to replication/basebackup.h, and
included that file in pg_basebackup.c. This avoids the duplicated
values. That file is okay to be included there.
> > If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
> > other process does? This is not a problem of this patch. This problem exists
> > also in current master. But ISTM it's better to solve that together. Thought?
>
> Once we're careful about not missing signals, I think PM death should be
> noticed too. The backup functionality itself would probably manage to
> finish without postmaster, however it's executed under walsender process.
>
> Question is where !PostmasterIsAlive() check should be added. I think it
> should go to the main loop of perform_base_backup(), but that's probably
> not in the scope of this patch.
Feel free to submit patches about this.
Thanks for your patch, and the numerous reviewers who took part.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
