Re: patch proposal

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 <nag1010(at)gmail(dot)com
> <mailto:nag1010(at)gmail(dot)com>> wrote:
> On Tue, Jan 31, 2017 at 6:49 AM, David Steele <david(at)pgmasters(dot)net
> <mailto:david(at)pgmasters(dot)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.

Regards,
--
-David
david(at)pgmasters(dot)net