Re: patch proposal

On Wed, Mar 1, 2017 at 1:14 AM, Venkata B Nagothi <nag1010(at)gmail(dot)com> wrote:

> Hi David,
>
> On Tue, Jan 31, 2017 at 6:49 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>
>> On 1/27/17 3:19 AM, Venkata B Nagothi wrote:
>>
>> > I will be adding the tests in
>> > src/test/recovery/t/003_recovery_targets.pl
>> > <http://003_recovery_targets.pl>. My tests would look more or less
>> > similar to recovery_target_action. I do not see any of the tests added
>> > for the parameter recovery_target_action ? Anyways, i will work on
>> > adding tests for recovery_target_incomplete.
>>
>> Do you know when those will be ready?
>>
>
> Attached are both the patches with tests included.
>
>
>>
>> >
>> >
>> > 4) I'm not convinced that fatal errors by default are the way to go.
>> > It's a pretty big departure from what we have now.
>> >
>> >
>> > I have changed the code to generate ERRORs instead of FATALs. Which is
>> > in the patch recoveryStartPoint.patch
>>
>> I think at this point in recovery any ERROR at all will probably be
>> fatal. Once you have some tests in place we'll know for sure.
>>
>> Either way, the goal would be to keep recovery from proceeding and let
>> the user adjust their targets. Since this is a big behavioral change
>> which will need buy in from the community.
>>
>
> Hoping to get some comments / feedback from the community on the second
> patch too.
>
> > Also, I don't think it's very intuitive how
>> recovery_target_incomplete
>> > works. For instance, if I set recovery_target_incomplete=promote
>> and
>> > set recovery_target_time, but pick a backup after the specified
>> time,
>> > then there will be an error "recovery_target_time %s is older..."
>> rather
>> > than a promotion.
>> >
>> >
>> > Yes, that is correct and that is the expected behaviour. Let me explain
>> -
>>
>> My point was that this combination of options could lead to confusion on
>> the part of users. However, I'd rather leave that alone for the time
>> being and focus on the first patch.
>>
>> > I have split the patch into two different
>> > pieces. One is to determine if the recovery can start at all and other
>> > patch deals with the incomplete recovery situation.
>>
>> I think the first patch looks promising and I would rather work through
>> that before considering the second patch. Once there are tests for the
>> first patch I will complete my review.
>>
>
> I have added all the tests for the second patch and all seem to be working
> fine.
>
> Regarding the first patch, i have included all the tests except for the
> test case on recovery_target_time.
> I did write the test, but, the test is kind of behaving weird which i am
> working through to resolve.
>

This issue has been resolved. All good now. To avoid any further confusion,
i have attached the latest versions (4) of both the patches with all the
tests included.

I have already changed the patch status to "Needs review".

Thank you !

Regards,

Venkata B N
Database Consultant