From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Daniel Verite <daniel@manitou-mail.org>
Subject: Re: ICU for global collation
Date: Thu, 10 Mar 2022 10:52:41 +0100
Message-ID: <29ae752f-80e9-8d31-601c-62cf01cc93d8@enterprisedb.com> (raw)
In-Reply-To: <20220305083830.lpz3k3yku5lmm5xs@jrouhaud>
References: <a6f807c2-2d11-2cf6-a524-cfbec56e7d5b@enterprisedb.com>
<20220118040246.dewl5pain2gkv2iv@jrouhaud>
<171c5f7c-43ee-dbe9-31f9-5e3eb06a54cf@enterprisedb.com>
<e381e36c-c7a2-2ec0-dbb4-da1dab3f0941@enterprisedb.com>
<20220121135119.ywszwl4353v5dzh5@jrouhaud>
<e2061949-f731-fd60-8b0b-3bae75095b4f@enterprisedb.com>
<20220121161331.xn7fhtfg5j2ucilb@jrouhaud>
<87e93704-b7f7-71f6-161d-8713e6334ed3@enterprisedb.com>
<704405ed-f1fa-d2a7-b9f9-48a07fb45648@enterprisedb.com>
<96a50803-72ba-8eec-948a-d87608ac15a0@enterprisedb.com>
<20220305083830.lpz3k3yku5lmm5xs@jrouhaud>
On 05.03.22 09:38, Julien Rouhaud wrote:
> @@ -168,18 +175,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
> errmsg("collation \"default\" cannot be copied")));
> }
>
> - if (localeEl)
> - {
> - collcollate = defGetString(localeEl);
> - collctype = defGetString(localeEl);
> - }
> [...]
>
> I tried to read the function and quickly got confused about whether possible
> problematic conditions could be reached or not and had protection or not. I
> think that DefineCollation is becoming more and more unreadable, with a mix of
> fromEl, !fromEl and either. Maybe it would be a good time to refactor the code
> a bit and have have something like
>
> if (fromEl)
> {
> // initialize all specific things
> }
> else
> {
> // initialize all specific things
> }
>
> // handle defaults and sanity checks
>
> What do you think?
How about the attached?
From c7b408ba646c5ee5f8c6ae84bec04ca4059ed08f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 10 Mar 2022 10:49:49 +0100
Subject: [PATCH] DefineCollation() code cleanup
Reorganize the code in DefineCollation() so that the parts using the
FROM clause and the parts not doing so are more cleanly separated. No
functionality change intended.
Reported-by: Julien Rouhaud <rjuju123@gmail.com>
---
src/backend/commands/collationcmds.c | 109 ++++++++++++++-------------
1 file changed, 57 insertions(+), 52 deletions(-)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.cindex 12fc2316f9..93df1d366c 100644--- a/src/backend/commands/collationcmds.c+++ b/src/backend/commands/collationcmds.c@@ -63,12 +63,11 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
DefElem *providerEl = NULL;
DefElem *deterministicEl = NULL;
DefElem *versionEl = NULL;
- char *collcollate = NULL;- char *collctype = NULL;- char *collproviderstr = NULL;- bool collisdeterministic = true;- int collencoding = 0;- char collprovider = 0;+ char *collcollate;+ char *collctype;+ bool collisdeterministic;+ int collencoding;+ char collprovider;
char *collversion = NULL;
Oid newoid;
ObjectAddress address;
@@ -167,65 +166,71 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("collation \"default\" cannot be copied")));
}
-- if (localeEl)+ else
{
- collcollate = defGetString(localeEl);- collctype = defGetString(localeEl);- }+ char *collproviderstr = NULL;- if (lccollateEl)- collcollate = defGetString(lccollateEl);+ collcollate = NULL;+ collctype = NULL;- if (lcctypeEl)- collctype = defGetString(lcctypeEl);+ if (localeEl)+ {+ collcollate = defGetString(localeEl);+ collctype = defGetString(localeEl);+ }- if (providerEl)- collproviderstr = defGetString(providerEl);+ if (lccollateEl)+ collcollate = defGetString(lccollateEl);- if (deterministicEl)- collisdeterministic = defGetBoolean(deterministicEl);+ if (lcctypeEl)+ collctype = defGetString(lcctypeEl);- if (versionEl)- collversion = defGetString(versionEl);+ if (providerEl)+ collproviderstr = defGetString(providerEl);- if (collproviderstr)- {- if (pg_strcasecmp(collproviderstr, "icu") == 0)- collprovider = COLLPROVIDER_ICU;- else if (pg_strcasecmp(collproviderstr, "libc") == 0)- collprovider = COLLPROVIDER_LIBC;+ if (deterministicEl)+ collisdeterministic = defGetBoolean(deterministicEl);+ else+ collisdeterministic = true;++ if (versionEl)+ collversion = defGetString(versionEl);++ if (collproviderstr)+ {+ if (pg_strcasecmp(collproviderstr, "icu") == 0)+ collprovider = COLLPROVIDER_ICU;+ else if (pg_strcasecmp(collproviderstr, "libc") == 0)+ collprovider = COLLPROVIDER_LIBC;+ else+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("unrecognized collation provider: %s",+ collproviderstr)));+ }
else
+ collprovider = COLLPROVIDER_LIBC;++ if (!collcollate)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("unrecognized collation provider: %s",- collproviderstr)));- }- else if (!fromEl)- collprovider = COLLPROVIDER_LIBC;-- if (!collcollate)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("parameter \"lc_collate\" must be specified")));+ errmsg("parameter \"lc_collate\" must be specified")));- if (!collctype)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("parameter \"lc_ctype\" must be specified")));+ if (!collctype)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("parameter \"lc_ctype\" must be specified")));- /*- * Nondeterministic collations are currently only supported with ICU- * because that's the only case where it can actually make a difference.- * So we can save writing the code for the other providers.- */- if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)- ereport(ERROR,- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),- errmsg("nondeterministic collations not supported with this provider")));+ /*+ * Nondeterministic collations are currently only supported with ICU+ * because that's the only case where it can actually make a difference.+ * So we can save writing the code for the other providers.+ */+ if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("nondeterministic collations not supported with this provider")));- if (!fromEl)- {
if (collprovider == COLLPROVIDER_ICU)
{
#ifdef USE_ICU
--
2.35.1
Attachments:0001-DefineCollation-code-cleanup.patch (text/plain, 4.8K) download
From c7b408ba646c5ee5f8c6ae84bec04ca4059ed08f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 10 Mar 2022 10:49:49 +0100
Subject: [PATCH] DefineCollation() code cleanup
Reorganize the code in DefineCollation() so that the parts using the
FROM clause and the parts not doing so are more cleanly separated. No
functionality change intended.
Reported-by: Julien Rouhaud <rjuju123@gmail.com>
---
src/backend/commands/collationcmds.c | 109 ++++++++++++++-------------
1 file changed, 57 insertions(+), 52 deletions(-)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.cindex 12fc2316f9..93df1d366c 100644--- a/src/backend/commands/collationcmds.c+++ b/src/backend/commands/collationcmds.c@@ -63,12 +63,11 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
DefElem *providerEl = NULL;
DefElem *deterministicEl = NULL;
DefElem *versionEl = NULL;
- char *collcollate = NULL;- char *collctype = NULL;- char *collproviderstr = NULL;- bool collisdeterministic = true;- int collencoding = 0;- char collprovider = 0;+ char *collcollate;+ char *collctype;+ bool collisdeterministic;+ int collencoding;+ char collprovider;
char *collversion = NULL;
Oid newoid;
ObjectAddress address;
@@ -167,65 +166,71 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("collation \"default\" cannot be copied")));
}
-- if (localeEl)+ else
{
- collcollate = defGetString(localeEl);- collctype = defGetString(localeEl);- }+ char *collproviderstr = NULL;- if (lccollateEl)- collcollate = defGetString(lccollateEl);+ collcollate = NULL;+ collctype = NULL;- if (lcctypeEl)- collctype = defGetString(lcctypeEl);+ if (localeEl)+ {+ collcollate = defGetString(localeEl);+ collctype = defGetString(localeEl);+ }- if (providerEl)- collproviderstr = defGetString(providerEl);+ if (lccollateEl)+ collcollate = defGetString(lccollateEl);- if (deterministicEl)- collisdeterministic = defGetBoolean(deterministicEl);+ if (lcctypeEl)+ collctype = defGetString(lcctypeEl);- if (versionEl)- collversion = defGetString(versionEl);+ if (providerEl)+ collproviderstr = defGetString(providerEl);- if (collproviderstr)- {- if (pg_strcasecmp(collproviderstr, "icu") == 0)- collprovider = COLLPROVIDER_ICU;- else if (pg_strcasecmp(collproviderstr, "libc") == 0)- collprovider = COLLPROVIDER_LIBC;+ if (deterministicEl)+ collisdeterministic = defGetBoolean(deterministicEl);+ else+ collisdeterministic = true;++ if (versionEl)+ collversion = defGetString(versionEl);++ if (collproviderstr)+ {+ if (pg_strcasecmp(collproviderstr, "icu") == 0)+ collprovider = COLLPROVIDER_ICU;+ else if (pg_strcasecmp(collproviderstr, "libc") == 0)+ collprovider = COLLPROVIDER_LIBC;+ else+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("unrecognized collation provider: %s",+ collproviderstr)));+ }
else
+ collprovider = COLLPROVIDER_LIBC;++ if (!collcollate)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("unrecognized collation provider: %s",- collproviderstr)));- }- else if (!fromEl)- collprovider = COLLPROVIDER_LIBC;-- if (!collcollate)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("parameter \"lc_collate\" must be specified")));+ errmsg("parameter \"lc_collate\" must be specified")));- if (!collctype)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("parameter \"lc_ctype\" must be specified")));+ if (!collctype)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("parameter \"lc_ctype\" must be specified")));- /*- * Nondeterministic collations are currently only supported with ICU- * because that's the only case where it can actually make a difference.- * So we can save writing the code for the other providers.- */- if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)- ereport(ERROR,- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),- errmsg("nondeterministic collations not supported with this provider")));+ /*+ * Nondeterministic collations are currently only supported with ICU+ * because that's the only case where it can actually make a difference.+ * So we can save writing the code for the other providers.+ */+ if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("nondeterministic collations not supported with this provider")));- if (!fromEl)- {
if (collprovider == COLLPROVIDER_ICU)
{
#ifdef USE_ICU
--
2.35.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, rjuju123@gmail.com, daniel@manitou-mail.org
Subject: Re: ICU for global collation
In-Reply-To: <29ae752f-80e9-8d31-601c-62cf01cc93d8@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox