On 08/20/2016 04:16 PM, Michael Paquier wrote:
> On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>> On 20/08/16 02:13, Michael Paquier wrote:
>>> On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat
>>> <adrien.nayrat@dalibo.com> wrote:
>>> Using a PG_TRY/CATCH block the way you do to show to user a different
>>> error message while the original one is actually correct does not
>>> sound like a good idea to me. It complicates the code and the original
>>> pattern matches what is already done for timestamps, where in case of
>>> error you'd get that:
>>> FATAL: invalid input syntax for type timestamp with time zone: "aa"
>>>
>>> I think that a better solution would be to make clearer in the docs
>>> that pg_lsn is used here. First, in recovery.conf.sample, let's use
>>> pg_lsn and not LSN. Then, in the sgml docs, let's add a reference to
>>> the page of pg_lsn as well:
>>> https://www.postgresql.org/docs/devel/static/datatype-pg-lsn.html
>>>
>>> Now we have that:
>>> + <para>
>>> + This parameter specifies the LSN of the write-ahead log stream up
>>> to
>>> + which recovery will proceed. The precise stopping point is also
>>> + influenced by <xref linkend="recovery-target-inclusive">.
>>> + </para>
>>> Perhaps we could just add an additional sentence: "This parameter
>>> value is parsed using the system datatype <link=pg_lsn>". Or something
>>> like that. Thoughts?
>>>
>>
>> +1
>
> So here is the patch resulting in that. After thinking more about that
> I have arrived at the conclusion to not use LSN in recovery.conf, but
> "WAL position (LSN)", and also mention in the docs that pg_lsn is used
> to parse the parameter value. A couple of log messages are changed
> similarly. It definitely makes it more understandable for users to
> speak of WAL positions.
Good, I prefer this.
> I also noticed that the top block of "Recovery Target Settings" needs
> to mention recovery_target_lsn, aka when name, xid, lsn and time are
> combined in the same recovery.conf, only the last value will be used.
>
Good catch!
As Julien said, there is nothing to notice that error comes from
recovery.conf.
My fear would be that an user encounters an error like this. Il will be
difficult to link to the recovery.conf.
For the others settings (xid, timeline,name) there is an explicit
message that notice error is in recovery.conf.
I see it is not the case for recovery_target_time.
Should we modify only the documentation or shoud we try to find a
solution to point the origin of error?
--
Adrien NAYRAT
http://dalibo.com - http://dalibo.org
