Re: Recovery target 'immediate'

On Thu, May 2, 2013 at 8:55 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 26 April 2013 18:13, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
>> On 26.04.2013 19:50, Magnus Hagander wrote:
>>>
>>> On Fri, Apr 26, 2013 at 6:43 PM, Simon Riggs<simon(at)2ndquadrant(dot)com>
>>> wrote:
>>>>
>>>> On 26 April 2013 17:25, Heikki Linnakangas<hlinnakangas(at)vmware(dot)com>
>>>> wrote:
>>>>>
>>>>> Actually, from a usability point of view I think would be nice to have
>>>>> just
>>>>>
>>>>> one setting, "recovery_target". It's already somewhat confusing to have
>>>>> recovery_target_xid, recovery_target_time, and recovery_target_name,
>>>>> which
>>>>> are mutually exclusive, and recovery_target_inclusive which is just a
>>>>> modifier for the others. Maybe something like:
>>>>>
>>>>> recovery_target = 'xid 1234'
>>>>> recovery_target = 'xid 1234 exclusive'
>>>>> recovery_target = '2013-04-22 12:33'
>>>>> recovery_target = '2013-04-22 12:33 exclusive'
>>>>> recovery_target = 'consistent'
>>>>> recovery_target = 'name: daily backup'
>>>>
>>>>
>>>> So now you want to change the whole existing API so it fits with your
>>>> one new requirement?
>>
>>
>> No, I think the above would be a usability improvement whether or not we add
>> the new feature.
>
>
> I don't see the usability improvement. This is only being suggested to
> make one new addition look cleaner; there isn't a common gripe that
> the use of parameters is hard to use, other than their location and
> the ability to treat them as GUCs.

Actually, there is - I hear it quite often from people not so
experienced in PostgreSQL. Though in fairness, I'm not entirely sure
the new syntax would help - some of those need a tool to do it for
them, really (and such tools exist, I believe).

That said, there is one property that's very unclear now and that's
that you can only set one of recovery_target_time, recovery_target_xid
and recovery_target_name. But they can be freely combined with
recovery_target_timeline and recovery_target_inclusive. That's quite
confusing.

> This changes the existing API which will confuse people that know it
> and invalidate everything written in software and on wikis as to how
> to do it. That means all the "in case of fire break glass"
> instructions are all wrong and need to be rewritten and retested.

Yes, *that* is the main reason *not* to make the change. It has a
pretty bad cost in backwards compatibility loss. There is a gain, but
I don't think it outweighs the cost.

> It also introduces a single common datatype for such entries, where
> before we had that xids were numbers, names were text, so this new
> mechanism operates completely differently from all other GUC
> parameters.
>
> Plus its inconsistent, in that with xids you have 'xid 1234' whereas
> timestamps just say '2013-04-22' rather than 'timestamp 2013-04-22',
> or with names should they end in a colon or not. There'n no clear
> differentiation between text for names and other keywords. Presumably
> we'll need a complex parser to sort that out.

I'm assuming that was just typos in Heikki's example. I'm sure he
meant them to be consistent.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/