public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1] Replace the char to bool array for null parameters
4+ messages / 4 participants
[nested] [flat]
* [PATCH v1] Replace the char to bool array for null parameters
@ 2021-01-19 07:51 Japin Li <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Japin Li @ 2021-01-19 07:51 UTC (permalink / raw)
---
doc/src/sgml/spi.sgml | 60 ++++++++++++-----------------
src/backend/executor/spi.c | 18 ++++-----
src/backend/utils/adt/ri_triggers.c | 8 ++--
src/backend/utils/adt/ruleutils.c | 10 ++---
src/include/executor/spi.h | 12 +++---
src/pl/plperl/plperl.c | 6 +--
src/pl/plpython/plpy_cursorobject.c | 6 +--
src/test/regress/regress.c | 12 +++---
8 files changed, 61 insertions(+), 71 deletions(-)
diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
index f5e0a35da0..d3cc405c42 100644
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -649,7 +649,7 @@ int SPI_exec(const char * <parameter>command</parameter>, long <parameter>count<
<synopsis>
int SPI_execute_with_args(const char *<parameter>command</parameter>,
int <parameter>nargs</parameter>, Oid *<parameter>argtypes</parameter>,
- Datum *<parameter>values</parameter>, const char *<parameter>nulls</parameter>,
+ Datum *<parameter>values</parameter>, const bool *<parameter>nulls</parameter>,
bool <parameter>read_only</parameter>, long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
@@ -728,7 +728,7 @@ int SPI_execute_with_args(const char *<parameter>command</parameter>,
</varlistentry>
<varlistentry>
- <term><literal>const char * <parameter>nulls</parameter></literal></term>
+ <term><literal>const bool * <parameter>nulls</parameter></literal></term>
<listitem>
<para>
an array of length <parameter>nargs</parameter>, describing which
@@ -739,12 +739,10 @@ int SPI_execute_with_args(const char *<parameter>command</parameter>,
If <parameter>nulls</parameter> is <symbol>NULL</symbol> then
<function>SPI_execute_with_args</function> assumes that no parameters
are null. Otherwise, each entry of the <parameter>nulls</parameter>
- array should be <literal>' '</literal> if the corresponding parameter
- value is non-null, or <literal>'n'</literal> if the corresponding parameter
+ array should be <literal>false</literal> if the corresponding parameter
+ value is non-null, or <literal>true</literal> if the corresponding parameter
value is null. (In the latter case, the actual value in the
- corresponding <parameter>values</parameter> entry doesn't matter.) Note
- that <parameter>nulls</parameter> is not a text string, just an array:
- it does not need a <literal>'\0'</literal> terminator.
+ corresponding <parameter>values</parameter> entry doesn't matter.)
</para>
</listitem>
</varlistentry>
@@ -1601,7 +1599,7 @@ bool SPI_is_cursor_plan(SPIPlanPtr <parameter>plan</parameter>)
<refsynopsisdiv>
<synopsis>
-int SPI_execute_plan(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char * <parameter>nulls</parameter>,
+int SPI_execute_plan(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const bool * <parameter>nulls</parameter>,
bool <parameter>read_only</parameter>, long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
@@ -1642,7 +1640,7 @@ int SPI_execute_plan(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>
</varlistentry>
<varlistentry>
- <term><literal>const char * <parameter>nulls</parameter></literal></term>
+ <term><literal>const bool * <parameter>nulls</parameter></literal></term>
<listitem>
<para>
An array describing which parameters are null. Must have same length as
@@ -1653,12 +1651,10 @@ int SPI_execute_plan(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>
If <parameter>nulls</parameter> is <symbol>NULL</symbol> then
<function>SPI_execute_plan</function> assumes that no parameters
are null. Otherwise, each entry of the <parameter>nulls</parameter>
- array should be <literal>' '</literal> if the corresponding parameter
- value is non-null, or <literal>'n'</literal> if the corresponding parameter
+ array should be <literal>false</literal> if the corresponding parameter
+ value is non-null, or <literal>true</literal> if the corresponding parameter
value is null. (In the latter case, the actual value in the
- corresponding <parameter>values</parameter> entry doesn't matter.) Note
- that <parameter>nulls</parameter> is not a text string, just an array:
- it does not need a <literal>'\0'</literal> terminator.
+ corresponding <parameter>values</parameter> entry doesn't matter.)
</para>
</listitem>
</varlistentry>
@@ -1946,7 +1942,7 @@ int SPI_execute_plan_with_receiver(SPIPlanPtr <parameter>plan</parameter>,
<refsynopsisdiv>
<synopsis>
-int SPI_execp(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char * <parameter>nulls</parameter>, long <parameter>count</parameter>)
+int SPI_execp(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const bool * <parameter>nulls</parameter>, long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
@@ -1985,7 +1981,7 @@ int SPI_execp(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values<
</varlistentry>
<varlistentry>
- <term><literal>const char * <parameter>nulls</parameter></literal></term>
+ <term><literal>const bool * <parameter>nulls</parameter></literal></term>
<listitem>
<para>
An array describing which parameters are null. Must have same length as
@@ -1996,12 +1992,10 @@ int SPI_execp(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values<
If <parameter>nulls</parameter> is <symbol>NULL</symbol> then
<function>SPI_execp</function> assumes that no parameters
are null. Otherwise, each entry of the <parameter>nulls</parameter>
- array should be <literal>' '</literal> if the corresponding parameter
- value is non-null, or <literal>'n'</literal> if the corresponding parameter
+ array should be <literal>false</literal> if the corresponding parameter
+ value is non-null, or <literal>true</literal> if the corresponding parameter
value is null. (In the latter case, the actual value in the
- corresponding <parameter>values</parameter> entry doesn't matter.) Note
- that <parameter>nulls</parameter> is not a text string, just an array:
- it does not need a <literal>'\0'</literal> terminator.
+ corresponding <parameter>values</parameter> entry doesn't matter.)
</para>
</listitem>
</varlistentry>
@@ -2051,7 +2045,7 @@ int SPI_execp(SPIPlanPtr <parameter>plan</parameter>, Datum * <parameter>values<
<refsynopsisdiv>
<synopsis>
Portal SPI_cursor_open(const char * <parameter>name</parameter>, SPIPlanPtr <parameter>plan</parameter>,
- Datum * <parameter>values</parameter>, const char * <parameter>nulls</parameter>,
+ Datum * <parameter>values</parameter>, const bool * <parameter>nulls</parameter>,
bool <parameter>read_only</parameter>)
</synopsis>
</refsynopsisdiv>
@@ -2117,7 +2111,7 @@ Portal SPI_cursor_open(const char * <parameter>name</parameter>, SPIPlanPtr <par
</varlistentry>
<varlistentry>
- <term><literal>const char * <parameter>nulls</parameter></literal></term>
+ <term><literal>const bool * <parameter>nulls</parameter></literal></term>
<listitem>
<para>
An array describing which parameters are null. Must have same length as
@@ -2128,12 +2122,10 @@ Portal SPI_cursor_open(const char * <parameter>name</parameter>, SPIPlanPtr <par
If <parameter>nulls</parameter> is <symbol>NULL</symbol> then
<function>SPI_cursor_open</function> assumes that no parameters
are null. Otherwise, each entry of the <parameter>nulls</parameter>
- array should be <literal>' '</literal> if the corresponding parameter
- value is non-null, or <literal>'n'</literal> if the corresponding parameter
+ array should be <literal>false</literal> if the corresponding parameter
+ value is non-null, or <literal>true</literal> if the corresponding parameter
value is null. (In the latter case, the actual value in the
- corresponding <parameter>values</parameter> entry doesn't matter.) Note
- that <parameter>nulls</parameter> is not a text string, just an array:
- it does not need a <literal>'\0'</literal> terminator.
+ corresponding <parameter>values</parameter> entry doesn't matter.)
</para>
</listitem>
</varlistentry>
@@ -2177,7 +2169,7 @@ Portal SPI_cursor_open(const char * <parameter>name</parameter>, SPIPlanPtr <par
Portal SPI_cursor_open_with_args(const char *<parameter>name</parameter>,
const char *<parameter>command</parameter>,
int <parameter>nargs</parameter>, Oid *<parameter>argtypes</parameter>,
- Datum *<parameter>values</parameter>, const char *<parameter>nulls</parameter>,
+ Datum *<parameter>values</parameter>, const bool *<parameter>nulls</parameter>,
bool <parameter>read_only</parameter>, int <parameter>cursorOptions</parameter>)
</synopsis>
</refsynopsisdiv>
@@ -2261,7 +2253,7 @@ Portal SPI_cursor_open_with_args(const char *<parameter>name</parameter>,
</varlistentry>
<varlistentry>
- <term><literal>const char * <parameter>nulls</parameter></literal></term>
+ <term><literal>const bool * <parameter>nulls</parameter></literal></term>
<listitem>
<para>
an array of length <parameter>nargs</parameter>, describing which
@@ -2272,12 +2264,10 @@ Portal SPI_cursor_open_with_args(const char *<parameter>name</parameter>,
If <parameter>nulls</parameter> is <symbol>NULL</symbol> then
<function>SPI_cursor_open_with_args</function> assumes that no parameters
are null. Otherwise, each entry of the <parameter>nulls</parameter>
- array should be <literal>' '</literal> if the corresponding parameter
- value is non-null, or <literal>'n'</literal> if the corresponding parameter
+ array should be <literal>false</literal> if the corresponding parameter
+ value is non-null, or <literal>true</literal> if the corresponding parameter
value is null. (In the latter case, the actual value in the
- corresponding <parameter>values</parameter> entry doesn't matter.) Note
- that <parameter>nulls</parameter> is not a text string, just an array:
- it does not need a <literal>'\0'</literal> terminator.
+ corresponding <parameter>values</parameter> entry doesn't matter.)
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index e28d242922..631ab2167a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -70,7 +70,7 @@ static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
DestReceiver *caller_dest);
static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls);
+ Datum *Values, const bool *Nulls);
static int _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount);
@@ -536,7 +536,7 @@ SPI_exec(const char *src, long tcount)
/* Execute a previously prepared plan */
int
-SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
+SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const bool *Nulls,
bool read_only, long tcount)
{
int res;
@@ -563,7 +563,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
/* Obsolete version of SPI_execute_plan */
int
-SPI_execp(SPIPlanPtr plan, Datum *Values, const char *Nulls, long tcount)
+SPI_execp(SPIPlanPtr plan, Datum *Values, const bool *Nulls, long tcount)
{
return SPI_execute_plan(plan, Values, Nulls, false, tcount);
}
@@ -634,7 +634,7 @@ SPI_execute_plan_with_receiver(SPIPlanPtr plan,
*/
int
SPI_execute_snapshot(SPIPlanPtr plan,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
bool read_only, bool fire_triggers, long tcount)
{
@@ -669,7 +669,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
int
SPI_execute_with_args(const char *src,
int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
bool read_only, long tcount)
{
int res;
@@ -1339,7 +1339,7 @@ SPI_freetuptable(SPITupleTable *tuptable)
*/
Portal
SPI_cursor_open(const char *name, SPIPlanPtr plan,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
bool read_only)
{
Portal portal;
@@ -1368,7 +1368,7 @@ Portal
SPI_cursor_open_with_args(const char *name,
const char *src,
int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
bool read_only, int cursorOptions)
{
Portal result;
@@ -2636,7 +2636,7 @@ fail:
*/
static ParamListInfo
_SPI_convert_params(int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls)
+ Datum *Values, const bool *Nulls)
{
ParamListInfo paramLI;
@@ -2649,7 +2649,7 @@ _SPI_convert_params(int nargs, Oid *argtypes,
ParamExternData *prm = ¶mLI->params[i];
prm->value = Values[i];
- prm->isnull = (Nulls && Nulls[i] == 'n');
+ prm->isnull = Nulls && Nulls[i];
prm->pflags = PARAM_FLAG_CONST;
prm->ptype = argtypes[i];
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6e3a41062f..793766570d 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -216,7 +216,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
bool detectNewRows, int expect_OK);
static void ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
- Datum *vals, char *nulls);
+ Datum *vals, bool *nulls);
static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
Relation pk_rel, Relation fk_rel,
TupleTableSlot *violatorslot, TupleDesc tupdesc,
@@ -2191,7 +2191,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
Oid save_userid;
int save_sec_context;
Datum vals[RI_MAX_NUMKEYS * 2];
- char nulls[RI_MAX_NUMKEYS * 2];
+ bool nulls[RI_MAX_NUMKEYS * 2];
/*
* Use the query type code to determine whether the query is run against
@@ -2314,7 +2314,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
static void
ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
- Datum *vals, char *nulls)
+ Datum *vals, bool *nulls)
{
const int16 *attnums;
bool isnull;
@@ -2327,7 +2327,7 @@ ri_ExtractValues(Relation rel, TupleTableSlot *slot,
for (int i = 0; i < riinfo->nkeys; i++)
{
vals[i] = slot_getattr(slot, attnums[i], &isnull);
- nulls[i] = isnull ? 'n' : ' ';
+ nulls[i] = isnull;
}
}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8a1fbda572..3c8fa2c95d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -531,7 +531,7 @@ static char *
pg_get_ruledef_worker(Oid ruleoid, int prettyFlags)
{
Datum args[1];
- char nulls[1];
+ bool nulls[1];
int spirc;
HeapTuple ruletup;
TupleDesc rulettc;
@@ -570,7 +570,7 @@ pg_get_ruledef_worker(Oid ruleoid, int prettyFlags)
* Get the pg_rewrite tuple for this rule
*/
args[0] = ObjectIdGetDatum(ruleoid);
- nulls[0] = ' ';
+ nulls[0] = false;
spirc = SPI_execute_plan(plan_getrulebyoid, args, nulls, true, 0);
if (spirc != SPI_OK_SELECT)
elog(ERROR, "failed to get pg_rewrite tuple for rule %u", ruleoid);
@@ -724,7 +724,7 @@ static char *
pg_get_viewdef_worker(Oid viewoid, int prettyFlags, int wrapColumn)
{
Datum args[2];
- char nulls[2];
+ bool nulls[2];
int spirc;
HeapTuple ruletup;
TupleDesc rulettc;
@@ -765,8 +765,8 @@ pg_get_viewdef_worker(Oid viewoid, int prettyFlags, int wrapColumn)
*/
args[0] = ObjectIdGetDatum(viewoid);
args[1] = DirectFunctionCall1(namein, CStringGetDatum(ViewSelectRuleName));
- nulls[0] = ' ';
- nulls[1] = ' ';
+ nulls[0] = false;
+ nulls[1] = false;
spirc = SPI_execute_plan(plan_getviewrule, args, nulls, true, 0);
if (spirc != SPI_OK_SELECT)
elog(ERROR, "failed to get pg_rewrite tuple for view %u", viewoid);
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 9c70603434..8e4bd34ff7 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -94,7 +94,7 @@ extern int SPI_connect(void);
extern int SPI_connect_ext(int options);
extern int SPI_finish(void);
extern int SPI_execute(const char *src, bool read_only, long tcount);
-extern int SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
+extern int SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const bool *Nulls,
bool read_only, long tcount);
extern int SPI_execute_plan_with_paramlist(SPIPlanPtr plan,
ParamListInfo params,
@@ -104,16 +104,16 @@ extern int SPI_execute_plan_with_receiver(SPIPlanPtr plan,
bool read_only, long tcount,
DestReceiver *dest);
extern int SPI_exec(const char *src, long tcount);
-extern int SPI_execp(SPIPlanPtr plan, Datum *Values, const char *Nulls,
+extern int SPI_execp(SPIPlanPtr plan, Datum *Values, const bool *Nulls,
long tcount);
extern int SPI_execute_snapshot(SPIPlanPtr plan,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
bool read_only, bool fire_triggers, long tcount);
extern int SPI_execute_with_args(const char *src,
int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
bool read_only, long tcount);
extern int SPI_execute_with_receiver(const char *src,
ParamListInfo params,
@@ -161,11 +161,11 @@ extern void SPI_freetuple(HeapTuple pointer);
extern void SPI_freetuptable(SPITupleTable *tuptable);
extern Portal SPI_cursor_open(const char *name, SPIPlanPtr plan,
- Datum *Values, const char *Nulls, bool read_only);
+ Datum *Values, const bool *Nulls, bool read_only);
extern Portal SPI_cursor_open_with_args(const char *name,
const char *src,
int nargs, Oid *argtypes,
- Datum *Values, const char *Nulls,
+ Datum *Values, const bool *Nulls,
bool read_only, int cursorOptions);
extern Portal SPI_cursor_open_with_paramlist(const char *name, SPIPlanPtr plan,
ParamListInfo params, bool read_only);
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 6299adf71a..1b8946d9fb 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3806,7 +3806,7 @@ SV *
plperl_spi_query_prepared(char *query, int argc, SV **argv)
{
int i;
- char *nulls;
+ bool *nulls;
Datum *argvalues;
plperl_query_desc *qdesc;
plperl_query_entry *hash_entry;
@@ -3849,7 +3849,7 @@ plperl_spi_query_prepared(char *query, int argc, SV **argv)
************************************************************/
if (argc > 0)
{
- nulls = (char *) palloc(argc);
+ nulls = (bool *) palloc(argc);
argvalues = (Datum *) palloc(argc * sizeof(Datum));
}
else
@@ -3869,7 +3869,7 @@ plperl_spi_query_prepared(char *query, int argc, SV **argv)
&qdesc->arginfuncs[i],
qdesc->argtypioparams[i],
&isnull);
- nulls[i] = isnull ? 'n' : ' ';
+ nulls[i] = isnull;
}
/************************************************************
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 08d8b607e3..0918fe0f1f 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -201,11 +201,11 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
PG_TRY();
{
Portal portal;
- char *volatile nulls;
+ bool *volatile nulls;
volatile int j;
if (nargs > 0)
- nulls = palloc(nargs * sizeof(char));
+ nulls = palloc(nargs * sizeof(bool));
else
nulls = NULL;
@@ -220,7 +220,7 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
bool isnull;
plan->values[j] = PLy_output_convert(arg, elem, &isnull);
- nulls[j] = isnull ? 'n' : ' ';
+ nulls[j] = isnull;
}
PG_FINALLY();
{
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 32ab9ed6b5..b31202d500 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -273,7 +273,7 @@ ttdummy(PG_FUNCTION_ARGS)
Datum newon,
newoff;
Datum *cvals; /* column values */
- char *cnulls; /* column nulls */
+ bool *cnulls; /* column nulls */
char *relname; /* triggered relation name */
Relation rel; /* triggered relation */
HeapTuple trigtuple;
@@ -374,27 +374,27 @@ ttdummy(PG_FUNCTION_ARGS)
/* Fetch tuple values and nulls */
cvals = (Datum *) palloc(natts * sizeof(Datum));
- cnulls = (char *) palloc(natts * sizeof(char));
+ cnulls = (bool *) palloc(natts * sizeof(bool));
for (i = 0; i < natts; i++)
{
cvals[i] = SPI_getbinval((newtuple != NULL) ? newtuple : trigtuple,
tupdesc, i + 1, &isnull);
- cnulls[i] = (isnull) ? 'n' : ' ';
+ cnulls[i] = isnull;
}
/* change date column(s) */
if (newtuple) /* UPDATE */
{
cvals[attnum[0] - 1] = newoff; /* start_date eq current date */
- cnulls[attnum[0] - 1] = ' ';
+ cnulls[attnum[0] - 1] = false;
cvals[attnum[1] - 1] = TTDUMMY_INFINITY; /* stop_date eq INFINITY */
- cnulls[attnum[1] - 1] = ' ';
+ cnulls[attnum[1] - 1] = false;
}
else
/* DELETE */
{
cvals[attnum[1] - 1] = newoff; /* stop_date eq current date */
- cnulls[attnum[1] - 1] = ' ';
+ cnulls[attnum[1] - 1] = false;
}
/* if there is no plan ... */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Order changes in PG16 since ICU introduction
@ 2023-05-22 20:09 Daniel Verite <[email protected]>
2023-05-24 15:39 ` Re: Order changes in PG16 since ICU introduction Jeff Davis <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Verite @ 2023-05-22 20:09 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Andrew Gierth <[email protected]>; Peter Eisentraut <[email protected]>; Sandro Santilli <[email protected]>; Tom Lane <[email protected]>; Regina Obe <[email protected]>; [email protected]
Jeff Davis wrote:
> If we special case locale=C, but do nothing for locale=fr_FR, then I'm
> not sure we've solved the problem. Andrew Gierth raised the issue here,
> which he called "maximally confusing":
>
> https://postgr.es/m/[email protected]
>
> That's why I feel that we need to make locale apply to whatever the
> provider is, not just when it happens to be C.
While I agree that the LOCALE option in CREATE DATABASE is
counter-intuitive, I find it questionable that blending ICU
and libc locales into it helps that much with the user experience.
Trying the lastest v6-* patches applied on top of 722541ead1
(before the pgindent run), here are a few examples when I
don't think it goes well.
The OS is Ubuntu 22.04 (glibc 2.35, ICU 70.1)
initdb:
Using default ICU locale "fr".
Using language tag "fr" for ICU locale "fr".
The database cluster will be initialized with this locale configuration:
provider: icu
ICU locale: fr
LC_COLLATE: fr_FR.UTF-8
LC_CTYPE: fr_FR.UTF-8
LC_MESSAGES: fr_FR.UTF-8
LC_MONETARY: fr_FR.UTF-8
LC_NUMERIC: fr_FR.UTF-8
LC_TIME: fr_FR.UTF-8
The default database encoding has accordingly been set to "UTF8".
#1
postgres=# create database test1 locale='fr_FR.UTF-8';
NOTICE: using standard form "fr-FR" for ICU locale "fr_FR.UTF-8"
ERROR: new ICU locale (fr-FR) is incompatible with the ICU locale of the
template database (fr)
HINT: Use the same ICU locale as in the template database, or use template0
as template.
That looks like a fairly generic case that doesn't work seamlessly.
#2
postgres=# create database test2 locale='C.UTF-8' template='template0';
NOTICE: using standard form "en-US-u-va-posix" for ICU locale "C.UTF-8"
CREATE DATABASE
en-US-u-va-posix does not sort like C.UTF-8 in glibc 2.35, so
this interpretation is arguably not what a user would expect.
I would expect the ICU warning or error (icu_validation_level) to kick
in instead of that transliteration.
#3
$ grep french /etc/locale.alias
french fr_FR.ISO-8859-1
postgres=# create database test3 locale='french' template='template0'
encoding='LATIN1';
WARNING: ICU locale "french" has unknown language "french"
HINT: To disable ICU locale validation, set parameter icu_validation_level
to DISABLED.
CREATE DATABASE
In practice we're probably getting the "und" ICU locale whereas "fr" would
be appropriate.
I assume that we would find more cases like that if testing on many
operating systems.
Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Order changes in PG16 since ICU introduction
2023-05-22 20:09 Re: Order changes in PG16 since ICU introduction Daniel Verite <[email protected]>
@ 2023-05-24 15:39 ` Jeff Davis <[email protected]>
2023-05-24 16:31 ` Re: Order changes in PG16 since ICU introduction Joe Conway <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Davis @ 2023-05-24 15:39 UTC (permalink / raw)
To: Daniel Verite <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Andrew Gierth <[email protected]>; Peter Eisentraut <[email protected]>; Sandro Santilli <[email protected]>; Tom Lane <[email protected]>; Regina Obe <[email protected]>; [email protected]
On Mon, 2023-05-22 at 22:09 +0200, Daniel Verite wrote:
> While I agree that the LOCALE option in CREATE DATABASE is
> counter-intuitive,
I think it's more than that. As Andreww Gierth pointed out:
$ initdb --locale=fr_FR
...
ICU locale: en-US
...
Is more than just counter-intuitive. I don't think we can ship 16 that
way.
> I find it questionable that blending ICU
> and libc locales into it helps that much with the user experience.
Thank you for going through some examples here. I agree that it's not
perfect, but we need some path to a reasonable ICU user experience, and
I think we'll have to accept some rough edges to avoid the worst cases,
like above.
> initdb:
>
> Using default ICU locale "fr".
> Using language tag "fr" for ICU locale "fr".
>
...
> #1
>
> postgres=# create database test1 locale='fr_FR.UTF-8';
> NOTICE: using standard form "fr-FR" for ICU locale "fr_FR.UTF-8"
> ERROR: new ICU locale (fr-FR) is incompatible with the ICU locale of
I don't see a problem here. If you specify LOCALE to CREATE DATABASE,
you should either be using "TEMPLATE template0", or you should be
expecting an error if the LOCALE doesn't match exactly.
What would you like to see happen here?
> #2
>
> postgres=# create database test2 locale='C.UTF-8'
> template='template0';
> NOTICE: using standard form "en-US-u-va-posix" for ICU locale
> "C.UTF-8"
> CREATE DATABASE
>
>
> en-US-u-va-posix does not sort like C.UTF-8 in glibc 2.35, so
> this interpretation is arguably not what a user would expect.
As you pointed out, this is not settled in libc either:
https://www.postgresql.org/message-id/8a3dc06f-9b9d-4ed7-9a12-2070d8b0165f%40manitou-mail.org
We really can't expect a particular order for a particular locale name,
unless we handle it specially like "C" or "POSIX". If we pass it to the
provider, we have to trust the provider to match our conceptual
expectations for that locale (and ideally version it properly).
> I would expect the ICU warning or error (icu_validation_level) to
> kick
> in instead of that transliteration.
Earlier versions of ICU (<= 63) do this transformation automatically,
and I don't see a reason to throw an error if ICU considers it valid.
The language tag en-US-u-va-posix will be stored in the catalog, and
that will be considered valid in later versions of ICU.
Later versions of ICU (>= 64) consider locales with a language name of
"C" to be obsolete and no longer valid. I added code to do the
transformation without error in these later versions, but I think we
have agreement to remove it.
If a user specifies the locale as "C.UTF-8", we can either pass it to
ICU and see whether that version accepts it or not (and if not, throw a
warning/error); or if we decide that "C.UTF-8" really means "C", we can
handle it in the memcmp() path like C and never send it to ICU.
> #3
>
> $ grep french /etc/locale.alias
> french fr_FR.ISO-8859-1
>
> postgres=# create database test3 locale='french' template='template0'
> encoding='LATIN1';
> WARNING: ICU locale "french" has unknown language "french"
> HINT: To disable ICU locale validation, set parameter
> icu_validation_level
> to DISABLED.
> CREATE DATABASE
>
>
> In practice we're probably getting the "und" ICU locale whereas "fr"
> would
> be appropriate.
This is a good point and illustrates that ICU is not a drop-in
replacement for libc in all cases.
I don't see a solution here that doesn't involve some rough edges,
though. "Locale" is a generic term, and if we continue to insist that
it really means a libc locale, then ICU will never be on an equal
footing with libc, let alone the preferred provider.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Order changes in PG16 since ICU introduction
2023-05-22 20:09 Re: Order changes in PG16 since ICU introduction Daniel Verite <[email protected]>
2023-05-24 15:39 ` Re: Order changes in PG16 since ICU introduction Jeff Davis <[email protected]>
@ 2023-05-24 16:31 ` Joe Conway <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Joe Conway @ 2023-05-24 16:31 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Daniel Verite <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Andrew Gierth <[email protected]>; Peter Eisentraut <[email protected]>; Sandro Santilli <[email protected]>; Tom Lane <[email protected]>; Regina Obe <[email protected]>; [email protected]
On 5/24/23 11:39, Jeff Davis wrote:
> On Mon, 2023-05-22 at 22:09 +0200, Daniel Verite wrote:
>> In practice we're probably getting the "und" ICU locale whereas "fr"
>> would be appropriate.
>
> This is a good point and illustrates that ICU is not a drop-in
> replacement for libc in all cases.
>
> I don't see a solution here that doesn't involve some rough edges,
> though. "Locale" is a generic term, and if we continue to insist that
> it really means a libc locale, then ICU will never be on an equal
> footing with libc, let alone the preferred provider.
Huge +1
IMHO the experience should be unified to the degree possible.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-05-24 16:31 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 07:51 [PATCH v1] Replace the char to bool array for null parameters Japin Li <[email protected]>
2023-05-22 20:09 Re: Order changes in PG16 since ICU introduction Daniel Verite <[email protected]>
2023-05-24 15:39 ` Re: Order changes in PG16 since ICU introduction Jeff Davis <[email protected]>
2023-05-24 16:31 ` Re: Order changes in PG16 since ICU introduction Joe Conway <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox