Horton

Thread

Commits

  1. Add CREATE DATABASE LOCALE option

  1. Add CREATE DATABASE LOCALE option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-05T20:17:25Z

    I propose this patch to add a LOCALE option to CREATE DATABASE.  This
sets both LC_COLLATE and LC_CTYPE with one option.  Similar behavior is
already supported in initdb, CREATE COLLATION, and createdb.

With collation providers other than libc, having separate lc_collate and
lc_ctype settings is not necessarily applicable, so this is also
preparation for such future functionality.

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

  2. Re: Add CREATE DATABASE LOCALE option

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2019-06-05T20:31:37Z

    On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
>
> I propose this patch to add a LOCALE option to CREATE DATABASE.  This
> sets both LC_COLLATE and LC_CTYPE with one option.  Similar behavior is
> already supported in initdb, CREATE COLLATION, and createdb.
>
> With collation providers other than libc, having separate lc_collate and
> lc_ctype settings is not necessarily applicable, so this is also
> preparation for such future functionality.
>

Cool... would be nice also add some test cases.

Regards,

--
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

  3. Re: Add CREATE DATABASE LOCALE option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-06T09:38:06Z

    On 2019-06-05 22:31, Fabrízio de Royes Mello wrote:
> On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com
> <mailto:peter.eisentraut@2ndquadrant.com>> wrote:
>>
>> I propose this patch to add a LOCALE option to CREATE DATABASE.  This
>> sets both LC_COLLATE and LC_CTYPE with one option.  Similar behavior is
>> already supported in initdb, CREATE COLLATION, and createdb.
>>
>> With collation providers other than libc, having separate lc_collate and
>> lc_ctype settings is not necessarily applicable, so this is also
>> preparation for such future functionality.
> 
> Cool... would be nice also add some test cases.

Right.  Any suggestions where to put them?

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

  4. Re: Add CREATE DATABASE LOCALE option

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2019-06-06T19:03:48Z

    On Thu, Jun 6, 2019 at 6:38 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2019-06-05 22:31, Fabrízio de Royes Mello wrote:
> > On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com
> > <mailto:peter.eisentraut@2ndquadrant.com>> wrote:
> >>
> >> I propose this patch to add a LOCALE option to CREATE DATABASE.  This
> >> sets both LC_COLLATE and LC_CTYPE with one option.  Similar behavior is
> >> already supported in initdb, CREATE COLLATION, and createdb.
> >>
> >> With collation providers other than libc, having separate lc_collate
and
> >> lc_ctype settings is not necessarily applicable, so this is also
> >> preparation for such future functionality.
> >
> > Cool... would be nice also add some test cases.
>
> Right.  Any suggestions where to put them?
>

Hmm... good question... I thought we already have some regression tests for
{CREATE|DROP} DATABASE but actually we don't... should we add a new one?

Att,

-- 
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

  5. Re: Add CREATE DATABASE LOCALE option

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-06T19:52:35Z

    On 2019-Jun-06, Fabrízio de Royes Mello wrote:

> > > Cool... would be nice also add some test cases.
> >
> > Right.  Any suggestions where to put them?
> 
> Hmm... good question... I thought we already have some regression tests for
> {CREATE|DROP} DATABASE but actually we don't... should we add a new one?

I think pg_dump/t/002_pg_dump.pl might be a good place.  Not the easiest
program in the world to work with, admittedly.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Add CREATE DATABASE LOCALE option

    Heikki Linnakangas <hlinnaka@iki.fi> — 2019-06-14T09:57:58Z

    On 05/06/2019 23:17, Peter Eisentraut wrote:
> I propose this patch to add a LOCALE option to CREATE DATABASE.  This
> sets both LC_COLLATE and LC_CTYPE with one option.  Similar behavior is
> already supported in initdb, CREATE COLLATION, and createdb.
> 
> With collation providers other than libc, having separate lc_collate and
> lc_ctype settings is not necessarily applicable, so this is also
> preparation for such future functionality.

One objection is that the proposed LOCALE option would only affect 
LC_COLLATE and LC_CTYPE. What about lc_messages, lc_monetary, lc_numeric 
and lc_time? initdb's --locale option sets those, too. Should CREATE 
DATABASE LOCALE set those as well?

