Обсуждение: Add CREATE DATABASE LOCALE option

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

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

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

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

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

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.

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

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.

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