Re: [HACKERS] patch proposal - Mailing list pgsql-hackers

Hi David,

On Tue, Jan 31, 2017 at 6:49 AM, David Steele <david@pgmasters.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.