Add CREATE DATABASE LOCALE option

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Add CREATE DATABASE LOCALE option

Peter Eisentraut-6
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

0001-Add-CREATE-DATABASE-LOCALE-option.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

fabriziomello

On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut <[hidden email]> 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
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Peter Eisentraut-6
On 2019-06-05 22:31, Fabrízio de Royes Mello wrote:

> On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut
> <[hidden email]
> <mailto:[hidden email]>> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

fabriziomello

On Thu, Jun 6, 2019 at 6:38 AM Peter Eisentraut <[hidden email]> wrote:

>
> On 2019-06-05 22:31, Fabrízio de Royes Mello wrote:
> > On Wed, Jun 5, 2019 at 5:17 PM Peter Eisentraut
> > <[hidden email]
> > <mailto:[hidden email]>> 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
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Heikki Linnakangas
In reply to this post by Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Peter Eisentraut-6
In reply to this post by Alvaro Herrera-9
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

v2-0001-Add-CREATE-DATABASE-LOCALE-option.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Peter Eisentraut-6
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

v3-0001-Add-CREATE-DATABASE-LOCALE-option.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: Add CREATE DATABASE LOCALE option

Peter Eisentraut-6
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