Re: Allow to_date() and to_timestamp() to accept localized names

On Thu, Sep 26, 2019 at 08:36:25PM +0200, Juan José Santamaría Flecha wrote:
>On Wed, Sep 25, 2019 at 9:57 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> This patch no longer applies.  Can you please rebase?
>
>Thank you for the notification.
>
>The patch rot after commit 1a950f3, a rebased version is attached.
>

Thanks. I did a quick review of this patch, and I think it's almost RFC.
I only found a couple of minor issue:

- In func.sgml, it seems we've lost this bit:

       <para>
        <literal>TM</literal> does not include trailing blanks.
        <function>to_timestamp</function> and <function>to_date</function> ignore
        the <literal>TM</literal> modifier.
       </para>

   Does that mean the function no longer ignore the TM modifier? That
   would be somewhat problematic (i.e. it might break some user code).
   But looking at the code I don't see why the patch would have this
   effect, so I suppose it's merely a doc bug.

- I don't think we need to include examples how to_timestmap ignores
   case, I'd say just stating the fact is clear enough. But if we want to
   have examples, I think we should not inline in the para but use the
   established pattern:

   <para>
     Some examples:
<programlisting>
...
</programlisting>
    </para>

   which is used elsewhere in the func.sgml file.

- In formatting.c the "common/unicode_norm.h" should be right after
   includes from "catalog/" to follow the alphabetic order (unless
   there's a reason why that does not work).

- I rather dislike the "dim" parameter name, because I immediately think
   "dimension" which makes little sense. I suggest renaming to "nitems"
   or "nelements" or something like that. 


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services