Hi Venkata,

On 2/28/17 11:59 PM, Venkata B Nagothi wrote:
On Wed, Mar 1, 2017 at 1:14 AM, Venkata B Nagothi <nag1...@gmail.com
<mailto:nag1...@gmail.com>> wrote:
    On Tue, Jan 31, 2017 at 6:49 AM, David Steele <da...@pgmasters.net
    <mailto:da...@pgmasters.net>> wrote:

        Do you know when those will be ready?

    Attached are both the patches with tests included.

Thanks for adding the tests.  They bring clarity to the patch.

Unfortunately, I don't think the first patch (recoveryStartPoint) will work as currently implemented. The problem I see is that the new function recoveryStartsHere() depends on pg_control containing a checkpoint right at the end of the backup. There's no guarantee that this is true, even if pg_control is copied last. That means a time, lsn, or xid that occurs somewhere in the middle of the backup can be selected without complaint from this code depending on timing.

The tests pass (or rather fail as expected) because they are written using values before the start of the backup. It's actually the end of the backup (where the database becomes consistent on recovery) that defines where PITR can start and this distinction definitely matters for very long backups. A better test would be to start the backup, get a time/lsn/xid after writing some data, and then make sure that time/lsn/xid is flagged as invalid on recovery.

It is also problematic to assume that transaction IDs commit in order. If checkPoint.latestCompletedXid contains 5 then a recovery to xid 4 may or may not be successful depending on the commit order. However, it appears in this case the patch would disallow recovery to 4.

I think this logic would need to be baked into recoveryStopsAfter() and/or recoveryStopsBefore() in order to work. It's not clear to me what that would look like, however, or if the xid check is even practical.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to