On the whole, +1 on adding the option. In practice, you always want to 
set LC_COLLATE and LC_CTYPE to the same value, so we should make that 
easy. But let's consider those other variables too, at least we've got 
to document it carefully.


PS. There was some discussion on doing this when the LC_COLLATE and 
LC_CTYPE options were added: 
https://www.postgresql.org/message-id/491862F7.1060501%40enterprisedb.com. 
My reading of that is that there was no strong consensus, so we just let 
it be.

- Heikki

  7. Re: Add CREATE DATABASE LOCALE option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-23T18:13:28Z

    On 2019-06-06 21:52, Alvaro Herrera wrote:
> On 2019-Jun-06, Fabrízio de Royes Mello wrote:
> 
>>>> Cool... would be nice also add some test cases.
>>>
>>> Right.  Any suggestions where to put them?
>>
>> Hmm... good question... I thought we already have some regression tests for
>> {CREATE|DROP} DATABASE but actually we don't... should we add a new one?
> 
> I think pg_dump/t/002_pg_dump.pl might be a good place.  Not the easiest
> program in the world to work with, admittedly.

Updated patch with test and expanded documentation.

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

  8. Re: Add CREATE DATABASE LOCALE option

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-07-13T17:20:12Z

    Hello Peter,

>> I think pg_dump/t/002_pg_dump.pl might be a good place.  Not the easiest
>> program in the world to work with, admittedly.
>
> Updated patch with test and expanded documentation.

Patch v2 applies cleanly, compiles, make check-world ok with tap enabled. 
Doc gen ok.

The addition looks reasonable.

The second error message about conflicting option could more explicit than 
a terse "conflicting or redundant options"? The user may expect later 
options to superseedes earlier options, for instance.

About the pg_dump code, I'm wondering whether it is worth generating 
LOCALE as it breaks backward compatibility (eg dumping a new db to load it 
into a older version).

If it is to be generated, I'd do merge the two conditions instead of 
nesting.

   if (strlen(collate) > 0 && strcmp(collate, ctype) == 0)
     // generate LOCALE

-- 
Fabien.

  9. Re: Add CREATE DATABASE LOCALE option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-07-22T18:36:39Z

    On 2019-07-13 19:20, Fabien COELHO wrote:
> The second error message about conflicting option could more explicit than 
> a terse "conflicting or redundant options"? The user may expect later 
> options to superseedes earlier options, for instance.

done

> About the pg_dump code, I'm wondering whether it is worth generating 
> LOCALE as it breaks backward compatibility (eg dumping a new db to load it 
> into a older version).

We don't really care about backward compatibility here.  Moving forward,
with ICU and such, we don't want to have to drag around old syntax forever.

> If it is to be generated, I'd do merge the two conditions instead of 
> nesting.
> 
>    if (strlen(collate) > 0 && strcmp(collate, ctype) == 0)
>      // generate LOCALE

done

How about this patch?

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

  10. Re: Add CREATE DATABASE LOCALE option

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-07-22T22:18:19Z

    Hello Peter,

>> About the pg_dump code, I'm wondering whether it is worth generating
>> LOCALE as it breaks backward compatibility (eg dumping a new db to load it
>> into a older version).
>
> We don't really care about backward compatibility here.  Moving forward,
> with ICU and such, we don't want to have to drag around old syntax forever.

We will drag it anyway because LOCALE is just a shortcut for the other two 
LC_* when they have the same value.

> How about this patch?

It applies cleanly, compiles, global & pg_dump make check ok, doc gen ok.

I'm still unconvinced of the interest of breaking backward compatibility, 
but this is no big deal.

I do not like much calling strlen() to check whether a string is empty, 
but this is pre-existing.

I switched the patch to READY.

-- 
Fabien.

  11. Re: Add CREATE DATABASE LOCALE option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-07-23T13:00:25Z

    On 2019-07-23 00:18, Fabien COELHO wrote:
> It applies cleanly, compiles, global & pg_dump make check ok, doc gen ok.
> 
> I'm still unconvinced of the interest of breaking backward compatibility, 
> but this is no big deal.
> 
> I do not like much calling strlen() to check whether a string is empty, 
> but this is pre-existing.
> 
> I switched the patch to READY.

committed

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